2013-04-11 03:16:00

by Chen Gang

[permalink] [raw]
Subject: [PATCH] kernel: kallsyms: memory override issue, need check destination buffer length


We don't export any symbols > 128 characters, but if we did then
kallsyms_expand_symbol() would overflow the buffer handed to it.
So we need check destination buffer length when copying.

the related test:
if we define an EXPORT function which name more than 128.
will panic when call kallsyms_lookup_name by init_kprobes on booting.
after check the length (provide this patch), it is ok.

Implementaion:
add additional destination buffer length parameter (maxlen)
if uncompressed string is too long (>= maxlen), it will be truncated.
not check the parameters whether valid, since it is a static function.


Signed-off-by: Chen Gang <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
---
kernel/kallsyms.c | 26 +++++++++++++++++---------
1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 2169fee..ae7b90d 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -84,12 +84,15 @@ static int is_ksym_addr(unsigned long addr)
/*
* Expand a compressed symbol data into the resulting uncompressed string,
+ * if uncompressed string is too long (>= maxlen), it will be truncated,
* given the offset to where the symbol is in the compressed stream.
*/
-static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
+static unsigned int kallsyms_expand_symbol(unsigned int off,
+ char *result, int maxlen)
{
int len, skipped_first = 0;
const u8 *tptr, *data;
+ char *begin = result;
/* Get the compressed symbol length from the first symbol byte. */
data = &kallsyms_names[off];
@@ -113,14 +116,16 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
while (*tptr) {
if (skipped_first) {
- *result = *tptr;
- result++;
+ *result++ = *tptr;
+ if (result - begin == maxlen - 1)
+ goto tail;
} else
skipped_first = 1;
tptr++;
}
}
+tail:
*result = '\0';
/* Return to offset to the next symbol. */
@@ -176,7 +181,7 @@ unsigned long kallsyms_lookup_name(const char *name)
unsigned int off;
for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
if (strcmp(namebuf, name) == 0)
return kallsyms_addresses[i];
@@ -195,7 +200,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
int ret;
for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
if (ret != 0)
return ret;
@@ -294,7 +299,8 @@ const char *kallsyms_lookup(unsigned long addr,
pos = get_symbol_pos(addr, symbolsize, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ namebuf, sizeof(namebuf));
if (modname)
*modname = NULL;
return namebuf;
@@ -315,7 +321,8 @@ int lookup_symbol_name(unsigned long addr, char *symname)
pos = get_symbol_pos(addr, NULL, NULL);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), symname);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ symname, sizeof(symname));
return 0;
}
/* See if it's in a module. */
@@ -333,7 +340,8 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
pos = get_symbol_pos(addr, size, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), name);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ name, sizeof(name));
modname[0] = '\0';
return 0;
}
@@ -463,7 +471,7 @@ static unsigned long get_ksymbol_core(struct kallsym_iter *iter)
iter->type = kallsyms_get_symbol_type(off);
- off = kallsyms_expand_symbol(off, iter->name);
+ off = kallsyms_expand_symbol(off, iter->name, sizeof(iter->name));
return off - iter->nameoff;
}
--
1.7.7.6


2013-04-11 04:08:37

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH] kernel: kallsyms: memory override issue, need check destination buffer length

Chen Gang <[email protected]> writes:
> We don't export any symbols > 128 characters, but if we did then
> kallsyms_expand_symbol() would overflow the buffer handed to it.
> So we need check destination buffer length when copying.
>
> the related test:
> if we define an EXPORT function which name more than 128.
> will panic when call kallsyms_lookup_name by init_kprobes on booting.
> after check the length (provide this patch), it is ok.
>
> Implementaion:
> add additional destination buffer length parameter (maxlen)
> if uncompressed string is too long (>= maxlen), it will be truncated.
> not check the parameters whether valid, since it is a static function.
>
>
> Signed-off-by: Chen Gang <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>

??!!! I never signed this off! I've never seen it before!

Minor comments below:

