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

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

[Tetsuo Handa]

Hello.

Since addition of %pT itself seems to be agreed, I refreshed this patch using
linux-next-20140109. Please pick up if this patch is OK for you; I will start
making patches for killing most of direct ->comm readers.

Regards.
----------------------------------------
>From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 9 Jan 2014 21:32:22 +0900
Subject: [PATCH] lib/vsprintf: add %pT format specifier

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 %pT format specifier for printing task_struct->comm.
Currently %pT 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().

An example for converting direct ->comm users is shown below. Since many debug
printings use p == current, you can pass NULL instead of p if p == current.

  pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
  pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
---
 Documentation/printk-formats.txt |    6 ++++++
 lib/vsprintf.c                   |   20 +++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 6f4eb32..94459b4 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -184,6 +184,12 @@ dentry names:
 	equivalent of %s dentry->d_name.name we used to use, %pd<n> prints
 	n last components.  %pD does the same thing for struct file.
 
+task_struct comm name:
+
+        %pT
+
+        For printing task_struct->comm.
+
 struct va_format:
 
 	%pV
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 185b6d3..a97f18b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+static noinline_for_stack
+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);
+	name[sizeof(name) - 1] = '\0';
+	return string(buf, end, name, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1246,6 +1261,7 @@ int kptr_restrict __read_mostly;
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file
+ * - 'T' task_struct->comm
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1257,7 +1273,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
 
-	if (!ptr && *fmt != 'K') {
+	if (!ptr && *fmt != 'K' && *fmt != 'T') {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
@@ -1385,6 +1401,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+	case 'T':
+		return comm_name(buf, end, ptr, spec, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.1

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

[Kees Cook]

On Thu, Jan 9, 2014 at 4:52 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Hello.
>
> Since addition of %pT itself seems to be agreed, I refreshed this patch using
> linux-next-20140109. Please pick up if this patch is OK for you; I will start
> making patches for killing most of direct ->comm readers.

Looks good; thanks for chasing this. :)

>
> Regards.
> ----------------------------------------
> >From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 9 Jan 2014 21:32:22 +0900
> Subject: [PATCH] lib/vsprintf: add %pT format specifier
>
> 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 %pT format specifier for printing task_struct->comm.
> Currently %pT 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().
>
> An example for converting direct ->comm users is shown below. Since many debug
> printings use p == current, you can pass NULL instead of p if p == current.
>
>   pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
>   pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Hello.
> 
> Since addition of %pT itself seems to be agreed,

sort-of.  The reason I suggested inventing a new token was code
density: avoid pointlessly passing current all the time.

Oh well, whatever - this patch has other intentions.
	
> I refreshed this patch using
> linux-next-20140109. Please pick up if this patch is OK for you; I will start
> making patches for killing most of direct ->comm readers.
> 
> Regards.
> ----------------------------------------
> >From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 9 Jan 2014 21:32:22 +0900
> Subject: [PATCH] lib/vsprintf: add %pT format specifier
> 
> 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 %pT format specifier for printing task_struct->comm.
> Currently %pT 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.

Not completely accurate - RCU won't protect code which accesses ->comm
from interrupts.  Printing current->comm from irq is quite daft, but I
bet there's code that does it :(

As long as the kernel doesn't crash or otherwise misbehave when this
happens, I think we're OK.

(And I guess there's also non-daft code which accesses current->comm
from interrupt context: oops, panic, etc).

> But before modifying set_task_comm() to use RCU,
> we need to kill direct ->comm users who do not use get_task_comm().
> 
> An example for converting direct ->comm users is shown below. Since many debug
> printings use p == current, you can pass NULL instead of p if p == current.
> 
>   pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
>   pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> ---
>  Documentation/printk-formats.txt |    6 ++++++
>  lib/vsprintf.c                   |   20 +++++++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 6f4eb32..94459b4 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -184,6 +184,12 @@ dentry names:
>  	equivalent of %s dentry->d_name.name we used to use, %pd<n> prints
>  	n last components.  %pD does the same thing for struct file.
>  
> +task_struct comm name:
> +
> +        %pT
> +
> +        For printing task_struct->comm.
> +
>  struct va_format:
>  
>  	%pV
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 185b6d3..a97f18b 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
>  	return number(buf, end, num, spec);
>  }
>  
> +static noinline_for_stack

hm, does noinline_for_stack actually do anything useful here?  I
suspect it *increases* stack depth in the way comm_name() is used here.

> +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);
> +	name[sizeof(name) - 1] = '\0';

get_task_comm() uses strncpy()?

> +	return string(buf, end, name, spec);
> +}
> +

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

