[Intel-gfx] [PATCH v4] drm/i915: Prevent overflow of execbuf.buffer_count and num_cliprects

[Tvrtko Ursulin]

On 16/11/2017 09:36, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-16 09:00:08)
>>
>> On 15/11/2017 16:08, Chris Wilson wrote:
>>> @@ -2083,14 +2083,14 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
>>>        if (!(args->flags & I915_EXEC_FENCE_ARRAY))
>>>                return NULL;
>>>    
>>> -     if (nfences > SIZE_MAX / sizeof(*fences))
>>> +     if (nfences > SIZE_MAX / max(sizeof(*fences), sizeof(*user)))
>>>                return ERR_PTR(-EINVAL);
>>
>> This check is just to stay in some reasonable limits, yes? Doesn't seem
>> to be directly related to the implementation below. I am not saying we
>> should allow more fences, just wondering whether it is worthy of a
>> comment? And maybe in that case it is not -EINVAL (ABI) but -ENOMEM (to
>> fake implementation cannot support it / doesn't want to support it)?
>> Assuming I did not miss something.
> 
>>>        user = u64_to_user_ptr(args->cliprects_ptr);
>>> -     if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
>>> +     if (!access_ok(VERIFY_READ, user, nfences * sizeof(*user)))
>>>                return ERR_PTR(-EFAULT);
>>>    
> 
> For access_ok we use nfences * sizeof(*user), therefore we have to make
> sure that doesn't overflow and produce small value which then passes the
> check and allows the caller to wander off into the badlands. So really
> the question is it EFAULT for the invalid pointer or EINVAL or the
> unusable parameters. We've (I've) taken the stance that we wanted to
> differentiate between the parameters being bogus and the result of using
> those parameters being bogus.

Oh right.. but looking at access_ok it seems that it is working with 
unsigned longs. Not that it is documents explicitly, it is just a macro 
and happens to use that under the covers (and via mm_segment_t type as 
well). So it happens to match with size_t, but perhaps it indeed 
warrants a comment and changing it to ULONG_MAX.

Regards,

Tvrtko