Subject: Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0
From:   Frank Rowand <frowand.list@gmail.com>
To:     Yin Xiujiang <yinxiujiang@kylinos.cn>, robh+dt@kernel.org
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211221092830.680839-1-yinxiujiang@kylinos.cn>
 <96165b43-4be2-644c-dfc9-42173fcfc775@gmail.com>
Message-ID: <df6e9b2b-073d-a8a6-daef-92da2fb0eda9@gmail.com>
Date:   Fri, 24 Dec 2021 10:18:27 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <96165b43-4be2-644c-dfc9-42173fcfc775@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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