Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

[Rob Herring]

On Tue, Dec 21, 2021 at 5:34 AM Yin Xiujiang <yinxiujiang@kylinos.cn> wrote:
>
> In of_unittest_untrack_overlay if id is less than 0 then
> overlay_id_bits will be out of bounds. And it is also mentioned
> in bugzilla as a bug report:
> https://bugzilla.kernel.org/show_bug.cgi?id=214867
>
> Fix this bug by tiggering WARN_ON()

If id shouldn't be less than 0, can we make it unsigned instead as
discussed here[1].

Rob

[1] https://lore.kernel.org/all/c474a371-b524-1da8-4a67-e72cf8f2b0f7@gmail.com/

Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

[Frank Rowand]

On 12/21/21 4:28 AM, Yin Xiujiang wrote:
> In of_unittest_untrack_overlay if id is less than 0 then
> overlay_id_bits will be out of bounds. And it is also mentioned
> in bugzilla as a bug report:
> https://bugzilla.kernel.org/show_bug.cgi?id=214867
> 
> Fix this bug by tiggering WARN_ON()

This patch is just papering over the underlying problems.

The tracking of overlay ids in unittest.c depends on some
expectations of the expected range of values of id to be
tracked.  The resulting implementation is a bit fragile.
Let me take a look at whether I can create an alternate
implementation of id tracking.

-Frank

> 
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Signed-off-by: Yin Xiujiang <yinxiujiang@kylinos.cn>
> ---
>  drivers/of/unittest.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 5b85a2a3792a..094f9f4444d0 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1907,7 +1907,7 @@ static int overlay_first_id = -1;
>  
>  static long of_unittest_overlay_tracked(int id)
>  {
> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>  		return 0;
>  	return overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id);
>  }
> @@ -1918,7 +1918,7 @@ static void of_unittest_track_overlay(int id)
>  		overlay_first_id = id;
>  	id -= overlay_first_id;
>  
> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>  		return;
>  	overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);
>  }
> @@ -1928,7 +1928,7 @@ static void of_unittest_untrack_overlay(int id)
>  	if (overlay_first_id < 0)
>  		return;
>  	id -= overlay_first_id;
> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>  		return;
>  	overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
>  }
> 

Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

[Frank Rowand]

On 12/21/21 6:20 AM, Rob Herring wrote:
> On Tue, Dec 21, 2021 at 5:34 AM Yin Xiujiang <yinxiujiang@kylinos.cn> wrote:
>>
>> In of_unittest_untrack_overlay if id is less than 0 then
>> overlay_id_bits will be out of bounds. And it is also mentioned
>> in bugzilla as a bug report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=214867
>>
>> Fix this bug by tiggering WARN_ON()
> 
> If id shouldn't be less than 0, can we make it unsigned instead as
> discussed here[1].
> 
> Rob
> 
> [1] https://lore.kernel.org/all/c474a371-b524-1da8-4a67-e72cf8f2b0f7@gmail.com/
> 

There are problems with changing to unsigned (fixable, but more extensive
code changes).

I think that the implementation of id tracking in unittest.c is overly
fragile, and I would prefer to just re-implement it (see also my reply
to the proposed patch).

Let me take a look at whether I can create an alternate
implementation of id tracking.

-Frank

Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

[Frank Rowand]

On 12/22/21 1:06 PM, Frank Rowand wrote:
> On 12/21/21 4:28 AM, Yin Xiujiang wrote:
>> In of_unittest_untrack_overlay if id is less than 0 then
>> overlay_id_bits will be out of bounds. And it is also mentioned
>> in bugzilla as a bug report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=214867
>>
>> Fix this bug by tiggering WARN_ON()
> 
> This patch is just papering over the underlying problems.
> 
> The tracking of overlay ids in unittest.c depends on some
> expectations of the expected range of values of id to be
> tracked.  The resulting implementation is a bit fragile.
> Let me take a look at whether I can create an alternate
> implementation of id tracking.

I am going to totally replace the id tracking with a more
robust version that resolves several issues.  Patch should
be done on Monday or Tuesday.

-Frank

> 
> -Frank
> 
>>
>> Reported-by: Erhard F. <erhard_f@mailbox.org>
>> Signed-off-by: Yin Xiujiang <yinxiujiang@kylinos.cn>
>> ---
>>  drivers/of/unittest.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index 5b85a2a3792a..094f9f4444d0 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -1907,7 +1907,7 @@ static int overlay_first_id = -1;
>>  
>>  static long of_unittest_overlay_tracked(int id)
>>  {
>> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
>> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>>  		return 0;
>>  	return overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id);
>>  }
>> @@ -1918,7 +1918,7 @@ static void of_unittest_track_overlay(int id)
>>  		overlay_first_id = id;
>>  	id -= overlay_first_id;
>>  
>> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
>> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>>  		return;
>>  	overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);
>>  }
>> @@ -1928,7 +1928,7 @@ static void of_unittest_untrack_overlay(int id)
>>  	if (overlay_first_id < 0)
>>  		return;
>>  	id -= overlay_first_id;
>> -	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
>> +	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS || id < 0))
>>  		return;
>>  	overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
>>  }
>>
>