diff mbox series

[v2,4/7] drm/i915: Check for integer truncation on the configuration of ttm place

Message ID 20220705122455.3866745-5-gwan-gyeong.mun@intel.com (mailing list archive)
State New, archived
Headers show
Series Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation | expand

Commit Message

Gwan-gyeong Mun July 5, 2022, 12:24 p.m. UTC 
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@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@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(-)

Comments

Mauro Carvalho Chehab July 5, 2022, 2:44 p.m. UTC | #1 
On Tue,  5 Jul 2022 15:24:52 +0300
Gwan-gyeong Mun <gwan-gyeong.mun@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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Reviewed-by: Nirmoy Das <nirmoy.das@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)
diff mbox series

Patch

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
 }
 
 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; \
+	} \
+})
 	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
 
 	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)