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 > > > > <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 <[email protected]> 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 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
