[PATCH] lib/vsprintf: add %pT format specifier
Richard Guy Briggs
rgb at redhat.com
Wed Feb 19 21:55:36 UTC 2014
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 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
> ----------------------------------------
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- 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