[Intel-gfx] [PATCH] drm/i915/query: Split out query item checks

[Tvrtko Ursulin]

On 09/01/2019 07:51, Abdiel Janulgue wrote:
> This simplifies adding new query item objects.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at 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