> -static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
> +static unsigned int kallsyms_expand_symbol(unsigned int off,
> + char *result, int maxlen)

'size_t maxlen' would be more explicit.

> {
> int len, skipped_first = 0;
> const u8 *tptr, *data;
> + char *begin = result;
> /* Get the compressed symbol length from the first symbol byte. */
> data = &kallsyms_names[off];
> @@ -113,14 +116,16 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
> while (*tptr) {
> if (skipped_first) {
> - *result = *tptr;
> - result++;
> + *result++ = *tptr;
> + if (result - begin == maxlen - 1)
> + goto tail;

You could just decrement maxlen instead, and handle maxlen == 0 at the
same time:

if (maxlen <= 1)
goto tail;
*result = *tptr;
result++;
maxlen--;

> @@ -176,7 +181,7 @@ unsigned long kallsyms_lookup_name(const char *name)
> unsigned int off;
> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> - off = kallsyms_expand_symbol(off, namebuf);
> + off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
> if (strcmp(namebuf, name) == 0)
> return kallsyms_addresses[i];

I prefer to use ARRAY_SIZE(namebuf) instead of sizeof(namebuf). That
way we break compile if the declaration is changed from an array to a
pointer one day.

Same for the others.

Thanks,
Rusty.

2013-04-11 04:20:08

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH] kernel: kallsyms: memory override issue, need check destination buffer length

On 2013年04月11日 12:08, Rusty Russell wrote:
> Chen Gang <[email protected]> writes:
>> We don't export any symbols > 128 characters, but if we did then
>> kallsyms_expand_symbol() would overflow the buffer handed to it.
>> So we need check destination buffer length when copying.
>>
>> the related test:
>> if we define an EXPORT function which name more than 128.
>> will panic when call kallsyms_lookup_name by init_kprobes on booting.
>> after check the length (provide this patch), it is ok.
>>
>> Implementaion:
>> add additional destination buffer length parameter (maxlen)
>> if uncompressed string is too long (>= maxlen), it will be truncated.
>> not check the parameters whether valid, since it is a static function.
>>
>>
>> Signed-off-by: Chen Gang <[email protected]>
>> Signed-off-by: Rusty Russell <[email protected]>
>
> ??!!! I never signed this off! I've never seen it before!
>

ok, thanks. I did not quite understand the rule of it.

originally, I think since you find it, I prefer to mention about you
in somewhere (e.g. mark as Reported-by ??)

could you provide additional suggestions for it ?

:-)


> Minor comments below:
>

I don't think they are 'minor comments'. I should improve them.

thank you for your work.

:-)


gchen.


