| Message ID | YxDSAj6tIrTZv5Y5@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: prevent integer overflow in query_engine_info() | expand |
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;
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;
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;
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(-)