Quoting Lionel Landwerlin (2018-01-17 12:39:48)
> 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?
It does. What I'm arguing is that if it only checks the last byte is
valid we will miss the overflow. First copy_to_user() says all bytes
valid, then second copy_to_user() starts at 0, which may also be mapped
into the process, however the combine copy is invalid. Just worrying
about edge cases that may never exist. But we don't want to be the next
/dev/samsung_backdoor do we? :)
> >> +
> >> + 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.
0 length seems like it should work. Let's see how you feel after hooking
up userspace :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
