Re: [PATCH] lib/vsprintf: add %pT format specifier

Tetsuo Handa wrote:

> 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 ---------------------------------------- -- Linux-audit mailing list Linux-audit(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit