| Message ID | 20190109075108.16779-1-abdiel.janulgue@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/query: Split out query item checks | expand |
On 09/01/2019 07:51, Abdiel Janulgue wrote: > This simplifies adding new query item objects. > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 40 ++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index cbcb957b7141..b4f26605f617 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -10,12 +10,33 @@ > #include "i915_query.h" > #include <uapi/drm/i915_drm.h> > > +static int init_query_item_check(void* data_ptr, size_t data_sz, void *ptr data_ prefix is not ideal since this is not the trailing data but the query header. Maybe query_ ? > + u32 total_length, > + struct drm_i915_query_item *query_item) > +{ > + if (query_item->length == 0) > + return total_length; > + > + if (query_item->length < total_length) > + return -EINVAL; > + > + if (copy_from_user(data_ptr, u64_to_user_ptr(query_item->data_ptr), > + data_sz)) > + return -EFAULT; Is lost type information a concern with copy_from_user.. let me check.. I am not sure TBH.. there seems to be type based object size check in there but does it work when indirected via void*? > + > + if (!access_ok(u64_to_user_ptr(query_item->data_ptr), > + total_length)) > + return -EFAULT; Kind of OK, but prevents the code for checking the whole user supplied buffer in queries which carry the length field. (Engine discovery is one of them.) Argument there was why check more than we will write - to catch userspace accidentally passing in a smaller buffer than it thinks it is passing in. But it is probably not an important enough concern to prevent code consolidation. So I think for me that leaves the question of type safety on the first check. > + > + return 0; > +} > + > static int query_topology_info(struct drm_i915_private *dev_priv, > struct drm_i915_query_item *query_item) > { > const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; > struct drm_i915_query_topology_info topo; > - u32 slice_length, subslice_length, eu_length, total_length; > + u32 ret, slice_length, subslice_length, eu_length, total_length; int ret; > > if (query_item->flags != 0) > return -EINVAL; > @@ -33,23 +54,14 @@ static int query_topology_info(struct drm_i915_private *dev_priv, > > total_length = sizeof(topo) + slice_length + subslice_length + eu_length; > > - if (query_item->length == 0) > - return total_length; > - > - if (query_item->length < total_length) > - return -EINVAL; > - > - if (copy_from_user(&topo, u64_to_user_ptr(query_item->data_ptr), > - sizeof(topo))) > - return -EFAULT; > + ret = init_query_item_check(&topo, sizeof(topo), total_length, > + query_item); > + if (ret != 0) > + return ret; > > if (topo.flags != 0) > return -EINVAL; > > - if (!access_ok(u64_to_user_ptr(query_item->data_ptr), > - total_length)) > - return -EFAULT; > - > memset(&topo, 0, sizeof(topo)); > topo.max_slices = sseu->max_slices; > topo.max_subslices = sseu->max_subslices; > Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-01-09 09:25:49) > > On 09/01/2019 07:51, Abdiel Janulgue wrote: > > This simplifies adding new query item objects. > > > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_query.c | 40 ++++++++++++++++++++----------- > > 1 file changed, 26 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > > index cbcb957b7141..b4f26605f617 100644 > > --- a/drivers/gpu/drm/i915/i915_query.c > > +++ b/drivers/gpu/drm/i915/i915_query.c > > @@ -10,12 +10,33 @@ > > #include "i915_query.h" > > #include <uapi/drm/i915_drm.h> > > > > +static int init_query_item_check(void* data_ptr, size_t data_sz, > > void *ptr > > data_ prefix is not ideal since this is not the trailing data but the > query header. Maybe query_ ? Maybe query_hdr, query_pkt? Or hdr, pkt? > > + u32 total_length, > > + struct drm_i915_query_item *query_item) > > +{ > > + if (query_item->length == 0) > > + return total_length; > > + > > + if (query_item->length < total_length) > > + return -EINVAL; > > + > > + if (copy_from_user(data_ptr, u64_to_user_ptr(query_item->data_ptr), > > + data_sz)) > > + return -EFAULT; > > Is lost type information a concern with copy_from_user.. let me check.. > I am not sure TBH.. there seems to be type based object size check in > there but does it work when indirected via void*? iirc void* is fine, just loss of fancy static checking. -Chris
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index cbcb957b7141..b4f26605f617 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -10,12 +10,33 @@ #include "i915_query.h" #include <uapi/drm/i915_drm.h> +static int init_query_item_check(void* data_ptr, size_t data_sz, + u32 total_length, + struct drm_i915_query_item *query_item) +{ + if (query_item->length == 0) + return total_length; + + if (query_item->length < total_length) + return -EINVAL; + + if (copy_from_user(data_ptr, u64_to_user_ptr(query_item->data_ptr), + data_sz)) + return -EFAULT; + + if (!access_ok(u64_to_user_ptr(query_item->data_ptr), + total_length)) + return -EFAULT; + + return 0; +} + static int query_topology_info(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) { const struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu; struct drm_i915_query_topology_info topo; - u32 slice_length, subslice_length, eu_length, total_length; + u32 ret, slice_length, subslice_length, eu_length, total_length; if (query_item->flags != 0) return -EINVAL; @@ -33,23 +54,14 @@ static int query_topology_info(struct drm_i915_private *dev_priv, total_length = sizeof(topo) + slice_length + subslice_length + eu_length; - if (query_item->length == 0) - return total_length; - - if (query_item->length < total_length) - return -EINVAL; - - if (copy_from_user(&topo, u64_to_user_ptr(query_item->data_ptr), - sizeof(topo))) - return -EFAULT; + ret = init_query_item_check(&topo, sizeof(topo), total_length, + query_item); + if (ret != 0) + return ret; if (topo.flags != 0) return -EINVAL; - if (!access_ok(u64_to_user_ptr(query_item->data_ptr), - total_length)) - return -EFAULT; - memset(&topo, 0, sizeof(topo)); topo.max_slices = sseu->max_slices; topo.max_subslices = sseu->max_subslices;
This simplifies adding new query item objects. Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_query.c | 40 ++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-)