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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
---
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;



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux