[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

[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

[Intel-gfx] [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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: prevent integer overflow in query_engine_info()

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5027 bytes --]

== Series Details ==

Series: drm/i915: prevent integer overflow in query_engine_info()
URL   : https://patchwork.freedesktop.org/series/108038/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12061 -> Patchwork_108038v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_108038v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_108038v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108038v1/index.html

Participating hosts (35 -> 33)
------------------------------

  Additional (1): bat-dg2-8 
  Missing    (3): fi-rkl-11600 fi-bdw-samus fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_108038v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@mman:
    - fi-skl-6600u:       [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12061/fi-skl-6600u/igt@i915_selftest@live@mman.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108038v1/fi-skl-6600u/igt@i915_selftest@live@mman.html

  
Known issues
------------

  Here are the changes found in Patchwork_108038v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-kefka:       [FAIL][3] ([i915#6298]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12061/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108038v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  
#### Warnings ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-5:          [DMESG-FAIL][5] ([i915#4494] / [i915#4957]) -> [DMESG-FAIL][6] ([i915#4957])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12061/bat-dg1-5/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108038v1/bat-dg1-5/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6380]: https://gitlab.freedesktop.org/drm/intel/issues/6380
  [i915#6580]: https://gitlab.freedesktop.org/drm/intel/issues/6580
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621


Build changes
-------------

  * Linux: CI_DRM_12061 -> Patchwork_108038v1

  CI-20190529: 20190529
  CI_DRM_12061: d25f068998ce803ef0a05883616344c0afcbc55a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6639: ba61c48dba71d5597d7297a07dc3e307665f961b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_108038v1: d25f068998ce803ef0a05883616344c0afcbc55a @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

1f50086ab021 drm/i915: prevent integer overflow in query_engine_info()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108038v1/index.html

[-- Attachment #2: Type: text/html, Size: 4135 bytes --]

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: [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;

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;

Re: [Intel-gfx] [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;