lkml.org 
[lkml]   [2022]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline
From
Date
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

\
 
 \ /
  Last update: 2022-01-18 03:18    [W:0.049 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site