diff mbox series

drm/i915/query: Split out query item checks

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

Commit Message

Abdiel Janulgue Jan. 9, 2019, 7:51 a.m. UTC
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(-)

Comments

Tvrtko Ursulin Jan. 9, 2019, 9:25 a.m. UTC | #1
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
Chris Wilson Jan. 9, 2019, 9:35 a.m. UTC | #2
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 mbox series

Patch

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;