| Message ID | 20180522121018.15199-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/05/18 13:10, Chris Wilson wrote: > nospec quite reasonably asserts that it will never be used with an index > larger than unsigned long (that being the largest possibly index into an > C array). However, our ubi uses the convention of u64 for any large > integer, running afoul of the assertion on 32b. Reduce our index to an > unsigned long, checking for type overflow first. > > drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': > include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) > > Reported-by: kbuild-all@01.org > Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 95f9d179afc4..3f502eef2431 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -102,7 +102,7 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > for (i = 0; i < args->num_items; i++, user_item_ptr++) { > struct drm_i915_query_item item; > - u64 func_idx; > + unsigned long func_idx; > int ret; > > if (copy_from_user(&item, user_item_ptr, sizeof(item))) > @@ -111,6 +111,9 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) I guess you can get rid of this if (item.query_id == 0) then : > if (item.query_id == 0) > return -EINVAL; > > + if (overflows_type(item.query_id - 1, unsigned long)) > + return -EINVAL; > + > func_idx = item.query_id - 1; > > ret = -EINVAL;
Quoting Lionel Landwerlin (2018-05-22 13:13:03) > On 22/05/18 13:10, Chris Wilson wrote: > > nospec quite reasonably asserts that it will never be used with an index > > larger than unsigned long (that being the largest possibly index into an > > C array). However, our ubi uses the convention of u64 for any large > > integer, running afoul of the assertion on 32b. Reduce our index to an > > unsigned long, checking for type overflow first. > > > > drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': > > include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) > > > > Reported-by: kbuild-all@01.org > > Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_query.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > index 95f9d179afc4..3f502eef2431 100644 > > --- a/drivers/gpu/drm/i915/i915_query.c > > +++ b/drivers/gpu/drm/i915/i915_query.c > > @@ -102,7 +102,7 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > > > for (i = 0; i < args->num_items; i++, user_item_ptr++) { > > struct drm_i915_query_item item; > > - u64 func_idx; > > + unsigned long func_idx; > > int ret; > > > > if (copy_from_user(&item, user_item_ptr, sizeof(item))) > > @@ -111,6 +111,9 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > I guess you can get rid of this if (item.query_id == 0) then : Hmm, we could indeed. The choice is whether we want to make it clear that id=0 is illegal (making it easier to add debug later?) -Chris
Quoting Chris Wilson (2018-05-22 13:17:06) > Quoting Lionel Landwerlin (2018-05-22 13:13:03) > > On 22/05/18 13:10, Chris Wilson wrote: > > > nospec quite reasonably asserts that it will never be used with an index > > > larger than unsigned long (that being the largest possibly index into an > > > C array). However, our ubi uses the convention of u64 for any large > > > integer, running afoul of the assertion on 32b. Reduce our index to an > > > unsigned long, checking for type overflow first. > > > > > > drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': > > > include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) > > > > > > Reported-by: kbuild-all@01.org > > > Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_query.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > > index 95f9d179afc4..3f502eef2431 100644 > > > --- a/drivers/gpu/drm/i915/i915_query.c > > > +++ b/drivers/gpu/drm/i915/i915_query.c > > > @@ -102,7 +102,7 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > > > > > for (i = 0; i < args->num_items; i++, user_item_ptr++) { > > > struct drm_i915_query_item item; > > > - u64 func_idx; > > > + unsigned long func_idx; > > > int ret; > > > > > > if (copy_from_user(&item, user_item_ptr, sizeof(item))) > > > @@ -111,6 +111,9 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > I guess you can get rid of this if (item.query_id == 0) then : > > Hmm, we could indeed. The choice is whether we want to make it clear > that id=0 is illegal (making it easier to add debug later?) On second thoughts, I don't think so since u64==unsigned long on 64b, so (u64)-1 should fit inside unsigned long. -Chris
On 22/05/18 13:10, Chris Wilson wrote: > nospec quite reasonably asserts that it will never be used with an index > larger than unsigned long (that being the largest possibly index into an > C array). However, our ubi uses the convention of u64 for any large > integer, running afoul of the assertion on 32b. Reduce our index to an > unsigned long, checking for type overflow first. > > drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': > include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) > > Reported-by: kbuild-all@01.org > Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 95f9d179afc4..3f502eef2431 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -102,7 +102,7 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > for (i = 0; i < args->num_items; i++, user_item_ptr++) { > struct drm_i915_query_item item; > - u64 func_idx; > + unsigned long func_idx; > int ret; > > if (copy_from_user(&item, user_item_ptr, sizeof(item))) > @@ -111,6 +111,9 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > if (item.query_id == 0) > return -EINVAL; > > + if (overflows_type(item.query_id - 1, unsigned long)) > + return -EINVAL; > + > func_idx = item.query_id - 1; > > ret = -EINVAL;
Quoting Lionel Landwerlin (2018-05-22 13:22:32) > On 22/05/18 13:10, Chris Wilson wrote: > > nospec quite reasonably asserts that it will never be used with an index > > larger than unsigned long (that being the largest possibly index into an > > C array). However, our ubi uses the convention of u64 for any large > > integer, running afoul of the assertion on 32b. Reduce our index to an > > unsigned long, checking for type overflow first. > > > > drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': > > include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) > > > > Reported-by: kbuild-all@01.org > > Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> And pushed. Thanks for the review, and sorry for the palaver. -Chris
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 95f9d179afc4..3f502eef2431 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -102,7 +102,7 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) for (i = 0; i < args->num_items; i++, user_item_ptr++) { struct drm_i915_query_item item; - u64 func_idx; + unsigned long func_idx; int ret; if (copy_from_user(&item, user_item_ptr, sizeof(item))) @@ -111,6 +111,9 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) if (item.query_id == 0) return -EINVAL; + if (overflows_type(item.query_id - 1, unsigned long)) + return -EINVAL; + func_idx = item.query_id - 1; ret = -EINVAL;
nospec quite reasonably asserts that it will never be used with an index larger than unsigned long (that being the largest possibly index into an C array). However, our ubi uses the convention of u64 for any large integer, running afoul of the assertion on 32b. Reduce our index to an unsigned long, checking for type overflow first. drivers/gpu/drm/i915/i915_query.c: In function 'i915_query_ioctl': include/linux/compiler.h:339:38: error: call to '__compiletime_assert_119' declared with attribute error: BUILD_BUG_ON failed: sizeof(_s) > sizeof(long) Reported-by: kbuild-all@01.org Fixes: 84b510e22da7 ("drm/i915/query: Protect tainted function pointer lookup") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_query.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)