[Andrew Morton]

On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> This patch introduces %pT format specifier for printing task_struct->comm.
> Currently %pT 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().

On reflection...

It isn't obvious that this patch is sufficiently beneficial until we
have that RCU code in place.

So I could retain this patch in -mm until we have that all sorted out. 
And I'll have to avoid merging %pT users into mainline in the
meanwhile!

Am I wrong?  The patch seems fairly pointless as a standalone thing?

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

[Tetsuo Handa]

Andrew Morton wrote:
> On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > This patch introduces %pT format specifier for printing task_struct->comm.
> > Currently %pT 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().
> 
> On reflection...
> 
> It isn't obvious that this patch is sufficiently beneficial until we
> have that RCU code in place.
> 
> So I could retain this patch in -mm until we have that all sorted out. 
> And I'll have to avoid merging %pT users into mainline in the
> meanwhile!
> 
> Am I wrong?  The patch seems fairly pointless as a standalone thing?
> 

  Step 1: (targeted to 3.14-rc1)
    Add "%pT" format specifier and commcpy() wrapper function.

  Step 2: (started after step 1 is reflected to other git trees)
    Replace printk("%s", p->comm) with printk("%pT", p).
    Replace strcpy(buf, p->comm) with commcpy(buf, p).

  Step 3: (started after step 2 is reflected to other git trees)
    Add rcu_read_lock()/rcu_read_unlock() into commcpy().
    Modify set_task_comm() etc. to replace ->comm using RCU.

