[PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Sasha Levin]

The buffer size in read_flush() is too small for the longest possible values
for it. This can lead to a kernel stack corruption:

[   43.047329] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff833e64b4
[   43.047329]
[   43.049030] Pid: 6015, comm: trinity-child18 Tainted: G        W    3.5.0-rc7-next-20120716-sasha #221
[   43.050038] Call Trace:
[   43.050435]  [<ffffffff836c60c2>] panic+0xcd/0x1f4
[   43.050931]  [<ffffffff833e64b4>] ? read_flush.isra.7+0xe4/0x100
[   43.051602]  [<ffffffff810e94e6>] __stack_chk_fail+0x16/0x20
[   43.052206]  [<ffffffff833e64b4>] read_flush.isra.7+0xe4/0x100
[   43.052951]  [<ffffffff833e6500>] ? read_flush_pipefs+0x30/0x30
[   43.053594]  [<ffffffff833e652c>] read_flush_procfs+0x2c/0x30
[   43.053596]  [<ffffffff812b9a8c>] proc_reg_read+0x9c/0xd0
[   43.053596]  [<ffffffff812b99f0>] ? proc_reg_write+0xd0/0xd0
[   43.053596]  [<ffffffff81250d5b>] do_loop_readv_writev+0x4b/0x90
[   43.053596]  [<ffffffff81250fd6>] do_readv_writev+0xf6/0x1d0
[   43.053596]  [<ffffffff812510ee>] vfs_readv+0x3e/0x60
[   43.053596]  [<ffffffff812511b8>] sys_readv+0x48/0xb0
[   43.053596]  [<ffffffff8378167d>] system_call_fastpath+0x1a/0x1f

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 net/sunrpc/cache.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 2afd2a8..f86d95e 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos,
 			  struct cache_detail *cd)
 {
-	char tbuf[20];
+	char tbuf[22];
 	unsigned long p = *ppos;
 	size_t len;
 
-	sprintf(tbuf, "%lu\n", convert_to_wallclock(cd->flush_time));
+	snprintf(tbuf, sizeof(tbuf), "%lu\n", convert_to_wallclock(cd->flush_time));
 	len = strlen(tbuf);
 	if (p >= len)
 		return 0;
-- 
1.7.8.6

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[J. Bruce Fields]

On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
> The buffer size in read_flush() is too small for the longest possible values
> for it. This can lead to a kernel stack corruption:

Thanks!

> 
> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
> index 2afd2a8..f86d95e 100644
> --- a/net/sunrpc/cache.c
> +++ b/net/sunrpc/cache.c
> @@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char __user *buf,
>  			  size_t count, loff_t *ppos,
>  			  struct cache_detail *cd)
>  {
> -	char tbuf[20];
> +	char tbuf[22];

I wonder how common this sort of calculation is in the kernel?  It might
provide some peace of mind to be able to write this something like

	char tbuf[MAXLEN_BASE10_UL + 2]  /* + 2 for final "\n\0" */

--b.

>  	unsigned long p = *ppos;
>  	size_t len;
>  
> -	sprintf(tbuf, "%lu\n", convert_to_wallclock(cd->flush_time));
> +	snprintf(tbuf, sizeof(tbuf), "%lu\n", convert_to_wallclock(cd->flush_time));
>  	len = strlen(tbuf);
>  	if (p >= len)
>  		return 0;
> -- 
> 1.7.8.6
> 

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Jim Rees]

J. Bruce Fields wrote:

  On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
  > The buffer size in read_flush() is too small for the longest possible values
  > for it. This can lead to a kernel stack corruption:
  
  Thanks!
  
  > 
  > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
  > index 2afd2a8..f86d95e 100644
  > --- a/net/sunrpc/cache.c
  > +++ b/net/sunrpc/cache.c
  > @@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char __user *buf,
  >  			  size_t count, loff_t *ppos,
  >  			  struct cache_detail *cd)
  >  {
  > -	char tbuf[20];
  > +	char tbuf[22];
  
  I wonder how common this sort of calculation is in the kernel?  It might
  provide some peace of mind to be able to write this something like
  
  	char tbuf[MAXLEN_BASE10_UL + 2]  /* + 2 for final "\n\0" */

You could use something like:

    char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */

since there are roughly 10 bits for every 3 decimal digits.

But I'm obviously confused, because I don't understand why tbuf needs to be
any more than 10 + 2.

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Dave Jones]

