[v2,1/2] drm/i915/userptr: add user_size limit check
diff mbox series

Message ID 20200116203150.923826-1-matthew.auld@intel.com
State New
Headers show
Series
  • [v2,1/2] drm/i915/userptr: add user_size limit check
Related show

Commit Message

Matthew Auld Jan. 16, 2020, 8:31 p.m. UTC
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@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Chris Wilson Jan. 16, 2020, 9:18 p.m. UTC | #1
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@intel.com>
> Cc: Chris Wilson <chris@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.
-Chris
Matthew Auld Jan. 17, 2020, 11:51 a.m. UTC | #2
On Thu, 16 Jan 2020 at 21:19, Chris Wilson <chris@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@intel.com>
> > Cc: Chris Wilson <chris@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.

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Jan. 17, 2020, 12:10 p.m. UTC | #3
Quoting Matthew Auld (2020-01-17 11:51:53)
> On Thu, 16 Jan 2020 at 21:19, Chris Wilson <chris@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@intel.com>
> > > Cc: Chris Wilson <chris@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
        */

We can send patches for get_user_pages...
-Chris
Chris Wilson Jan. 17, 2020, 1:08 p.m. UTC | #4
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@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@intel.com>
> > > > Cc: Chris Wilson <chris@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@chris-wilson.co.uk>
-Chris

Patch
diff mbox series

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;
+
+	if (overflows_type(args->user_size, obj->base.size))
+		return -E2BIG;
 
 	if (!args->user_size)
 		return -EINVAL;