On 14/01/12, Tetsuo Handa wrote: > Tetsuo Handa wrote: Thank you for the heads up Tetsuo!

> > Geert Uytterhoeven wrote: > > > On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton > > > <akpm(a)linux-foundation.org&gt; 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(a)I-love.SAKURA.ne.jp&gt; > 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(a)I-love.SAKURA.ne.jp&gt; > --- > 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(a)I-love.SAKURA.ne.jp&gt; > 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(a)I-love.SAKURA.ne.jp&gt; > --- > 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(a)I-love.SAKURA.ne.jp&gt; > 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(a)I-love.SAKURA.ne.jp&gt; > --- > 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(a)I-love.SAKURA.ne.jp&gt; > 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(a)I-love.SAKURA.ne.jp&gt; > --- > 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