diff mbox

SUNRPC: Prevent kernel stack corruption on long values of flush

Message ID 1342476086-21638-1-git-send-email-levinsasha928@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sasha Levin July 16, 2012, 10:01 p.m. UTC
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(-)

Comments

J. Bruce Fields July 18, 2012, 5:39 p.m. UTC | #1
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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jim Rees July 18, 2012, 8 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Jones July 18, 2012, 8:33 p.m. UTC | #3
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jim Rees July 18, 2012, 8:55 p.m. UTC | #4
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin Oct. 17, 2012, 5:59 p.m. UTC | #5
On Wed, Jul 18, 2012 at 1:39 PM, J. Bruce Fields <bfields@fieldses.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boaz Harrosh Oct. 17, 2012, 6:29 p.m. UTC | #6
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Oct. 17, 2012, 7:02 p.m. UTC | #7
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@fieldses.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight Oct. 18, 2012, 8:34 a.m. UTC | #8
> ...

> 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
diff mbox

Patch

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;