>> -static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
>> +static unsigned int kallsyms_expand_symbol(unsigned int off,
>> + char *result, int maxlen)
>
> 'size_t maxlen' would be more explicit.
>
>> {
>> int len, skipped_first = 0;
>> const u8 *tptr, *data;
>> + char *begin = result;
>> /* Get the compressed symbol length from the first symbol byte. */
>> data = &kallsyms_names[off];
>> @@ -113,14 +116,16 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
>> while (*tptr) {
>> if (skipped_first) {
>> - *result = *tptr;
>> - result++;
>> + *result++ = *tptr;
>> + if (result - begin == maxlen - 1)
>> + goto tail;
>
> You could just decrement maxlen instead, and handle maxlen == 0 at the
> same time:
>
> if (maxlen <= 1)
> goto tail;
> *result = *tptr;
> result++;
> maxlen--;
>
>> @@ -176,7 +181,7 @@ unsigned long kallsyms_lookup_name(const char *name)
>> unsigned int off;
>> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
>> - off = kallsyms_expand_symbol(off, namebuf);
>> + off = kallsyms_expand_symbol(off, namebuf, sizeof(namebuf));
>> if (strcmp(namebuf, name) == 0)
>> return kallsyms_addresses[i];
>
> I prefer to use ARRAY_SIZE(namebuf) instead of sizeof(namebuf). That
> way we break compile if the declaration is changed from an array to a
> pointer one day.
>
> Same for the others.
>
> Thanks,
> Rusty.
>
>


--
Chen Gang

Asianux Corporation

2013-04-11 05:33:29

by Chen Gang

[permalink] [raw]
Subject: [PATCH v2] kernel: kallsyms: memory override issue, need check destination buffer length


We don't export any symbols > 128 characters, but if we did then
kallsyms_expand_symbol() would overflow the buffer handed to it.
So we need check destination buffer length when copying.

the related test:
if we define an EXPORT function which name more than 128.
will panic when call kallsyms_lookup_name by init_kprobes on booting.
after check the length (provide this patch), it is ok.

Implementaion:
add additional destination buffer length parameter (maxlen)
if uncompressed string is too long (>= maxlen), it will be truncated.
not check the parameters whether valid, since it is a static function.


Signed-off-by: Chen Gang <[email protected]>
---
kernel/kallsyms.c | 26 ++++++++++++++++++--------
1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 2169fee..43d8150 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -84,9 +84,11 @@ static int is_ksym_addr(unsigned long addr)

/*
* Expand a compressed symbol data into the resulting uncompressed string,
+ * if uncompressed string is too long (>= maxlen), it will be truncated,
* given the offset to where the symbol is in the compressed stream.
*/
-static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
+static unsigned int kallsyms_expand_symbol(unsigned int off,
+ char *result, size_t maxlen)
{
int len, skipped_first = 0;
const u8 *tptr, *data;
@@ -113,15 +115,20 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)

while (*tptr) {
if (skipped_first) {
+ if (maxlen <= 1)
+ goto tail;
*result = *tptr;
result++;
+ maxlen--;
} else
skipped_first = 1;
tptr++;
}
}

- *result = '\0';
+tail:
+ if (maxlen)
+ *result = '\0';

/* Return to offset to the next symbol. */
return off;
@@ -176,7 +183,7 @@ unsigned long kallsyms_lookup_name(const char *name)
unsigned int off;

for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));

if (strcmp(namebuf, name) == 0)
return kallsyms_addresses[i];
@@ -195,7 +202,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
int ret;

for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
if (ret != 0)
return ret;
@@ -294,7 +301,8 @@ const char *kallsyms_lookup(unsigned long addr,

pos = get_symbol_pos(addr, symbolsize, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ namebuf, ARRAY_SIZE(namebuf));
if (modname)
*modname = NULL;
return namebuf;
@@ -315,7 +323,8 @@ int lookup_symbol_name(unsigned long addr, char *symname)

pos = get_symbol_pos(addr, NULL, NULL);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), symname);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ symname, ARRAY_SIZE(symname));
return 0;
}
/* See if it's in a module. */
@@ -333,7 +342,8 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,

pos = get_symbol_pos(addr, size, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), name);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ name, ARRAY_SIZE(name));
modname[0] = '\0';
return 0;
}
@@ -463,7 +473,7 @@ static unsigned long get_ksymbol_core(struct kallsym_iter *iter)

iter->type = kallsyms_get_symbol_type(off);

- off = kallsyms_expand_symbol(off, iter->name);
+ off = kallsyms_expand_symbol(off, iter->name, ARRAY_SIZE(iter->name));

return off - iter->nameoff;
}
--
1.7.7.6

2013-04-12 09:49:17

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH v2] kernel: kallsyms: memory override issue, need check destination buffer length

Hello Rusty Russell:

please help to review this patch whether is OK, when you have time.

thanks.

gchen.