On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote:

 > You could use something like:
 > 
 >     char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */
 > 
 > since there are roughly 10 bits for every 3 decimal digits.
 > 
 > But I'm obviously confused, because I don't understand why tbuf needs to be
 > any more than 10 + 2.

Unsigned long isn't necessarily 32 bits.
On 64-bit systems %lu can be up to 18446744073709551615

	Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Jim Rees]

Dave Jones wrote:

  On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote:
  
   > You could use something like:
   > 
   >     char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */
   > 
   > since there are roughly 10 bits for every 3 decimal digits.
   > 
   > But I'm obviously confused, because I don't understand why tbuf needs to be
   > any more than 10 + 2.
  
  Unsigned long isn't necessarily 32 bits.
  On 64-bit systems %lu can be up to 18446744073709551615

Thanks.  You caught me thinking "Intel."  How embarrassing.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Boaz Harrosh]

On 07/18/2012 01:55 PM, Jim Rees wrote:
> Dave Jones wrote:
> 
>   
>   Unsigned long isn't necessarily 32 bits.
>   On 64-bit systems %lu can be up to 18446744073709551615
> 
> Thanks.  You caught me thinking "Intel."  How embarrassing.

What? why even on Intel-64 long is 64bit. long is always the
same or bigger then a pointer (A pointer must always fit
in a long)

On the other hand int is 32bit in Intel-64 unlike some
other CPUs where int(s) may get to be 64bit as well.

Cheers
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[David Laight]

> ...
> long is always the same or bigger then a pointer
> (A pointer must always fit in a long)
> ...

Linux may make that assumption, but it doesn't have
to be true. 64bit windows still has 32bit long.
C99 inttypes.h defines [u]intptr_t to be an integral type
that is large enough to hold a pointer to any data item.
(That in itself is problematic for implementations that
encode multiple characters into a machine word and need
to use 'fat' pointers in order to encode the offset.)

	David

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[J. Bruce Fields]

