[PATCH] drm/i915: prevent integer overflow in query_engine_info()

[PATCH] drm/i915: prevent integer overflow in query_engine_info()

[Dan Carpenter]

This code uses struct_size() but it stores the result in an int so the
integer overflow checks are not effective.  Record the types as size_t
to prevent the size from being truncated.

Fixes: bf3c50837506 ("drm/i915/query: Use struct_size() helper")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I do not know if the integer overflow can happen.  This is a hardenning
patch just like the conversion to struct_size().

 drivers/gpu/drm/i915/i915_query.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 6ec9c9fb7b0d..43a499fbdc8d 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -13,7 +13,7 @@
 #include <uapi/drm/i915_drm.h>
 
 static int copy_query_item(void *query_hdr, size_t query_sz,
-			   u32 total_length,
+			   size_t total_length,
 			   struct drm_i915_query_item *query_item)
 {
 	if (query_item->length == 0)
@@ -135,7 +135,8 @@ query_engine_info(struct drm_i915_private *i915,
 	struct drm_i915_engine_info info = { };
 	unsigned int num_uabi_engines = 0;
 	struct intel_engine_cs *engine;
-	int len, ret;
+	size_t len;
+	int ret;
 
 	if (query_item->flags)
 		return -EINVAL;
-- 
2.35.1

Re: [Intel-gfx] [PATCH] drm/i915: prevent integer overflow in query_engine_info()

[Andrzej Hajda]

On 01.09.2022 17:38, Dan Carpenter wrote:
> This code uses struct_size() but it stores the result in an int so the
> integer overflow checks are not effective.  Record the types as size_t
> to prevent the size from being truncated.
> 
> Fixes: bf3c50837506 ("drm/i915/query: Use struct_size() helper")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej
> ---
> I do not know if the integer overflow can happen.  This is a hardenning
> patch just like the conversion to struct_size().
> 
>   drivers/gpu/drm/i915/i915_query.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 6ec9c9fb7b0d..43a499fbdc8d 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -13,7 +13,7 @@
>   #include <uapi/drm/i915_drm.h>
>   
>   static int copy_query_item(void *query_hdr, size_t query_sz,
> -			   u32 total_length,
> +			   size_t total_length,
>   			   struct drm_i915_query_item *query_item)
>   {
>   	if (query_item->length == 0)
> @@ -135,7 +135,8 @@ query_engine_info(struct drm_i915_private *i915,
>   	struct drm_i915_engine_info info = { };
>   	unsigned int num_uabi_engines = 0;
>   	struct intel_engine_cs *engine;
> -	int len, ret;
> +	size_t len;
> +	int ret;
>   
>   	if (query_item->flags)
>   		return -EINVAL;

Re: [PATCH] drm/i915: prevent integer overflow in query_engine_info()

[Tvrtko Ursulin]

Hi Dan,

On 01/09/2022 16:38, Dan Carpenter wrote:
> This code uses struct_size() but it stores the result in an int so the
> integer overflow checks are not effective.  Record the types as size_t
> to prevent the size from being truncated.
> 
> Fixes: bf3c50837506 ("drm/i915/query: Use struct_size() helper")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I do not know if the integer overflow can happen.  This is a hardenning
> patch just like the conversion to struct_size().

It can't since num_uabi_engines in "len = struct_size(query_ptr, 
engines, num_uabi_engines);" is max double digits and sizeof(struct 
drm_i915_engine_info) is 56 bytes on a glance.

Nevertheless hardening is almost always beneficial so no fundamental 
complaints. Just that this patch I think leaves some parts unresolved. 
Mostly that copy_query_item now can implicitly truncate in "return 
total_length" and likewise query_engine_info in "return ret;".

Maybe we could add, on top of your patch, something like:

  static int copy_query_item(void *query_hdr, size_t query_sz,
-                          u32 total_length,
+                          size_t total_length,
                            struct drm_i915_query_item *query_item)
  {
+       if (overflows_type(query_sz, query_item->length) ||
+           overflows_type(total_length, query_item->length))
+               return -ERANGE; /* ??? */
+

(query->item_length is s32 so matches the int return type.)

And change all variables holding result of copy_query_item to size_t.

Don't know, it could be it's an overkill. More opinions?

Regards,

Tvrtko

> 
>   drivers/gpu/drm/i915/i915_query.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 6ec9c9fb7b0d..43a499fbdc8d 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -13,7 +13,7 @@
>   #include <uapi/drm/i915_drm.h>
>   
>   static int copy_query_item(void *query_hdr, size_t query_sz,
> -			   u32 total_length,
> +			   size_t total_length,
>   			   struct drm_i915_query_item *query_item)
>   {
>   	if (query_item->length == 0)
> @@ -135,7 +135,8 @@ query_engine_info(struct drm_i915_private *i915,
>   	struct drm_i915_engine_info info = { };
>   	unsigned int num_uabi_engines = 0;
>   	struct intel_engine_cs *engine;
> -	int len, ret;
> +	size_t len;
> +	int ret;
>   
>   	if (query_item->flags)
>   		return -EINVAL;