| Message ID | 20250121145203.2851237-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/backlight: Return immediately when scale() finds invalid parameters | expand |
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@linux-foundation.org> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Suggested-by: David Laight <david.laight.linux@gmail.com> > Cc: David Laight <david.laight.linux@gmail.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.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 >
On Tue, 21 Jan 2025 at 14:59, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > > I'm pushing this soon to drm-intel-next, unless Linus want to take > this one directly to his tree Let's just go through the proper channels and go through the drm tree. Unless I've issed something, I think this is only an active issue on parisc (and only with certain compiler versions at that), so it isn't some super-urgent thing that needs to be expedited. Thanks, Linus
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);
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@linux-foundation.org> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Suggested-by: David Laight <david.laight.linux@gmail.com> Cc: David Laight <david.laight.linux@gmail.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- 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(-)