On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote:
> J. Bruce Fields wrote:
> 
>   On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
>   > The buffer size in read_flush() is too small for the longest possible values
>   > for it. This can lead to a kernel stack corruption:
>   
>   Thanks!
>   
>   > 
>   > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>   > index 2afd2a8..f86d95e 100644
>   > --- a/net/sunrpc/cache.c
>   > +++ b/net/sunrpc/cache.c
>   > @@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char __user *buf,
>   >  			  size_t count, loff_t *ppos,
>   >  			  struct cache_detail *cd)
>   >  {
>   > -	char tbuf[20];
>   > +	char tbuf[22];
>   
>   I wonder how common this sort of calculation is in the kernel?  It might
>   provide some peace of mind to be able to write this something like
>   
>   	char tbuf[MAXLEN_BASE10_UL + 2]  /* + 2 for final "\n\0" */
> 
> You could use something like:
> 
>     char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */
> 
> since there are roughly 10 bits for every 3 decimal digits.

So we could do something like this.  OK, I'm not sure I care enough.

--b.

diff --git a/include/linux/string.h b/include/linux/string.h
index e033564..ed34180 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -126,6 +126,10 @@ extern void argv_free(char **argv);
 extern bool sysfs_streq(const char *s1, const char *s2);
 extern int strtobool(const char *s, bool *res);
 
+/* length of the decimal representation of an unsigned integer.  Just an
+ * approximation, but it's right for types of size 1 to 36 bytes: */
+#define base10len(i) (sizeof(i) * 24 / 10 + 1)
+
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
 int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 2afd2a8..1dcd2b3 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1409,7 +1409,7 @@ static ssize_t read_flush(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos,
 			  struct cache_detail *cd)
 {
-	char tbuf[20];
+	char tbuf[base10len(unsigned long) + 2];
 	unsigned long p = *ppos;
 	size_t len;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Sasha Levin]

On 07/18/2012 11:33 PM, Sasha Levin wrote:
> On 07/18/2012 11:08 PM, J. Bruce Fields wrote:
>> On Wed, Jul 18, 2012 at 04:00:49PM -0400, Jim Rees wrote:
>>> J. Bruce Fields wrote:
>>>
>>>   On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
>>>   > The buffer size in read_flush() is too small for the longest possible values
>>>   > for it. This can lead to a kernel stack corruption:
>>>   
>>>   Thanks!
>>>   
>>>   > 
>>>   > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>>>   > index 2afd2a8..f86d95e 100644
>>>   > --- a/net/sunrpc/cache.c
>>>   > +++ b/net/sunrpc/cache.c
>>>   > @@ -1409,11 +1409,11 @@ static ssize_t read_flush(struct file *file, char __user *buf,
>>>   >  			  size_t count, loff_t *ppos,
>>>   >  			  struct cache_detail *cd)
>>>   >  {
>>>   > -	char tbuf[20];
>>>   > +	char tbuf[22];
>>>   
>>>   I wonder how common this sort of calculation is in the kernel?  It might
>>>   provide some peace of mind to be able to write this something like
>>>   
>>>   	char tbuf[MAXLEN_BASE10_UL + 2]  /* + 2 for final "\n\0" */
>>>
>>> You could use something like:
>>>
>>>     char tbuf[sizeof (unsigned long) * 24 / 10 + 1 + 2]; /* + 2 for final "\n\0" */
>>>
>>> since there are roughly 10 bits for every 3 decimal digits.
>>
>> So we could do something like this.  OK, I'm not sure I care enough.
>>
>> --b.
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index e033564..ed34180 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -126,6 +126,10 @@ extern void argv_free(char **argv);
>>  extern bool sysfs_streq(const char *s1, const char *s2);
>>  extern int strtobool(const char *s, bool *res);
>>  
>> +/* length of the decimal representation of an unsigned integer.  Just an
>> + * approximation, but it's right for types of size 1 to 36 bytes: */
>> +#define base10len(i) (sizeof(i) * 24 / 10 + 1)
>> +
>>  #ifdef CONFIG_BINARY_PRINTF
>>  int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
>>  int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index 2afd2a8..1dcd2b3 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -1409,7 +1409,7 @@ static ssize_t read_flush(struct file *file, char __user *buf,
>>  			  size_t count, loff_t *ppos,
>>  			  struct cache_detail *cd)
>>  {
>> -	char tbuf[20];
>> +	char tbuf[base10len(unsigned long) + 2];
>>  	unsigned long p = *ppos;
>>  	size_t len;
>>  
>>
> 
> Learning from what happened in this specific case, there are actually 2 issues here:
> 
>  - Array size was constant and too small, which is solved by the patch above.
>  - We were blindly trying to sprintf() into that array, this issue may pop back up if someone decides to change the format string forgetting to modify the array declaration.
> 
> What about adding the following itoa() type helper:

Fat fingers :(

Something like this (which obviously needs tons of work):

#define base10len(i) (sizeof(i) * 24 / 10 + 1)
#define itoa(x) \
({ \
        static char str[base10len(x)]; \
        sprintf(str, "%lu", (x)); \
        str; \
})


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Jim Rees]

Sasha Levin wrote:

  > Learning from what happened in this specific case, there are actually 2 issues here:
  > 
  >  - Array size was constant and too small, which is solved by the patch above.
  >  - We were blindly trying to sprintf() into that array, this issue may pop back up if someone decides to change the format string forgetting to modify the array declaration.
  > 

The original patch changed the sprintf to snprintf, and that still seems
like a good idea.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[Sasha Levin]

On Wed, Jul 18, 2012 at 1:39 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
>> The buffer size in read_flush() is too small for the longest possible values
>> for it. This can lead to a kernel stack corruption:
>
> Thanks!

I've just stumbled on this crash again, and noticed that this patch
never made it in.

Was it just a mixup, or is something still missing?


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush

[J. Bruce Fields]

On Wed, Oct 17, 2012 at 01:59:39PM -0400, Sasha Levin wrote:
> On Wed, Jul 18, 2012 at 1:39 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> > On Tue, Jul 17, 2012 at 12:01:26AM +0200, Sasha Levin wrote:
> >> The buffer size in read_flush() is too small for the longest possible values
> >> for it. This can lead to a kernel stack corruption:
> >
> > Thanks!
> 
> I've just stumbled on this crash again, and noticed that this patch
> never made it in.
> 
> Was it just a mixup, or is something still missing?

Oh, man, I guess I got distracted by the subsequent base10len()
discussion.

Added to my for-3.7 branch, I'll push that out after some tests and
hopefully send in a pull request tomorrow.

Thanks for noticing the ommission.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html