On 2013年04月11日 13:32, Chen Gang wrote:
>
> We don't export any symbols > 128 characters, but if we did then
> kallsyms_expand_symbol() would overflow the buffer handed to it.
> So we need check destination buffer length when copying.
>
> the related test:
> if we define an EXPORT function which name more than 128.
> will panic when call kallsyms_lookup_name by init_kprobes on booting.
> after check the length (provide this patch), it is ok.
>
> Implementaion:
> add additional destination buffer length parameter (maxlen)
> if uncompressed string is too long (>= maxlen), it will be truncated.
> not check the parameters whether valid, since it is a static function.
>
>
> Signed-off-by: Chen Gang <[email protected]>
> ---
> kernel/kallsyms.c | 26 ++++++++++++++++++--------
> 1 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 2169fee..43d8150 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -84,9 +84,11 @@ static int is_ksym_addr(unsigned long addr)
>
> /*
> * Expand a compressed symbol data into the resulting uncompressed string,
> + * if uncompressed string is too long (>= maxlen), it will be truncated,
> * given the offset to where the symbol is in the compressed stream.
> */
> -static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
> +static unsigned int kallsyms_expand_symbol(unsigned int off,
> + char *result, size_t maxlen)
> {
> int len, skipped_first = 0;
> const u8 *tptr, *data;
> @@ -113,15 +115,20 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
>
> while (*tptr) {
> if (skipped_first) {
> + if (maxlen <= 1)
> + goto tail;
> *result = *tptr;
> result++;
> + maxlen--;
> } else
> skipped_first = 1;
> tptr++;
> }
> }
>
> - *result = '\0';
> +tail:
> + if (maxlen)
> + *result = '\0';
>
> /* Return to offset to the next symbol. */
> return off;
> @@ -176,7 +183,7 @@ unsigned long kallsyms_lookup_name(const char *name)
> unsigned int off;
>
> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> - off = kallsyms_expand_symbol(off, namebuf);
> + off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
>
> if (strcmp(namebuf, name) == 0)
> return kallsyms_addresses[i];
> @@ -195,7 +202,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
> int ret;
>
> for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> - off = kallsyms_expand_symbol(off, namebuf);
> + off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
> if (ret != 0)
> return ret;
> @@ -294,7 +301,8 @@ const char *kallsyms_lookup(unsigned long addr,
>
> pos = get_symbol_pos(addr, symbolsize, offset);
> /* Grab name */
> - kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
> + kallsyms_expand_symbol(get_symbol_offset(pos),
> + namebuf, ARRAY_SIZE(namebuf));
> if (modname)
> *modname = NULL;
> return namebuf;
> @@ -315,7 +323,8 @@ int lookup_symbol_name(unsigned long addr, char *symname)
>
> pos = get_symbol_pos(addr, NULL, NULL);
> /* Grab name */
> - kallsyms_expand_symbol(get_symbol_offset(pos), symname);
> + kallsyms_expand_symbol(get_symbol_offset(pos),
> + symname, ARRAY_SIZE(symname));
> return 0;
> }
> /* See if it's in a module. */
> @@ -333,7 +342,8 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
>
> pos = get_symbol_pos(addr, size, offset);
> /* Grab name */
> - kallsyms_expand_symbol(get_symbol_offset(pos), name);
> + kallsyms_expand_symbol(get_symbol_offset(pos),
> + name, ARRAY_SIZE(name));
> modname[0] = '\0';
> return 0;
> }
> @@ -463,7 +473,7 @@ static unsigned long get_ksymbol_core(struct kallsym_iter *iter)
>
> iter->type = kallsyms_get_symbol_type(off);
>
> - off = kallsyms_expand_symbol(off, iter->name);
> + off = kallsyms_expand_symbol(off, iter->name, ARRAY_SIZE(iter->name));
>
> return off - iter->nameoff;
> }
>


--
Chen Gang

Asianux Corporation

2013-04-15 03:50:55

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH v2] kernel: kallsyms: memory override issue, need check destination buffer length

Chen Gang <[email protected]> writes:
> We don't export any symbols > 128 characters, but if we did then
> kallsyms_expand_symbol() would overflow the buffer handed to it.
> So we need check destination buffer length when copying.
>
> the related test:
> if we define an EXPORT function which name more than 128.
> will panic when call kallsyms_lookup_name by init_kprobes on booting.
> after check the length (provide this patch), it is ok.
>
> Implementaion:
> add additional destination buffer length parameter (maxlen)
> if uncompressed string is too long (>= maxlen), it will be truncated.
> not check the parameters whether valid, since it is a static function.

