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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
