[Intel-gfx] [PATCH v2 1/2] drm/i915/userptr: add user_size limit check

[Chris Wilson]

Quoting Chris Wilson (2020-01-17 12:10:58)
> Quoting Matthew Auld (2020-01-17 11:51:53)
> > On Thu, 16 Jan 2020 at 21:19, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > >
> > > Quoting Matthew Auld (2020-01-16 20:31:49)
> > > > Don't allow a mismatch between obj->base.size/vma->size and the actual
> > > > number of pages for the backing store, which is limited to INT_MAX
> > > > pages.
> > > >
> > > > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > > index e5558af111e2..fef96a303d9d 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > > @@ -768,6 +768,18 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> > > >         if (args->flags & ~(I915_USERPTR_READ_ONLY |
> > > >                             I915_USERPTR_UNSYNCHRONIZED))
> > > >                 return -EINVAL;
> > > > +       /*
> > > > +        * XXX: There is a prevalence of the assumption that we fit the
> > > > +        * object's page count inside a 32bit _signed_ variable. Let's document
> > > > +        * this and catch if we ever need to fix it. In the meantime, if you do
> > > > +        * spot such a local variable, please consider fixing!
> > > > +        */
> > > > +
> > > > +       if (args->user_size >> PAGE_SHIFT > INT_MAX)
> > > > +               return -E2BIG;
> > >
> > > I'm convinced that the following patch is the last bug (excusing
> > > i915_gem_internal.c), and think we should commit to removing this limit.
> > 
> > You mean on our side? There is still all the sg_table stuff,
> > __get_user_pages_fast etc.
> 
> Didn't notice the get_user_pages -- some use long, sone ints. oops.
> 
> sg_table I was thinking of just the sg_length snafu that we work around.
> We can kill off sg_table itself as we never pass that outside of the
> driver, and just assume our chunking is correct. (Basically lifting more
> of lib/scatterlist.c into our control, one day we really should tell
> them their code doesn't scale to our use.)
> 
> Ok. Let's collate this information into something like
> 
>        /*
>         * XXX: There is a prevalence of the assumption that we fit the
>         * object's page count inside a 32bit _signed_ variable. Let's document
>         * this and catch if we ever need to fix it. In the meantime, if you do
>         * spot such a local variable, please consider fixing!
>         *
>         * Aside from our own locals (for which we have no excuse!):
>         * - sg_table embeds unsigned int for num_pages
>         * - get_user_pages*() mixed ints with longs
>         */

If you include the information about what we know remains,

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris