Messages in this thread Patch in this message | | | Date | Wed, 23 Jan 2008 21:43:43 +0300 | From | Cyrill Gorcunov <> | Subject | Re: [PATCH 1/6] POWERPC: use KSYM_NAME_LEN |
| |
[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 -
| |