RE: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
> ... > 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
... 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
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 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
On Wed, Jul 18, 2012 at 1:39 PM, 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! 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
On Wed, Jul 18, 2012 at 1:39 PM, J. Bruce Fields bfie...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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 bfie...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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: 833e64b4
[ 43.047329]
[ 43.049030] Pid: 6015, comm: trinity-child18 Tainted: GW
3.5.0-rc7-next-20120716-sasha #221
[ 43.050038] Call Trace:
[ 43.050435] [] panic+0xcd/0x1f4
[ 43.050931] [] ? read_flush.isra.7+0xe4/0x100
[ 43.051602] [] __stack_chk_fail+0x16/0x20
[ 43.052206] [] read_flush.isra.7+0xe4/0x100
[ 43.052951] [] ? read_flush_pipefs+0x30/0x30
[ 43.053594] [] read_flush_procfs+0x2c/0x30
[ 43.053596] [] proc_reg_read+0x9c/0xd0
[ 43.053596] [] ? proc_reg_write+0xd0/0xd0
[ 43.053596] [] do_loop_readv_writev+0x4b/0x90
[ 43.053596] [] do_readv_writev+0xf6/0x1d0
[ 43.053596] [] vfs_readv+0x3e/0x60
[ 43.053596] [] sys_readv+0x48/0xb0
[ 43.053596] [] system_call_fastpath+0x1a/0x1f
Signed-off-by: Sasha Levin
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] SUNRPC: Prevent kernel stack corruption on long values of flush
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: 833e64b4
[ 43.047329]
[ 43.049030] Pid: 6015, comm: trinity-child18 Tainted: GW
3.5.0-rc7-next-20120716-sasha #221
[ 43.050038] Call Trace:
[ 43.050435] [836c60c2] panic+0xcd/0x1f4
[ 43.050931] [833e64b4] ? read_flush.isra.7+0xe4/0x100
[ 43.051602] [810e94e6] __stack_chk_fail+0x16/0x20
[ 43.052206] [833e64b4] read_flush.isra.7+0xe4/0x100
[ 43.052951] [833e6500] ? read_flush_pipefs+0x30/0x30
[ 43.053594] [833e652c] read_flush_procfs+0x2c/0x30
[ 43.053596] [812b9a8c] proc_reg_read+0x9c/0xd0
[ 43.053596] [812b99f0] ? proc_reg_write+0xd0/0xd0
[ 43.053596] [81250d5b] do_loop_readv_writev+0x4b/0x90
[ 43.053596] [81250fd6] do_readv_writev+0xf6/0x1d0
[ 43.053596] [812510ee] vfs_readv+0x3e/0x60
[ 43.053596] [812511b8] sys_readv+0x48/0xb0
[ 43.053596] [8378167d] system_call_fastpath+0x1a/0x1f
Signed-off-by: Sasha Levin levinsasha...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
