linux-kernel - Re: [PATCH v2] drm/i915/backlight: Return immediately when scale() finds invalid parameters

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z5AmlJ8stVQ4L5jS@intel.com>
Date: Tue, 21 Jan 2025 17:58:28 -0500
From: Rodrigo Vivi <rodrigo.vivi@...el.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Jani Nikula <jani.nikula@...ux.intel.com>, Joonas Lahtinen
	<joonas.lahtinen@...ux.intel.com>, Tvrtko Ursulin <tursulin@...ulin.net>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	<intel-gfx@...ts.freedesktop.org>, <intel-xe@...ts.freedesktop.org>,
	<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>, "Linus
 Torvalds" <torvalds@...ux-foundation.org>, David Laight
	<david.laight.linux@...il.com>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v2] drm/i915/backlight: Return immediately when scale()
 finds invalid parameters

On Tue, Jan 21, 2025 at 06:52:03AM -0800, Guenter Roeck wrote:
> The scale() functions detects invalid parameters, but continues
> its calculations anyway. This causes bad results if negative values
> are used for unsigned operations. Worst case, a division by 0 error
> will be seen if source_min == source_max.
> 
> On top of that, after v6.13, the sequence of WARN_ON() followed by clamp()
> may result in a build error with gcc 13.x.
> 
> drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
> include/linux/compiler_types.h:542:45: error:
> 	call to '__compiletime_assert_415' declared with attribute error:
> 	clamp() low limit source_min greater than high limit source_max
> 
> This happens if the compiler decides to rearrange the code as follows.
> 
>         if (source_min > source_max) {
>                 WARN(..);
>                 /* Do the clamp() knowing that source_min > source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         } else {
>                 /* Do the clamp knowing that source_min <= source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         }
> 
> Fix the problem by evaluating the return values from WARN_ON and returning
> immediately after a warning. While at it, fix divide by zero error seen
> if source_min == source_max.
> 
> Analyzed-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Suggested-by: David Laight <david.laight.linux@...il.com>
> Cc: David Laight <david.laight.linux@...il.com>
> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@...el.com>

I'm pushing this soon to drm-intel-next, unless Linus want to take
this one directly to his tree

Thanks,
Rodrigo.

> ---
> v2: Simplify code to always return target_min after a warning,
>     and also warn if source_min == source_max.
> 
>  drivers/gpu/drm/i915/display/intel_backlight.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 3f81a726cc7d..ca588bed82b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -40,8 +40,9 @@ static u32 scale(u32 source_val,
>  {
>  	u64 target_val;
>  
> -	WARN_ON(source_min > source_max);
> -	WARN_ON(target_min > target_max);
> +	if (WARN_ON(source_min >= source_max) ||
> +	    WARN_ON(target_min > target_max))
> +		return target_min;
>  
>  	/* defensive */
>  	source_val = clamp(source_val, source_min, source_max);
> -- 
> 2.45.2
>