Found a bug already:

kernel/kallsyms.c: In function ‘kallsyms_lookup’:
kernel/kallsyms.c:305:78: error: negative width in bit-field ‘<anonymous>’
kernel/kallsyms.c: In function ‘lookup_symbol_name’:
kernel/kallsyms.c:327:78: error: negative width in bit-field ‘<anonymous>’
kernel/kallsyms.c: In function ‘lookup_symbol_attrs’:
kernel/kallsyms.c:346:69: error: negative width in bit-field ‘<anonymous>’

Cheers,
Rusty.

2013-04-15 04:30:55

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH v2] kernel: kallsyms: memory override issue, need check destination buffer length

On 2013年04月15日 10:05, Rusty Russell wrote:
> Chen Gang <[email protected]> writes:
>> > We don't export any symbols > 128 characters, but if we did then
>> > kallsyms_expand_symbol() would overflow the buffer handed to it.
>> > So we need check destination buffer length when copying.
>> >
>> > the related test:
>> > if we define an EXPORT function which name more than 128.
>> > will panic when call kallsyms_lookup_name by init_kprobes on booting.
>> > after check the length (provide this patch), it is ok.
>> >
>> > Implementaion:
>> > add additional destination buffer length parameter (maxlen)
>> > if uncompressed string is too long (>= maxlen), it will be truncated.
>> > not check the parameters whether valid, since it is a static function.
> Found a bug already:
>
> kernel/kallsyms.c: In function ‘kallsyms_lookup’:
> kernel/kallsyms.c:305:78: error: negative width in bit-field ‘<anonymous>’
> kernel/kallsyms.c: In function ‘lookup_symbol_name’:
> kernel/kallsyms.c:327:78: error: negative width in bit-field ‘<anonymous>’
> kernel/kallsyms.c: In function ‘lookup_symbol_attrs’:
> kernel/kallsyms.c:346:69: error: negative width in bit-field ‘<anonymous>’
>

oh... it is my fault, I will send v3.

I only tested kallsyms_on_each_symbol and kallsyms_lookup_name (they
were of cause OK). , not test others.

ARRAY_SIZE is really valuable to help find bugs.

and next, after code changes, I should compile it again, at least.

:-)


> Cheers,


--
Chen Gang

Asianux Corporation

2013-04-15 04:48:39

by Chen Gang

[permalink] [raw]
Subject: [PATCH v3] kernel: kallsyms: memory override issue, need check destination buffer length


We don't export any symbols > 128 characters, but if we did then
kallsyms_expand_symbol() would overflow the buffer handed to it.
So we need check destination buffer length when copying.

the related test:
if we define an EXPORT function which name more than 128.
will panic when call kallsyms_lookup_name by init_kprobes on booting.
after check the length (provide this patch), it is ok.

Implementaion:
add additional destination buffer length parameter (maxlen)
if uncompressed string is too long (>= maxlen), it will be truncated.
not check the parameters whether valid, since it is a static function.


Signed-off-by: Chen Gang <[email protected]>
---
kernel/kallsyms.c | 26 ++++++++++++++++++--------
1 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 2169fee..3127ad5 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -84,9 +84,11 @@ static int is_ksym_addr(unsigned long addr)

/*
* Expand a compressed symbol data into the resulting uncompressed string,
+ * if uncompressed string is too long (>= maxlen), it will be truncated,
* given the offset to where the symbol is in the compressed stream.
*/
-static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
+static unsigned int kallsyms_expand_symbol(unsigned int off,
+ char *result, size_t maxlen)
{
int len, skipped_first = 0;
const u8 *tptr, *data;
@@ -113,15 +115,20 @@ static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)

while (*tptr) {
if (skipped_first) {
+ if (maxlen <= 1)
+ goto tail;
*result = *tptr;
result++;
+ maxlen--;
} else
skipped_first = 1;
tptr++;
}
}

- *result = '\0';
+tail:
+ if (maxlen)
+ *result = '\0';

