POWERPC: use KSYM_NAME_LEN

PATCH Applied Cyrill Gorcunov 2 participants 5 messages Jan 23, 2008 PowerPC

lore

Share & tools

lore

Cyrill Gorcunov

author

2008-01-23 17:40 UTC #0 lore raw

PATCH 1/6 POWERPC: use KSYM_NAME_LEN

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 <[email protected]>

---

arch/powerpc/xmon/xmon.c +4 -4

Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
===================================================================
@@ -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();

Paulo Marques

2008-01-23 18:26 UTC #1 lore raw

Quoted 1 line · Cyrill Gorcunov

Use KSYM_NAME_LEN instead of numeric value.

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

Quoted 2 lines

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.

arch/powerpc/xmon/xmon.c static unsigned long ndump = 64; · line 72

Quoted 13 lines

Signed-off-by Cyrill Gorcunov <[email protected]>
---
Index: linux-2.6.git/arch/powerpc/xmon/xmon.c
===================================================================
arch/powerpc/xmon/xmon.c +1 -1
@@ -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.

arch/powerpc/xmon/xmon.c scanhex(unsigned long *vp) · line 2357

Quoted 8 lines

arch/powerpc/xmon/xmon.c +1 -1

 #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.

Quoted 13 lines

arch/powerpc/xmon/xmon.c +1 -1

 			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':

arch/powerpc/xmon/xmon.c symbol_lookup(void) · line 2482

Quoted 8 lines

arch/powerpc/xmon/xmon.c +1 -1

@@ -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.

Signature · 4 lines

Paulo Marques - www.grupopie.com

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

Cyrill Gorcunov

author

2008-01-23 18:44 UTC #2 lore raw

[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]

Quoted 33 lines

Quoted 1 line · Cyrill Gorcunov
Use KSYM_NAME_LEN instead of numeric value.
The patch series looks like a nice cleanup, except for a few things in this patch.
Quoted 2 lines
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.
Quoted 9 lines
Signed-off-by Cyrill Gorcunov <[email protected]>
---
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
arch/powerpc/xmon/xmon.c static unsigned long ndump = 64; · line 72
Quoted 5 lines
arch/powerpc/xmon/xmon.c +1 -1
@@ -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.

Quoted 11 lines

arch/powerpc/xmon/xmon.c scanhex(unsigned long *vp) · line 2357
Quoted 8 lines
arch/powerpc/xmon/xmon.c +1 -1
 #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.

Quoted 15 lines

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

Quoted 12 lines

arch/powerpc/xmon/xmon.c +1 -1

 			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)

Quoted 8 lines

arch/powerpc/xmon/xmon.c symbol_lookup(void) · line 2482

Quoted 8 lines

arch/powerpc/xmon/xmon.c +1 -1

 		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 ;)

Quoted 4 lines

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!

Quoted 7 lines

Signature · 5 lines

-- 
Paulo Marques - www.grupopie.com

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

- Cyrill -

Paulo Marques

2008-01-23 18:59 UTC #3 lore raw

Quoted 11 lines · Cyrill Gorcunov

[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]
Quoted 8 lines
arch/powerpc/xmon/xmon.c symbol_lookup(void) · line 2482
Quoted 7 lines · Cyrill Gorcunov
[...]
arch/powerpc/xmon/xmon.c +1 -1
 	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

Quoted 8 lines

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");

Quoted 1 line

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.

Quoted 3 lines

Quoted 1 line
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.

Signature · 3 lines

Paulo Marques - www.grupopie.com

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

Cyrill Gorcunov

author

2008-01-23 19:08 UTC #4 lore raw

[Paulo Marques - Wed, Jan 23, 2008 at 06:59:09PM +0000]

Quoted 20 lines

Quoted 18 lines · Cyrill Gorcunov
[Paulo Marques - Wed, Jan 23, 2008 at 06:26:28PM +0000]
Quoted 8 lines
arch/powerpc/xmon/xmon.c symbol_lookup(void) · line 2482
Quoted 7 lines · Cyrill Gorcunov
[...]
arch/powerpc/xmon/xmon.c +1 -1
 	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).

Quoted 7 lines

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!!!

Quoted 5 lines

Quoted 1 line
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

Quoted 4 lines

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

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
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.

;)

Quoted 6 lines

Signature · 4 lines

-- 
Paulo Marques - www.grupopie.com

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

- Cyrill -