[Intel-gfx] [PATCH v7 6/6] drm/i915: expose rcs topology through query uAPI

[Lionel Landwerlin]

On 16/01/18 21:48, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-01-16 19:18:24)
>> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
>> index 51736af7f573..038f292e1f2a 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -25,8 +25,102 @@
>>   #include "i915_drv.h"
>>   #include <uapi/drm/i915_drm.h>
>>   
>> +static int copy_query_data(struct drm_i915_query_item *query_item,
>> +                          const void *item_ptr, u32 item_length,
>> +                          const void *data_ptr, u32 data_length)
>> +{
>> +       u32 total_length = item_length + data_length;
> BUG_ON(add_overflows(item_length, data_length)) ?

Sure. At the moment those 2 lengths are dictated by uapi structs and 
internal numbers in sseu_dev_info, so unlikely to overflow.
Belt & braces!

>
>> +
>> +       if (query_item->length == 0)
>> +               return total_length;
>> +
>> +       if (query_item->length < total_length)
>> +               return -EINVAL;
>> +
>> +       if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
>> +                        item_ptr, item_length))
>> +               return -EFAULT;
>> +
>> +       if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + item_length),
>> +                        data_ptr, data_length))
>> +               return -EFAULT;
> Unchecked overflow (data_ptr + item_length) here.
>
> if (!access_ok(VERIFY_WRITE, u64_to_user_ptr(query_item->data_ptr), total_length))
> 	return -EFAULT;
>
> Would cover it, and then if you wanted you could then use
> __copy_to_user().

I thought copy_to_user() did an access_ok() internally. Am I wrong?

>
>> +
>> +       return total_length;
>> +}
>> +
>> +static int query_slice_info(struct drm_i915_private *dev_priv,
>> +                           struct drm_i915_query_item *query_item)
>> +{
>> +       const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>> +       struct drm_i915_query_slice_info slice_info;
>> +
>> +       if (sseu->max_slices == 0)
>> +               return -ENODEV;
> Brr. This would then abort all queries not just this one, that doesn't
> seem very user friendly. EFAULT are definite abort, can't continue, this
> is just factual.
> -Chris
>

Ah... Very good point. I guess we should return a 0 length to mean 
unsupported query?
Will add a comment in uapi.