Re: [PATCH 6/8] drm/i915: Sanitize stolen memory size calculation

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 26, 2017 at 04:40:11PM +0300, Imre Deak wrote:
> On GEN8+ (not counting CHV) the calculation can in theory result in an
> incorrect sign extension with all upper bits set. In practice this is
> unlikely to happen since it would require 4GB of stolen memory set
> aside. For consistency still prevent the sign extension explicitly
> everywhere.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 13bf099..4b764b0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2577,14 +2577,14 @@ static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
>  {
>  	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
>  	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
> -	return snb_gmch_ctl << 25; /* 32 MB units */
> +	return (size_t)snb_gmch_ctl << 25; /* 32 MB units */

So the u16 gets promoted to int, which gets converted to size_t,
which may be larger than int, and thus things get sign extended.

Can't happen in the gen6 case actually due to SNB_GMCH_GMS_MASK being
small enough. But the gen8 case at least looks theoretically possible.
But having the case everywhere seems like the best way to avoid
someone copy-pasting the wrong thing when the next variant gets added.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl)
>  {
>  	bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT;
>  	bdw_gmch_ctl &= BDW_GMCH_GMS_MASK;
> -	return bdw_gmch_ctl << 25; /* 32 MB units */
> +	return (size_t)bdw_gmch_ctl << 25; /* 32 MB units */
>  }
>  
>  static size_t chv_get_stolen_size(u16 gmch_ctrl)
> @@ -2598,11 +2598,11 @@ static size_t chv_get_stolen_size(u16 gmch_ctrl)
>  	 * 0x17 to 0x1d: 4MB increments start at 36MB
>  	 */
>  	if (gmch_ctrl < 0x11)
> -		return gmch_ctrl << 25;
> +		return (size_t)gmch_ctrl << 25;
>  	else if (gmch_ctrl < 0x17)
> -		return (gmch_ctrl - 0x11 + 2) << 22;
> +		return (size_t)(gmch_ctrl - 0x11 + 2) << 22;
>  	else
> -		return (gmch_ctrl - 0x17 + 9) << 22;
> +		return (size_t)(gmch_ctrl - 0x17 + 9) << 22;
>  }
>  
>  static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
> @@ -2611,10 +2611,10 @@ static size_t gen9_get_stolen_size(u16 gen9_gmch_ctl)
>  	gen9_gmch_ctl &= BDW_GMCH_GMS_MASK;
>  
>  	if (gen9_gmch_ctl < 0xf0)
> -		return gen9_gmch_ctl << 25; /* 32 MB units */
> +		return (size_t)gen9_gmch_ctl << 25; /* 32 MB units */
>  	else
>  		/* 4MB increments starting at 0xf0 for 4MB */
> -		return (gen9_gmch_ctl - 0xf0 + 1) << 22;
> +		return (size_t)(gen9_gmch_ctl - 0xf0 + 1) << 22;
>  }
>  
>  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx