[Intel-gfx] [PATCH v2 4/7] drm/i915: Check for integer truncation on the configuration of ttm place

[Mauro Carvalho Chehab]

On Tue,  5 Jul 2022 15:24:52 +0300
Gwan-gyeong Mun <gwan-gyeong.mun at intel.com> wrote:

> There is an impedance mismatch between the first/last valid page
> frame number of ttm place in unsigned and our memory/page accounting in
> unsigned long.
> As the object size is under the control of userspace, we have to be prudent
> and catch the conversion errors.
> To catch the implicit truncation as we switch from unsigned long to
> unsigned, we use overflows_type check and report E2BIG or overflow_type
> prior to the operation.
> 
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 12 +++++++++---
>  drivers/gpu/drm/i915/intel_region_ttm.c | 16 +++++++++++++---
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index cdcb3ee0c433..d579524663b3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -137,19 +137,25 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
>  	if (mr->type == INTEL_MEMORY_SYSTEM)
>  		return;
>  
> +#define SAFE_CONVERSION(ptr, value) ({ \
> +	if (!safe_conversion(ptr, value)) { \
> +		GEM_BUG_ON(overflows_type(value, *ptr)); \
> +	} \
> +})
>  	if (flags & I915_BO_ALLOC_CONTIGUOUS)
>  		place->flags |= TTM_PL_FLAG_CONTIGUOUS;
>  	if (offset != I915_BO_INVALID_OFFSET) {
> -		place->fpfn = offset >> PAGE_SHIFT;
> -		place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
> +		SAFE_CONVERSION(&place->fpfn, offset >> PAGE_SHIFT);
> +		SAFE_CONVERSION(&place->lpfn, place->fpfn + (size >> PAGE_SHIFT));
>  	} else if (mr->io_size && mr->io_size < mr->total) {
>  		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>  			place->flags |= TTM_PL_FLAG_TOPDOWN;
>  		} else {
>  			place->fpfn = 0;
> -			place->lpfn = mr->io_size >> PAGE_SHIFT;
> +			SAFE_CONVERSION(&place->lpfn, mr->io_size >> PAGE_SHIFT);
>  		}
>  	}

> +#undef SAFE_CONVERSION
Why?

>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> index 62ff77445b01..8fcb8654b978 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> @@ -202,24 +202,34 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>  	struct ttm_resource *res;
>  	int ret;
>  
> +#define SAFE_CONVERSION(ptr, value) ({ \
> +	if (!safe_conversion(ptr, value)) { \
> +		GEM_BUG_ON(overflows_type(value, *ptr)); \
> +		ret = -E2BIG; \
> +		goto out; \
> +	} \
> +})

It is a bad practice to change execution inside a macro.
See "12) Macros, Enums and RTL" at Documentation/process/coding-style.rst.

>  	if (flags & I915_BO_ALLOC_CONTIGUOUS)
>  		place.flags |= TTM_PL_FLAG_CONTIGUOUS;
>  	if (offset != I915_BO_INVALID_OFFSET) {
> -		place.fpfn = offset >> PAGE_SHIFT;
> -		place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
> +		SAFE_CONVERSION(&place.fpfn, offset >> PAGE_SHIFT);
> +		SAFE_CONVERSION(&place.lpfn, place.fpfn + (size >> PAGE_SHIFT));
>  	} else if (mem->io_size && mem->io_size < mem->total) {
>  		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>  			place.flags |= TTM_PL_FLAG_TOPDOWN;
>  		} else {
>  			place.fpfn = 0;
> -			place.lpfn = mem->io_size >> PAGE_SHIFT;
> +			SAFE_CONVERSION(&place.lpfn, mem->io_size >> PAGE_SHIFT);
>  		}
>  	}

> +#undef SAFE_CONVERSION
Why?

>  
>  	mock_bo.base.size = size;
>  	mock_bo.bdev = &mem->i915->bdev;
>  
>  	ret = man->func->alloc(man, &mock_bo, &place, &res);
> +
> +out:
>  	if (ret == -ENOSPC)
>  		ret = -ENXIO;
>  	if (!ret)