Subject: Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0
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>
From:   Frank Rowand <frowand.list@gmail.com>
Message-ID: <96165b43-4be2-644c-dfc9-42173fcfc775@gmail.com>
Date:   Wed, 22 Dec 2021 12:06:33 -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: <20211221092830.680839-1-yinxiujiang@kylinos.cn>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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