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;
