Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

[Greg Ungerer]

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

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.


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

Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

[Hangyu Hua]

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

Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

[Hangyu Hua]

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

Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

[Greg Ungerer]

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.

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.

Regards
Greg

[PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline

[Hangyu Hua]

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;
 }
-- 
2.25.1