RE: [PATCH] drm/i915/display: Prevent u64 underflow in intel_fbc_stolen_end

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 07 Jan 2026, "Cavitt, Jonathan" <jonathan.cavitt@intel.com> wrote:
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com> 
> Sent: Wednesday, January 7, 2026 3:39 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Gupta, Saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>; Zanoni, Paulo R <paulo.r.zanoni@intel.com>; ville.syrjala@linux.intel.com; daniel.vetter@ffwll.ch
> Subject: Re: [PATCH] drm/i915/display: Prevent u64 underflow in intel_fbc_stolen_end
>> 
>> On Fri, 19 Dec 2025, Jonathan Cavitt <jonathan.cavitt@intel.com> 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 <jonathan.cavitt@intel.com>
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > ---
>> >  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