Messages in this thread | | | Subject | Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline | From | Hangyu Hua <> | Date | Tue, 18 Jan 2022 10:18:16 +0800 |
| |
Hi Greg,
On 2022/1/17 下午12:03, Greg Ungerer wrote: > Hi Hangyu, > > On 13/1/22 11:58 am, Hangyu Hua wrote: >> When the size of commandp >= size, array out of bound write occurs >> because >> len == 0. >> >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> arch/m68k/kernel/uboot.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/m68k/kernel/uboot.c b/arch/m68k/kernel/uboot.c >> index 928dbd33fc4a..63eaf3c3ddcd 100644 >> --- a/arch/m68k/kernel/uboot.c >> +++ b/arch/m68k/kernel/uboot.c >> @@ -101,5 +101,6 @@ __init void process_uboot_commandline(char >> *commandp, int size) >> } >> parse_uboot_commandline(commandp, len); >> - commandp[len - 1] = 0; >> + if (len > 0) >> + commandp[len - 1] = 0; >> } >> > > I am not convinced this is wrong for the reason you think it is. > Looking at the code in its entirety: > > __init void process_uboot_commandline(char *commandp, int size) > { > int len, n; > > n = strnlen(commandp, size); > commandp += n; > len = size - n; > if (len) { > /* Add the whitespace separator */ > *commandp++ = ' '; > len--; > } > > parse_uboot_commandline(commandp, len); > commandp[len - 1] = 0; > } > > > "commandp" is moved based on the return of the strnlen(). So in the > case of commandp actually being full of valid characters (so n == size, > and thus len == 0) the commandp technically points outside of its > real size at that point. But "command[[len - 1]" would actually be > pointing to the last char in the original commandp array (so the original > commandp[size - 1]). Well at least if you are happy with the use of > negative array indexes. >
You mean this is a friendly out of bound beacause "command[[len - 1]" pointing to the last char in the original commandp array. I used to think command[[len - 1] = 0 may be a zero-terminated for command. You can see my discussion with Andreas Schwab and my patch v1 in
https://lore.kernel.org/all/CAOo-nLJG71QqqD0-cJDyH0rY2VTx1eO9nHVQ5MCe8J0iiME_vw@mail.gmail.com/
But this still be a out of bound write because "commandp" is a macro definition with a fixed size.
> Clearly this could be structured better. There is no point in calling > parse_uboot_commandline() if len == 0, or even if len == 1, since you > cannot add anymore to the command line, it is full. > I think it is no point too. But the caller (setup_arch()) don't check the size of "commandp" before call parse_uboot_commandline(). Instead we do this in parse_uboot_commandline(). So it may be better to move these checks to the caller ?
> Regards > Greg
Thanks for your reply Hangyu Hua
| |