This patch and commcpy() patch ( https://lkml.org/lkml/2014/1/10/217 ) are
targeted to be merged into 3.14-rc1. This is because we can start writing
patches to use "%pT" or commcpy() only after other git trees have rebased
using 3.14-rc1. After all direct ->comm readers are replaced to use "%pT" or
commcpy(), I can start ->comm modifying functions to use RCU.

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

[Joe Perches]

On Sat, 2014-01-11 at 10:28 +0900, Tetsuo Handa wrote:
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", p->comm) with printk("%pT", p).

Replace printk("%s", current->comm) with printk("%pT", NULL);

?

>     Replace strcpy(buf, p->comm) with commcpy(buf, p).

>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.

Aren't steps 2 and 3 independent?

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

[Tetsuo Handa]

Joe Perches wrote:
> On Sat, 2014-01-11 at 10:28 +0900, Tetsuo Handa wrote:
> >   Step 1: (targeted to 3.14-rc1)
> >     Add "%pT" format specifier and commcpy() wrapper function.
> > 
> >   Step 2: (started after step 1 is reflected to other git trees)
> >     Replace printk("%s", p->comm) with printk("%pT", p).
> 
> Replace printk("%s", current->comm) with printk("%pT", NULL);
> 
> ?

Right. I forgot to add it.

> 
> >     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
> >   Step 3: (started after step 2 is reflected to other git trees)
> >     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
> >     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> Aren't steps 2 and 3 independent?
> 

If step 3 is merged before step 2 complete, those who are not using
"%pT" or commcpy() might crash due to reading RCU protected ->comm without
rcu_read_lock()/rcu_read_unlock(). 

Step 2 depends on step 1 for avoiding compile time errors.
Step 3 depends on step 2 for avoiding run time errors.

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

[Andrew Morton]

On Sat, 11 Jan 2014 10:28:51 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Andrew Morton wrote:
> > On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > 
> > > This patch introduces %pT format specifier for printing task_struct->comm.
> > > Currently %pT 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().
> > 
> > On reflection...
> > 
> > It isn't obvious that this patch is sufficiently beneficial until we
> > have that RCU code in place.
> > 
> > So I could retain this patch in -mm until we have that all sorted out. 
> > And I'll have to avoid merging %pT users into mainline in the
> > meanwhile!
> > 
> > Am I wrong?  The patch seems fairly pointless as a standalone thing?
> > 
> 
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", p->comm) with printk("%pT", p).
>     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.

In the absence of step 3, steps 1 and 2 are rather pointless churn.

So I think it would be better to merge (into mainline) steps 1 and 3
first and at the same time.  Then start thinking about step 2.

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

[Tetsuo Handa]

Andrew Morton wrote:
> > This patch introduces %pT format specifier for printing task_struct->comm.
> > Currently %pT 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.
> 
> Not completely accurate - RCU won't protect code which accesses ->comm
> from interrupts.  Printing current->comm from irq is quite daft, but I
> bet there's code that does it :(
> 
> As long as the kernel doesn't crash or otherwise misbehave when this
> happens, I think we're OK.
> 
> (And I guess there's also non-daft code which accesses current->comm
> from interrupt context: oops, panic, etc).

Excuse me, but are interrupts relevant?

I think that readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

will be protected from writers that wait freeing p->comm using
synchronize_rcu() or call_rcu().

Is synchronize_rcu() or call_rcu() insufficient for protecting readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

 from interrupts?

> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
> >  	return number(buf, end, num, spec);
> >  }
> >  
> > +static noinline_for_stack
> 
> hm, does noinline_for_stack actually do anything useful here?  I
> suspect it *increases* stack depth in the way comm_name() is used here.
> 
I just added noinline_for_stack as with other functions does.
But indeed, stack used by name[] is only 16 bytes but stack used by function
arguments are larger than 16 bytes. We should remove noinline_for_stack ?

> > +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);
> > +	name[sizeof(name) - 1] = '\0';
> 
> get_task_comm() uses strncpy()?

I think unconditionally copying 16 bytes using memcpy() is faster than
conditionally copying until '\0'.

> 
> > +	return string(buf, end, name, spec);
> > +}
> > +

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

[Joe Perches]

On Sat, 2014-01-11 at 10:59 +0900, Tetsuo Handa wrote:
> I just added noinline_for_stack as with other functions does.
> But indeed, stack used by name[] is only 16 bytes but stack used by function
> arguments are larger than 16 bytes. We should remove noinline_for_stack ?

My recollection is that certain gcc versions don't do
well with multiple inline functions that have stack
variables.

Instead of collapsing the variables, gcc will
accumulate the stack depth of all the inlines
args instead of reusing the stack.

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

[Tetsuo Handa]

Joe Perches wrote:
> On Sat, 2014-01-11 at 10:59 +0900, Tetsuo Handa wrote:
> > I just added noinline_for_stack as with other functions does.
> > But indeed, stack used by name[] is only 16 bytes but stack used by function
> > arguments are larger than 16 bytes. We should remove noinline_for_stack ?
> 
> My recollection is that certain gcc versions don't do
> well with multiple inline functions that have stack
> variables.
> 
> Instead of collapsing the variables, gcc will
> accumulate the stack depth of all the inlines
> args instead of reusing the stack.
> 

I tried what commit cf3b429b "vsprintf.c: use noinline_for_stack" says
using gcc 4.4.7, but it made no difference for stack usage.
Thus, it seems harmless to keep noinline_for_stack keyword.

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

[Tetsuo Handa]

Andrew Morton wrote:
> In the absence of step 3, steps 1 and 2 are rather pointless churn.
> 
> So I think it would be better to merge (into mainline) steps 1 and 3
> first and at the same time.  Then start thinking about step 2.