/* Return to offset to the next symbol. */
return off;
@@ -176,7 +183,7 @@ unsigned long kallsyms_lookup_name(const char *name)
unsigned int off;

for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));

if (strcmp(namebuf, name) == 0)
return kallsyms_addresses[i];
@@ -195,7 +202,7 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
int ret;

for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
- off = kallsyms_expand_symbol(off, namebuf);
+ off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
ret = fn(data, namebuf, NULL, kallsyms_addresses[i]);
if (ret != 0)
return ret;
@@ -294,7 +301,8 @@ const char *kallsyms_lookup(unsigned long addr,

pos = get_symbol_pos(addr, symbolsize, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), namebuf);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ namebuf, KSYM_NAME_LEN);
if (modname)
*modname = NULL;
return namebuf;
@@ -315,7 +323,8 @@ int lookup_symbol_name(unsigned long addr, char *symname)

pos = get_symbol_pos(addr, NULL, NULL);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), symname);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ symname, KSYM_NAME_LEN);
return 0;
}
/* See if it's in a module. */
@@ -333,7 +342,8 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,

pos = get_symbol_pos(addr, size, offset);
/* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos), name);
+ kallsyms_expand_symbol(get_symbol_offset(pos),
+ name, KSYM_NAME_LEN);
modname[0] = '\0';
return 0;
}
@@ -463,7 +473,7 @@ static unsigned long get_ksymbol_core(struct kallsym_iter *iter)

iter->type = kallsyms_get_symbol_type(off);

- off = kallsyms_expand_symbol(off, iter->name);
+ off = kallsyms_expand_symbol(off, iter->name, ARRAY_SIZE(iter->name));

return off - iter->nameoff;
}
--
1.7.7.6

2013-04-15 23:17:24

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH v3] kernel: kallsyms: memory override issue, need check destination buffer length

Chen Gang <[email protected]> writes:

> We don't export any symbols > 128 characters, but if we did then
> kallsyms_expand_symbol() would overflow the buffer handed to it.
> So we need check destination buffer length when copying.
>
> the related test:
> if we define an EXPORT function which name more than 128.
> will panic when call kallsyms_lookup_name by init_kprobes on booting.
> after check the length (provide this patch), it is ok.
>
> Implementaion:
> add additional destination buffer length parameter (maxlen)
> if uncompressed string is too long (>= maxlen), it will be truncated.
> not check the parameters whether valid, since it is a static function.

Thanks, applied.

I've put this in my modules-next branch.

Cheers,
Rusty.

2013-04-16 01:19:50

by Chen Gang

[permalink] [raw]
Subject: Re: [PATCH v3] kernel: kallsyms: memory override issue, need check destination buffer length

On 2013??04??15?? 13:48, Rusty Russell wrote:
> Chen Gang <[email protected]> writes:
>
>> > We don't export any symbols > 128 characters, but if we did then
>> > kallsyms_expand_symbol() would overflow the buffer handed to it.
>> > So we need check destination buffer length when copying.
>> >
>> > the related test:
>> > if we define an EXPORT function which name more than 128.
>> > will panic when call kallsyms_lookup_name by init_kprobes on booting.
>> > after check the length (provide this patch), it is ok.
>> >
>> > Implementaion:
>> > add additional destination buffer length parameter (maxlen)
>> > if uncompressed string is too long (>= maxlen), it will be truncated.
>> > not check the parameters whether valid, since it is a static function.
> Thanks, applied.
>
> I've put this in my modules-next branch.
>

thank you very much to spend time resources for it.

and next, I should be careful to send patch. so do not wast other
members' time resources for my careless mistakes.

the details are:
a. if modify code, need always compile again, at least.
b. reduce reply times, so can reduce some of my waste information.

I will let additional machine (a PC) for test and compile instead of
my current work machine, so when I have to do another things (normally,
it is), the individual machine can continue compiling or testing.

thanks again.

:-)

> Cheers,
> Rusty.
>
>

--
Chen Gang

Asianux Corporation