| Message ID | 1342476086-21638-1-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
> ... > 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 --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;
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(-)