Unfortunately we can't.
Step 2 depends on step 1 for avoiding compile time errors.
Step 3 depends on step 2 for avoiding run time errors.

  Step 1: (targeted to 3.14-rc1)
    Add "%pT" format specifier and commcpy() wrapper function.

  Step 2: (started after step 1 is reflected to other git trees)
    Replace printk("%s", current->comm) with printk("%pT", NULL).
    Replace printk("%s", p->comm) with printk("%pT", p).
    Replace strcpy(buf, p->comm) with commcpy(buf, p).

  Step 3: (started after step 2 is reflected to other git trees)
    Add rcu_read_lock()/rcu_read_unlock() into commcpy().
    Modify set_task_comm() etc. to replace ->comm using RCU.

If step 3 is merged into mainline before step 2 complete, those who are not
using "%pT" or commcpy() might crash due to reading RCU protected ->comm
without rcu_read_lock()/rcu_read_unlock().


Let me confirm, Paul.

  I'm trying to change task_struct->comm to use RCU.
  At step 3, I'm planning to do

  static inline void *commcpy(void *buf, const struct task_struct *tsk)
  {
  	rcu_read_lock();
  	memcpy(buf, tsk->comm, TASK_COMM_LEN);
  	rcu_read_unlock();
  	return buf;
  }

  and let set_task_comm() wait for readers using synchronize_rcu() or
  call_rcu().

  Given that commcpy() can be called from any context, are synchronize_rcu()
  and call_rcu() sufficient for waiting for commcpy() users?

Regards.

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

[Paul E. McKenney]

On Sat, Jan 11, 2014 at 12:09:46PM +0900, Tetsuo Handa wrote:
> Andrew Morton wrote:
> > In the absence of step 3, steps 1 and 2 are rather pointless churn.
> > 
> > So I think it would be better to merge (into mainline) steps 1 and 3
> > first and at the same time.  Then start thinking about step 2.
> 
> Unfortunately we can't.
> Step 2 depends on step 1 for avoiding compile time errors.
> Step 3 depends on step 2 for avoiding run time errors.
> 
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", current->comm) with printk("%pT", NULL).
>     Replace printk("%s", p->comm) with printk("%pT", p).
>     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> If step 3 is merged into mainline before step 2 complete, those who are not
> using "%pT" or commcpy() might crash due to reading RCU protected ->comm
> without rcu_read_lock()/rcu_read_unlock().
> 
> 
> Let me confirm, Paul.
> 
>   I'm trying to change task_struct->comm to use RCU.
>   At step 3, I'm planning to do
> 
>   static inline void *commcpy(void *buf, const struct task_struct *tsk)
>   {
>   	rcu_read_lock();
>   	memcpy(buf, tsk->comm, TASK_COMM_LEN);
>   	rcu_read_unlock();
>   	return buf;
>   }
> 
>   and let set_task_comm() wait for readers using synchronize_rcu() or
>   call_rcu().
> 
>   Given that commcpy() can be called from any context, are synchronize_rcu()
>   and call_rcu() sufficient for waiting for commcpy() users?

Yep, that should work.

							Thanx, Paul

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

[Geert Uytterhoeven]

On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
<akpm@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.
And even if it wasn't, data between the string zero terminator and the end of
the buffer wouild be leaked.

>> +     name[sizeof(name) - 1] = '\0';

You can use strlcpy() here instead of memcpy and clear.

> get_task_comm() uses strncpy()?

char *get_task_comm(char *buf, struct task_struct *tsk)
{
        /* buf must be at least sizeof(tsk->comm) in size */
        task_lock(tsk);
        strncpy(buf, tsk->comm, sizeof(tsk->comm));
        task_unlock(tsk);
        return buf;
}

Is get_task_comm() used to prepare data for userspace?
strncpy() fills the remaining of the buffer with zeroes, so it avoids leaking
data.

Note that strncpy() may leave the buffer non-zero-terminated if the source
string is too long, but as set_task_comm() uses strlcpy(), this should never
be the case:

void set_task_comm(struct task_struct *tsk, char *buf)
{
        task_lock(tsk);
        trace_task_rename(tsk, buf);
        strlcpy(tsk->comm, buf, sizeof(tsk->comm));
        task_unlock(tsk);
        perf_event_comm(tsk);
}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Pavel Machek]

On Fri 2014-01-10 17:57:30, Andrew Morton wrote:
> On Sat, 11 Jan 2014 10:28:51 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > Andrew Morton wrote:
> > > On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > > 
> > > > This patch introduces %pT format specifier for printing task_struct->comm.
> > > > Currently %pT 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().
> > > 
> > > On reflection...
> > > 
> > > It isn't obvious that this patch is sufficiently beneficial until we
> > > have that RCU code in place.
> > > 
> > > So I could retain this patch in -mm until we have that all sorted out. 
> > > And I'll have to avoid merging %pT users into mainline in the
> > > meanwhile!
> > > 
> > > Am I wrong?  The patch seems fairly pointless as a standalone thing?
> > > 
> > 
> >   Step 1: (targeted to 3.14-rc1)
> >     Add "%pT" format specifier and commcpy() wrapper function.
> > 
> >   Step 2: (started after step 1 is reflected to other git trees)
> >     Replace printk("%s", p->comm) with printk("%pT", p).
> >     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> > 
> >   Step 3: (started after step 2 is reflected to other git trees)
> >     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
> >     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> In the absence of step 3, steps 1 and 2 are rather pointless churn.

Dunno. Given how often it appears in the code, it looks to me like
step 1 is worthwile cleanup on its own...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Tetsuo Handa]

Geert Uytterhoeven wrote:
> On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
> <akpm@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.
  The reader got some garbage bytes.

What is worse, there are some writers who changes p->comm without using
set_task_comm(). This means that we cannot prove that p->comm contains '\0',
making readers to get non-'\0' terminated comm name.

> And even if it wasn't, data between the string zero terminator and the end of
> the buffer wouild be leaked.
> 
> >> +     name[sizeof(name) - 1] = '\0';
> 
> You can use strlcpy() here instead of memcpy and clear.

For example, strlcpy() in lib/string.c is

size_t strlcpy(char *dest, const char *src, size_t size)
{
        size_t ret = strlen(src);

        if (size) {
                size_t len = (ret >= size) ? size - 1 : ret;
                memcpy(dest, src, len);
                dest[len] = '\0';
        }
        return ret;
}

and strlen(p->comm) can change after ret is calculated, leading to the result
as with strncpy(buf, p->comm, TASK_COMM_LEN).

> strncpy() fills the remaining of the buffer with zeroes, so it avoids leaking
> data.

I don't think that is true, as described above.

> 
> Note that strncpy() may leave the buffer non-zero-terminated if the source
> string is too long, but as set_task_comm() uses strlcpy(), this should never
> be the case:

I don't think that is true, as described above.

Trying to copy p->comm using strncpy() or strlcpy() is not safe. Copy 16 bytes
using memcpy(), and explicitly terminate with '\0' is the safer way, although
any approach may get some garbage bytes.

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

[Tetsuo Handa]

Tetsuo Handa wrote:
> Geert Uytterhoeven wrote:
> > On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
> > <akpm@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@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@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@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@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@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@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@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@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
----------------------------------------

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

[Richard Guy Briggs]

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@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@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@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@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@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@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@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@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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@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

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

[Richard Guy Briggs]

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@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@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@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@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@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@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@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@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@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@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

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

[Tetsuo Handa]

Richard Guy Briggs wrote:
> I assume another patchset is pending?

I'm waiting for comments on RCU commname patch at
https://lkml.org/lkml/2014/2/17/206 because Andrew Morton wants to see
how RCU commname change looks like before he merges commcpy() and %pT patches
( http://www.spinics.net/linux/fedora/linux-security-module/msg18069.html ).

But even if we come to conclusion that RCU commname does not worth merging,
I think commcpy() and %pT patches can be merged...