On Wed, 07 Jan 2026, "Cavitt, Jonathan" <[email protected]> wrote: > -----Original Message----- > From: Jani Nikula <[email protected]> > Sent: Wednesday, January 7, 2026 3:39 AM > To: Cavitt, Jonathan <[email protected]>; > [email protected] > Cc: Gupta, Saurabhg <[email protected]>; Zuo, Alex > <[email protected]>; Cavitt, Jonathan <[email protected]>; Zanoni, > Paulo R <[email protected]>; [email protected]; > [email protected] > Subject: Re: [PATCH] drm/i915/display: Prevent u64 underflow in > intel_fbc_stolen_end >> >> On Fri, 19 Dec 2025, Jonathan Cavitt <[email protected]> wrote: >> > Static analysis reveals a potential integer underflow in >> > intel_fbc_stolen_end. This can apparently occur if >> > intel_parent_stolen_area_size returns zero (or, theoretically, any value >> > less than 2^23), as 2^23 is subtracted from the return value and stored >> > in a u64. While this doesn't appear to cause any issues due to the use >> > of the min() function to clamp the return values from the >> > intel_fbc_stolen_end function, it would be best practice to avoid >> > undeflowing values like this on principle. So, rework the function to >> > prevent the underflow from occurring. Note that the underflow at >> > present would result in the value of intel_fbc_cfb_base_max being >> > returned at the end of intel_fbc_stolen_end, so just return that if the >> > value of intel_parent_stolen_area_size is too small. >> > >> > While we're here, create a macro for the 2^23 value and modify the >> > execution path for readability. >> > >> > Fixes: a9da512b3ed7 ("drm/i915: avoid the last 8mb of stolen on BDW/SKL") >> > Signed-off-by: Jonathan Cavitt <[email protected]> >> > Cc: Paulo Zanoni <[email protected]> >> > Cc: Ville Syrjälä <[email protected]> >> > Cc: Daniel Vetter <[email protected]> >> > --- >> > drivers/gpu/drm/i915/display/intel_fbc.c | 20 ++++++++++++++------ >> > 1 file changed, 14 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c >> > b/drivers/gpu/drm/i915/display/intel_fbc.c >> > index fef2f35ff1e9..00c32df50933 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> > @@ -807,21 +807,29 @@ static u64 intel_fbc_cfb_base_max(struct >> > intel_display *display) >> > return BIT_ULL(32); >> > } >> > >> > +#define STOLEN_RESERVE_MAX SZ_8M >> > static u64 intel_fbc_stolen_end(struct intel_display *display) >> > { >> > - u64 end; >> > + u64 end = intel_fbc_cfb_base_max(display); >> > >> > /* The FBC hardware for BDW/SKL doesn't have access to the stolen >> > * reserved range size, so it always assumes the maximum (8mb) is used. >> > * If we enable FBC using a CFB on that memory range we'll get FIFO >> > * underruns, even if that range is not reserved by the BIOS. */ >> > if (display->platform.broadwell || >> > - (DISPLAY_VER(display) == 9 && !display->platform.broxton)) >> > - end = intel_parent_stolen_area_size(display) - 8 * 1024 * 1024; >> > - else >> > - end = U64_MAX; >> > + (DISPLAY_VER(display) == 9 && !display->platform.broxton)) { >> > + u64 stolen_area_size = intel_parent_stolen_area_size(display); >> > + >> > + /* If stolen_area_size is less than STOLEN_RESERVE_MAX, >> > + * use intel_fbc_cfb_base_max instead. */ >> >> Please use the proper multi-line comment style. > > Should I also fix the comment about FBC hardware while I'm here?
I wouldn't mind. > >> >> > + if (stolen_area_size < STOLEN_RESERVE_MAX) >> > + return end; >> >> check_sub_overflow(), perhaps with a drm_WARN_ON(), would be the way to >> go I think. You can get rid of the extra macro too. > > So, instead of STOLEN_RESERVE_MAX, just directly reference SZ_8M here? Yeah, since using check_sub_overflow() means you don't have to reference the value twice, so you don't need the macro for it. > -Jonathan Cavitt > >> >> > + >> > + stolen_area_size -= STOLEN_RESERVE_MAX; >> >> A blank line is preferred before return. >> >> > + return min(end, stolen_area_size); >> > + } >> > >> > - return min(end, intel_fbc_cfb_base_max(display)); >> > + return end; >> > } >> > >> > static int intel_fbc_min_limit(const struct intel_plane_state >> > *plane_state) >> >> -- >> Jani Nikula, Intel >> -- Jani Nikula, Intel
