[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c9238f43-406f-3a75-c469-7d8ad30f88f3@gmail.com>
Date: Tue, 18 Jan 2022 18:40:20 +0800
From: Hangyu Hua <hbh25y@...il.com>
To: Greg Ungerer <gerg@...ux-m68k.org>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>,
linux-m68k <linux-m68k@...ts.linux-m68k.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in
process_uboot_commandline
Hi, Greg
On 2022/1/18 下午4:26, Greg Ungerer wrote:
> Hi Hangyu,
>
> On 18/1/22 12:18 pm, Hangyu Hua wrote:
>> 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@...il.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.
>
> No, "commandp" is not a macro, it is a parameter to this function, is a
> char pointer.
> It points into a char array of size "size" (which will be non-zero).
> It is modified during execution of this function.
> I don't see an out-of-bound write here.
>
I am sorry i make a mistake in here. What i want to express is that
setup_arch call parse_uboot_commandline with m68k_command_line or
command_line.The definitions of m68k_command_line and command_line
are:
char __initdata command_line[COMMAND_LINE_SIZE];
static char m68k_command_line[CL_SIZE] __initdata;
And I undertand what you mean. You are right. There isn't a out-of-bound.
>
>>> 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 ?
>
> No, I don't think so. The caller doesn't care if it is already full.
> And the common case is that process_uboot_commandline() is empty
> when CONFIG_UBOOT is not enabled.
>
> Regards
> Greg
>
>
Thanks
Powered by blists - more mailing lists