linux-kernel - Re: [PATCH] of: unittest: Fix out of bounds array access when id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <96165b43-4be2-644c-dfc9-42173fcfc775@gmail.com>
Date:   Wed, 22 Dec 2021 12:06:33 -0600
From:   Frank Rowand <frowand.list@...il.com>
To:     Yin Xiujiang <yinxiujiang@...inos.cn>, robh+dt@...nel.org
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of: unittest: Fix out of bounds array access when id < 0

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@...lbox.org>
> Signed-off-by: Yin Xiujiang <yinxiujiang@...inos.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);
>  }
>