[PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[Cyrill Gorcunov]

Use KSYM_NAME_LEN instead of numeric value.
Actually because of too small 'tmp' there is
a potential buffer overflow.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/xmon/xmon.c	2008-01-23 19:04:42.000000000 +0300
+++ linux-2.6.git/arch/powerpc/xmon/xmon.c	2008-01-23 19:12:45.000000000 +0300
@@ -69,7 +69,7 @@ static unsigned long ndump = 64;
 static unsigned long nidump = 16;
 static unsigned long ncsum = 4096;
 static int termch;
-static char tmpstr[128];
+static char tmpstr[KSYM_NAME_LEN];
 
 #define JMP_BUF_LEN	23
 static long bus_error_jmp[JMP_BUF_LEN];
@@ -2354,7 +2354,7 @@ scanhex(unsigned long *vp)
 		}
 	} else if (c == '$') {
 		int i;
-		for (i=0; i<63; i++) {
+		for (i = 0; i < sizeof(tmpstr) / 2; i++) {
 			c = inchar();
 			if (isspace(c)) {
 				termch = c;
@@ -2467,7 +2467,7 @@ symbol_lookup(void)
 {
 	int type = inchar();
 	unsigned long addr;
-	static char tmp[64];
+	static char tmp[KSYM_NAME_LEN];
 
 	switch (type) {
 	case 'a':
@@ -2476,7 +2476,7 @@ symbol_lookup(void)
 		termch = 0;
 		break;
 	case 's':
-		getstring(tmp, 64);
+		getstring(tmp, sizeof(tmp));
 		if (setjmp(bus_error_jmp) == 0) {
 			catch_memory_errors = 1;
 			sync();

Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[Paulo Marques]

Cyrill Gorcunov wrote:
> Use KSYM_NAME_LEN instead of numeric value.

The patch series looks like a nice cleanup, except for a few things in 
this patch.

> Actually because of too small 'tmp' there is
> a potential buffer overflow.

I don't think there is. "tmp" is not being passed to kallsyms to be 
filled with a symbol name, but it's being used to hold a name written by 
the user to lookup an address.

If the powerpc/xmon people feel that 63 characters is enough to hold a 
symbol name, it's their problem, but there is no buffer overflow.

> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 
> Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
> ===================================================================
> --- linux-2.6.git.orig/arch/powerpc/xmon/xmon.c	2008-01-23 19:04:42.000000000 +0300
> +++ linux-2.6.git/arch/powerpc/xmon/xmon.c	2008-01-23 19:12:45.000000000 +0300
> @@ -69,7 +69,7 @@ static unsigned long ndump = 64;
>  static unsigned long nidump = 16;
>  static unsigned long ncsum = 4096;
>  static int termch;
> -static char tmpstr[128];
> +static char tmpstr[KSYM_NAME_LEN];

This one seems ok, since "tmpstr" is used everywhere to hold symbol names.

>  #define JMP_BUF_LEN	23
>  static long bus_error_jmp[JMP_BUF_LEN];
> @@ -2354,7 +2354,7 @@ scanhex(unsigned long *vp)
>  		}
>  	} else if (c == '$') {
>  		int i;
> -		for (i=0; i<63; i++) {
> +		for (i = 0; i < sizeof(tmpstr) / 2; i++) {

This one is completely out of the blue. Why "sizeof(tmpstr) / 2"?

It would make more sense to use "sizeof(tmpstr) - 1", but either way it 
is a change in behavior.

>  			c = inchar();
>  			if (isspace(c)) {
>  				termch = c;
> @@ -2467,7 +2467,7 @@ symbol_lookup(void)
>  {
>  	int type = inchar();
>  	unsigned long addr;
> -	static char tmp[64];
> +	static char tmp[KSYM_NAME_LEN];
>  
>  	switch (type) {
>  	case 'a':
> @@ -2476,7 +2476,7 @@ symbol_lookup(void)
>  		termch = 0;
>  		break;
>  	case 's':
> -		getstring(tmp, 64);
> +		getstring(tmp, sizeof(tmp));
>  		if (setjmp(bus_error_jmp) == 0) {
>  			catch_memory_errors = 1;
>  			sync();

This also introduces a change in behavior. It is still a nice cleanup, 
though. So, if the powerpc people feel they can spare an extra 64 bytes 
of stack here, I guess it's ok.

-- 
Paulo Marques - www.grupopie.com

"As far as we know, our computer has never had an undetected error."
Weisert

Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[Cyrill Gorcunov]

[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]
> Cyrill Gorcunov wrote:
>> Use KSYM_NAME_LEN instead of numeric value.
>
> The patch series looks like a nice cleanup, except for a few things in this 
> patch.
>
>> Actually because of too small 'tmp' there is
>> a potential buffer overflow.
>
> I don't think there is. "tmp" is not being passed to kallsyms to be filled 
> with a symbol name, but it's being used to hold a name written by the user 
> to lookup an address.
>
> If the powerpc/xmon people feel that 63 characters is enough to hold a 
> symbol name, it's their problem, but there is no buffer overflow.
>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> ---
>> Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
>> ===================================================================
>> --- linux-2.6.git.orig/arch/powerpc/xmon/xmon.c	2008-01-23 
>> 19:04:42.000000000 +0300
>> +++ linux-2.6.git/arch/powerpc/xmon/xmon.c	2008-01-23 19:12:45.000000000 
>> +0300
>> @@ -69,7 +69,7 @@ static unsigned long ndump = 64;
>>  static unsigned long nidump = 16;
>>  static unsigned long ncsum = 4096;
>>  static int termch;
>> -static char tmpstr[128];
>> +static char tmpstr[KSYM_NAME_LEN];
>
> This one seems ok, since "tmpstr" is used everywhere to hold symbol names.
>
>>  #define JMP_BUF_LEN	23
>>  static long bus_error_jmp[JMP_BUF_LEN];
>> @@ -2354,7 +2354,7 @@ scanhex(unsigned long *vp)
>>  		}
>>  	} else if (c == '$') {
>>  		int i;
>> -		for (i=0; i<63; i++) {
>> +		for (i = 0; i < sizeof(tmpstr) / 2; i++) {
>
> This one is completely out of the blue. Why "sizeof(tmpstr) / 2"?
>

the original code was 63 but 63 is 128/2-1 so to not change the
original idea of 'use a half size in this case' I made it like
that.

> It would make more sense to use "sizeof(tmpstr) - 1", but either way it is 
> a change in behavior.
>
>>  			c = inchar();
>>  			if (isspace(c)) {
>>  				termch = c;
>> @@ -2467,7 +2467,7 @@ symbol_lookup(void)
>>  {
>>  	int type = inchar();
>>  	unsigned long addr;
>> -	static char tmp[64];
>> +	static char tmp[KSYM_NAME_LEN];
>>   	switch (type) {
>>  	case 'a':
>> @@ -2476,7 +2476,7 @@ symbol_lookup(void)
>>  		termch = 0;
>>  		break;
>>  	case 's':
>> -		getstring(tmp, 64);
>> +		getstring(tmp, sizeof(tmp));
>>  		if (setjmp(bus_error_jmp) == 0) {
>>  			catch_memory_errors = 1;
>>  			sync();

just after that poin in the original code a call to kallsyms_lookup_name
is done - so i think it could be an overflow (of course it depends
on what *exactly* the name is being searched, and Paulo - I didn't
managed to get *the whole picture* of what is going on in this
code - so the thoughs were like: kallsyms_lookup_name could find
a quite long name restricted by KSYM_NAME_LEN (dunno how it could
happens - due to buggy code or due to memory corruption outside,
it does not matter - the only matter - it *could* find that long
name).

Anyway - it's just an attempt ;) we always could drop it far-far away ;)

>
> This also introduces a change in behavior. It is still a nice cleanup, 
> though. So, if the powerpc people feel they can spare an extra 64 bytes of 
> stack here, I guess it's ok.

Thanks a lot for review Paulo!

>
> -- 
> Paulo Marques - www.grupopie.com
>
> "As far as we know, our computer has never had an undetected error."
> Weisert
>
		- Cyrill -

Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[Paulo Marques]

Cyrill Gorcunov wrote:
> [Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]
>> Cyrill Gorcunov wrote:
>>> [...]
>>>  	case 's':
>>> -		getstring(tmp, 64);
>>> +		getstring(tmp, sizeof(tmp));
>>>  		if (setjmp(bus_error_jmp) == 0) {
>>>  			catch_memory_errors = 1;
>>>  			sync();
> 
> just after that poin in the original code a call to kallsyms_lookup_name
> is done - so i think it could be an overflow (of course it depends
> on what *exactly* the name is being searched, and Paulo - I didn't
> managed to get *the whole picture* of what is going on in this
> code - so the thoughs were like: kallsyms_lookup_name could find
> a quite long name restricted by KSYM_NAME_LEN (dunno how it could
> happens - due to buggy code or due to memory corruption outside,
> it does not matter - the only matter - it *could* find that long
> name).

Ah, now I understand your confusion: kallsyms_lookup_name doesn't fill 
the name. It searches the name and returns the address. It is the 
_caller_ that fills the name, not kallsyms_lookup_name.

It is used for stuff like: "give me the address of function foo":
addr = kallsyms_lookup_name("foo");

> Anyway - it's just an attempt ;) we always could drop it far-far away ;)

I think that using KSYM_NAME_LEN would be a nice cleanup for xmon, but 
it is for the powerpc guys to decide if they want to do it. I just 
wanted to point the change in behavior so that it wouldn't go unnoticed.

For all we know, the stack may at that point be close to full and an 
extra 64 bytes may tip it over the edge.

>> This also introduces a change in behavior. It is still a nice cleanup, 
>> though. So, if the powerpc people feel they can spare an extra 64 bytes of 
>> stack here, I guess it's ok.
> 
> Thanks a lot for review Paulo!

No problem. I always keep an eye out for kallsyms related stuff.

-- 
Paulo Marques - www.grupopie.com

"There cannot be a crisis today; my schedule is already full."

Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN

[Cyrill Gorcunov]

[Paulo Marques - Wed, Jan 23, 2008 at 06:59:09PM +0000]
> Cyrill Gorcunov wrote:
>> [Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]
>>> Cyrill Gorcunov wrote:
>>>> [...]
>>>>  	case 's':
>>>> -		getstring(tmp, 64);
>>>> +		getstring(tmp, sizeof(tmp));
>>>>  		if (setjmp(bus_error_jmp) == 0) {
>>>>  			catch_memory_errors = 1;
>>>>  			sync();
>> just after that poin in the original code a call to kallsyms_lookup_name
>> is done - so i think it could be an overflow (of course it depends
>> on what *exactly* the name is being searched, and Paulo - I didn't
>> managed to get *the whole picture* of what is going on in this
>> code - so the thoughs were like: kallsyms_lookup_name could find
>> a quite long name restricted by KSYM_NAME_LEN (dunno how it could
>> happens - due to buggy code or due to memory corruption outside,
>> it does not matter - the only matter - it *could* find that long
>> name).
>
> Ah, now I understand your confusion: kallsyms_lookup_name doesn't fill the 
> name. It searches the name and returns the address. It is the _caller_ that 
> fills the name, not kallsyms_lookup_name.
>
> It is used for stuff like: "give me the address of function foo":
> addr = kallsyms_lookup_name("foo");
>

oh my, how could I oversight that... damn!!! my bad!!!

>> Anyway - it's just an attempt ;) we always could drop it far-far away ;)
>
> I think that using KSYM_NAME_LEN would be a nice cleanup for xmon, but it 
> is for the powerpc guys to decide if they want to do it. I just wanted to 
> point the change in behavior so that it wouldn't go unnoticed.

thanks, it's really important

>
> For all we know, the stack may at that point be close to full and an extra 
> 64 bytes may tip it over the edge.
>

oops, thanks for pointing out

>>> This also introduces a change in behavior. It is still a nice cleanup, 
>>> though. So, if the powerpc people feel they can spare an extra 64 bytes 
>>> of stack here, I guess it's ok.
>> Thanks a lot for review Paulo!
>
> No problem. I always keep an eye out for kallsyms related stuff.

;)

>
> -- 
> Paulo Marques - www.grupopie.com
>
> "There cannot be a crisis today; my schedule is already full."
>
		- Cyrill -