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 <[email protected]>
---
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
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.
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
>
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; \
})
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;
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
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.
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.
On Wed, Jul 18, 2012 at 1:39 PM, J. Bruce Fields <[email protected]> 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
PiAuLi4NCj4gbG9uZyBpcyBhbHdheXMgdGhlIHNhbWUgb3IgYmlnZ2VyIHRoZW4gYSBwb2ludGVy
DQo+IChBIHBvaW50ZXIgbXVzdCBhbHdheXMgZml0IGluIGEgbG9uZykNCj4gLi4uDQoNCkxpbnV4
IG1heSBtYWtlIHRoYXQgYXNzdW1wdGlvbiwgYnV0IGl0IGRvZXNuJ3QgaGF2ZQ0KdG8gYmUgdHJ1
ZS4gNjRiaXQgd2luZG93cyBzdGlsbCBoYXMgMzJiaXQgbG9uZy4NCkM5OSBpbnR0eXBlcy5oIGRl
ZmluZXMgW3VdaW50cHRyX3QgdG8gYmUgYW4gaW50ZWdyYWwgdHlwZQ0KdGhhdCBpcyBsYXJnZSBl
bm91Z2ggdG8gaG9sZCBhIHBvaW50ZXIgdG8gYW55IGRhdGEgaXRlbS4NCihUaGF0IGluIGl0c2Vs
ZiBpcyBwcm9ibGVtYXRpYyBmb3IgaW1wbGVtZW50YXRpb25zIHRoYXQNCmVuY29kZSBtdWx0aXBs
ZSBjaGFyYWN0ZXJzIGludG8gYSBtYWNoaW5lIHdvcmQgYW5kIG5lZWQNCnRvIHVzZSAnZmF0JyBw
b2ludGVycyBpbiBvcmRlciB0byBlbmNvZGUgdGhlIG9mZnNldC4pDQoNCglEYXZpZA0KDQo=
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
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 <[email protected]> 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.