[PATCH] lib/vsprintf: add %pT format specifier
Richard Guy Briggs
rgb at redhat.com
Fri Feb 21 17:56:41 UTC 2014
On 14/02/19, Richard Guy Briggs wrote:
> On 14/01/12, Tetsuo Handa wrote:
> > Tetsuo Handa wrote:
>
> Thank you for the heads up Tetsuo!
I assume another patchset is pending? I echo AKPM's observation that
multiple patches per post is awkward. Are you able to use git
format-patch and git send-email? The discussion and reproducer would go
in the --cover-letter.
> > > Geert Uytterhoeven wrote:
> > > > On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
> > > > <akpm at linux-foundation.org> wrote:
> > > > >> +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> > > > >> + struct printf_spec spec, const char *fmt)
> > > > >> +{
> > > > >> + char name[TASK_COMM_LEN];
> > > > >> +
> > > > >> + /* Caller can pass NULL instead of current. */
> > > > >> + if (!tsk)
> > > > >> + tsk = current;
> > > > >> + /* Not using get_task_comm() in case I'm in IRQ context. */
> > > > >> + memcpy(name, tsk->comm, TASK_COMM_LEN);
> > > >
> > > > So this may copy more bytes than the actual string length of tsk->comm.
> > > > As this is a temporary buffer, that just wastes cycles.
> > >
> > > For example, strncpy() in arch/x86/lib/string_32.c is
> > >
> > > char *strncpy(char *dest, const char *src, size_t count)
> > > {
> > > int d0, d1, d2, d3;
> > > asm volatile("1:\tdecl %2\n\t"
> > > "js 2f\n\t"
> > > "lodsb\n\t"
> > > "stosb\n\t"
> > > "testb %%al,%%al\n\t"
> > > "jne 1b\n\t"
> > > "rep\n\t"
> > > "stosb\n"
> > > "2:"
> > > : "=&S" (d0), "=&D" (d1), "=&c" (d2), "=&a" (d3)
> > > : "" (src), "1" (dest), "2" (count) : "memory");
> > > return dest;
> > > }
> > >
> > > and strncpy() in lib/string.c is
> > >
> > > char *strncpy(char *dest, const char *src, size_t count)
> > > {
> > > char *tmp = dest;
> > >
> > > while (count) {
> > > if ((*tmp = *src) != 0)
> > > src++;
> > > tmp++;
> > > count--;
> > > }
> > > return dest;
> > > }
> > >
> > > while memcpy(name, tsk->comm, TASK_COMM_LEN) is
> > >
> > > u64 *dest = (u64 *) name;
> > > u64 *src = (u64 *) tsk->comm;
> > > *dest++ = *src++;
> > > *dest = *src;
> > >
> > > if sizeof(long) == 64. I can't understand why unconditionally copying 8 bytes *
> > > 2 consumes more cycles than conditionally copying up to 16 bytes...
> > >
> > > Also, strncpy() in lib/string.c is not safe for copying task_struct->comm, for
> > > task_struct->comm can change at any moment.
> > >
> > > Initial state:
> > >
> > > p->comm contains "secret_commname\0"
> > >
> > > A reader calls strncpy(buf, p->comm, 16)
> > > In strncpy() does
> > >
> > > char *dest = buf
> > > char *src = tsk->comm
> > > char *tmp = dest
> > > while (16)
> > > if ((buf[0] = 's') != 0)
> > > src++
> > > tmp++;
> > > 15
> > > while (15)
> > > if ((buf[1] = 'e') != 0)
> > > src++
> > > tmp++
> > > 14
> > >
> > > At this moment preemption happens, and a writer jumps in.
> > > The writer calls set_task_comm(p, "x").
> > > Now p->comm contains "x\0cret_commname\0".
> > > The preemption ends and the reader continues the loop.
> > > Now *src == '\0' but continues copying.
> > >
> > > while (14)
> > > if ((buf[2] = 'c') != 0)
> > > src++
> > > tmp++
> > > 13
> > > (...snipped...)
> > > while (1)
> > > if ((buf[15] = '\0') != 0)
> > > tmp++
> > > 0
> > > return dest
> > >
> > > and gets "xecret_commname\0" in the buf.
> >
> > Oops, my example was bad, though the conclusion does not changte.
> > Start with "Here We Go\0\0\0\0\0\0", and a preempted writer changes it to
> > "Let's Go\0\0\0\0\0\0\0\0" when a reader has copied 'H' 'e' 'r' 'e'. Then,
> > the reader gets "Heres Go\0\0\0\0\0\0\0\0" in the buf.
> >
> > What I wanted to say is: Do not use strncpy() or strlcpy() for copying
> > task_struct->comm to temporary buffer, for it can be changed while reading it.
> >
> >
> > Hello, audit subsystem users.
> > Below are patches for avoiding racing in audit logs.
> > ----------------------------------------
> > >From de04a5b08b611293b05b4b4fcc82dc1cd1b89ac3 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > Date: Sun, 12 Jan 2014 16:28:12 +0900
> > Subject: [PATCH 1/4] exec: Add wrapper function for reading task_struct->comm.
> >
> > Since task_struct->comm can be modified by other threads while the current
> > thread is reading it, it is recommended to use get_task_comm() for reading it.
> >
> > However, since get_task_comm() holds task_struct->alloc_lock spinlock,
> > some users cannot use get_task_comm(). Also, a lot of users are directly
> > reading from task_struct->comm even if they can use get_task_comm().
> > Such users might obtain inconsistent result.
> >
> > This patch introduces a wrapper function for reading task_struct->comm .
> > Currently this function does not provide consistency. I'm planning to change to
> > use RCU in the future. By using RCU, the comm name read from task_struct->comm
> > will be guaranteed to be consistent. But before modifying set_task_comm() to
> > use RCU, we need to kill direct ->comm users who do not use get_task_comm().
> >
> > Users directly reading from task_struct->comm for printing purpose can use
> > %pT format specifier rather than this wrapper function.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > ---
> > include/linux/sched.h | 18 ++++++++++++++++++
> > 1 files changed, 18 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 53f97eb..a31e148 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1665,6 +1665,24 @@ static inline cputime_t task_gtime(struct task_struct *t)
> > extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st);
> > extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st);
> >
> > +/**
> > + * commcpy - Copy task_struct->comm to buffer.
> > + *
> > + * @buf: Buffer to copy @tsk->comm which must be at least TASK_COMM_LEN bytes.
> > + * @tsk: Pointer to "struct task_struct".
> > + *
> > + * Returns @buf .
> > + *
> > + * Please use this wrapper function which will be updated in the future to read
> > + * @tsk->comm in a consistent way using RCU.
> > + */
> > +static inline char *commcpy(char *buf, const struct task_struct *tsk)
> > +{
> > + memcpy(buf, tsk->comm, TASK_COMM_LEN);
> > + buf[TASK_COMM_LEN - 1] = '\0';
> > + return buf;
> > +}
> > +
> > /*
> > * Per process flags
> > */
> > --
> > 1.7.1
> > ----------------------------------------
> > >From a09631ee2536d581b3c713690cf134cc84c8cce9 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > Date: Sun, 12 Jan 2014 16:36:21 +0900
> > Subject: [PATCH 2/4] LSM: Pass comm name via commcpy()
> >
> > When we pass task->comm to audit_log_untrustedstring(), we need to pass a
> > snapshot of it using commcpy() because task->comm can be changed from
> > "HelloLinuxWorld\0" (a string where
> > audit_string_contains_control("HelloLinuxWorld\0", 15) would return 0) to
> > "Good Morning\0\0\0\0" (a string where
> > audit_string_contains_control("Good Morning\0\0\0\0", 15) would return 1)
> > during a call to audit_log_untrustedstring(ab, task->comm). As a result,
> > the audit log will contain unexpected bytes (e.g. '"' and '\0') and might
> > confuse users who expect that the audit log does not contain such bytes.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > ---
> > security/lsm_audit.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 9a62045..a6c9152 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -212,6 +212,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> > struct common_audit_data *a)
> > {
> > struct task_struct *tsk = current;
> > + char name[TASK_COMM_LEN];
> >
> > /*
> > * To keep stack sizes in check force programers to notice if they
> > @@ -221,7 +222,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> > BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> >
> > audit_log_format(ab, " pid=%d comm=", tsk->pid);
> > - audit_log_untrustedstring(ab, tsk->comm);
> > + audit_log_untrustedstring(ab, commcpy(name, tsk));
> >
> > switch (a->type) {
> > case LSM_AUDIT_DATA_NONE:
> > @@ -280,7 +281,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> > tsk = a->u.tsk;
> > if (tsk && tsk->pid) {
> > audit_log_format(ab, " pid=%d comm=", tsk->pid);
> > - audit_log_untrustedstring(ab, tsk->comm);
> > + audit_log_untrustedstring(ab, commcpy(name, tsk));
> > }
> > break;
> > case LSM_AUDIT_DATA_NET:
> > --
> > 1.7.1
> > ----------------------------------------
> > >From a3679132e7c22e6c74e5cfc36237656e5b252c52 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > Date: Sun, 12 Jan 2014 16:38:32 +0900
> > Subject: [PATCH 3/4] Integrity: Pass comm name via commcpy()
> >
> > When we pass task->comm to audit_log_untrustedstring(), we need to pass a
> > snapshot of it using commcpy() because task->comm can be changed from
> > "HelloLinuxWorld\0" (a string where
> > audit_string_contains_control("HelloLinuxWorld\0", 15) would return 0) to
> > "Good Morning\0\0\0\0" (a string where
> > audit_string_contains_control("Good Morning\0\0\0\0", 15) would return 1)
> > during a call to audit_log_untrustedstring(ab, task->comm). As a result,
> > the audit log will contain unexpected bytes (e.g. '"' and '\0') and might
> > confuse users who expect that the audit log does not contain such bytes.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > ---
> > security/integrity/integrity_audit.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> > index d7efb30..eb853d9 100644
> > --- a/security/integrity/integrity_audit.c
> > +++ b/security/integrity/integrity_audit.c
> > @@ -33,6 +33,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
> > const char *cause, int result, int audit_info)
> > {
> > struct audit_buffer *ab;
> > + char name[TASK_COMM_LEN];
> >
> > if (!integrity_audit_info && audit_info == 1) /* Skip info messages */
> > return;
> > @@ -49,7 +50,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
> > audit_log_format(ab, " cause=");
> > audit_log_string(ab, cause);
> > audit_log_format(ab, " comm=");
> > - audit_log_untrustedstring(ab, current->comm);
> > + audit_log_untrustedstring(ab, commcpy(name, current));
> > if (fname) {
> > audit_log_format(ab, " name=");
> > audit_log_untrustedstring(ab, fname);
> > --
> > 1.7.1
> > ----------------------------------------
> > >From 8ac36b53256b1495ee3c12f3b52deabdd3e67d72 Mon Sep 17 00:00:00 2001
> > From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > Date: Sun, 12 Jan 2014 16:42:50 +0900
> > Subject: [PATCH 4/4] Audit: Pass comm name via commcpy()
> >
> > When we pass task->comm to audit_log_untrustedstring(), we need to pass a
> > snapshot of it using commcpy() because task->comm can be changed from
> > "HelloLinuxWorld\0" (a string where
> > audit_string_contains_control("HelloLinuxWorld\0", 15) would return 0) to
> > "Good Morning\0\0\0\0" (a string where
> > audit_string_contains_control("Good Morning\0\0\0\0", 15) would return 1)
> > during a call to audit_log_untrustedstring(ab, task->comm). As a result,
> > the audit log will contain unexpected bytes (e.g. '"' and '\0') and might
> > confuse users who expect that the audit log does not contain such bytes.
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > ---
> > kernel/auditsc.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 90594c9..3b1bf3c 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2352,6 +2352,7 @@ static void audit_log_task(struct audit_buffer *ab)
> > kuid_t auid, uid;
> > kgid_t gid;
> > unsigned int sessionid;
> > + char name[TASK_COMM_LEN];
> >
> > auid = audit_get_loginuid(current);
> > sessionid = audit_get_sessionid(current);
> > @@ -2364,7 +2365,7 @@ static void audit_log_task(struct audit_buffer *ab)
> > sessionid);
> > audit_log_task_context(ab);
> > audit_log_format(ab, " pid=%d comm=", current->pid);
> > - audit_log_untrustedstring(ab, current->comm);
> > + audit_log_untrustedstring(ab, commcpy(name, current));
> > }
> >
> > static void audit_log_abend(struct audit_buffer *ab, char *reason, long signr)
> > --
> > 1.7.1
>
> - RGB
- RGB
--
Richard Guy Briggs <rbriggs at redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
More information about the Linux-audit
mailing list