drm/i915/gem: Hook user-extensions upto MMAP_OFFSET_IOCTL
diff mbox series

Message ID 20191204162803.3841140-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gem: Hook user-extensions upto MMAP_OFFSET_IOCTL
Related show

Commit Message

Chris Wilson Dec. 4, 2019, 4:28 p.m. UTC
From: Chris Wilson <ickle@braswell.alporthouse.com>

Call i915_user_extensions() to validate the arg->extensions pointer, and
so return consistent error numbers for the future.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Matthew Auld Dec. 4, 2019, 4:52 p.m. UTC | #1
On Wed, 4 Dec 2019 at 16:29, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> From: Chris Wilson <ickle@braswell.alporthouse.com>
>
> Call i915_user_extensions() to validate the arg->extensions pointer, and
> so return consistent error numbers for the future.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 37aabbfa869a..3a3f30bc8ac7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -17,6 +17,7 @@
>  #include "i915_gem_object.h"
>  #include "i915_gem_mman.h"
>  #include "i915_trace.h"
> +#include "i915_user_extensions.h"
>  #include "i915_vma.h"
>
>  static inline bool
> @@ -617,9 +618,12 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
>         struct drm_i915_private *i915 = to_i915(dev);
>         struct drm_i915_gem_mmap_offset *args = data;
>         enum i915_mmap_type type;
> +       int err;
>
> -       if (args->extensions)
> -               return -EINVAL;
> +       err = i915_user_extensions(u64_to_user_ptr(args->extensions),
> +                                  NULL, 0, NULL);
> +       if (err)
> +               return err;

Don't we have to check get_user first on the ptr?

>
>         switch (args->flags) {
>         case I915_MMAP_OFFSET_GTT:
> --
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Dec. 4, 2019, 5:24 p.m. UTC | #2
Quoting Matthew Auld (2019-12-04 16:52:12)
> On Wed, 4 Dec 2019 at 16:29, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > From: Chris Wilson <ickle@braswell.alporthouse.com>
> >
> > Call i915_user_extensions() to validate the arg->extensions pointer, and
> > so return consistent error numbers for the future.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 37aabbfa869a..3a3f30bc8ac7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -17,6 +17,7 @@
> >  #include "i915_gem_object.h"
> >  #include "i915_gem_mman.h"
> >  #include "i915_trace.h"
> > +#include "i915_user_extensions.h"
> >  #include "i915_vma.h"
> >
> >  static inline bool
> > @@ -617,9 +618,12 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> >         struct drm_i915_private *i915 = to_i915(dev);
> >         struct drm_i915_gem_mmap_offset *args = data;
> >         enum i915_mmap_type type;
> > +       int err;
> >
> > -       if (args->extensions)
> > -               return -EINVAL;
> > +       err = i915_user_extensions(u64_to_user_ptr(args->extensions),
> > +                                  NULL, 0, NULL);
> > +       if (err)
> > +               return err;
> 
> Don't we have to check get_user first on the ptr?

No, it's checked by i915_user_extensions(). It's a bit redundant in the
other callers :) Since the first thing we do is while (ext).

I have some memory of starting out with NULL being an error for
i915_user_extensions, but we settled on while(ext) for a compact
non-recursive loop.
-Chris
Matthew Auld Dec. 4, 2019, 5:58 p.m. UTC | #3
On Wed, 4 Dec 2019 at 17:24, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Matthew Auld (2019-12-04 16:52:12)
> > On Wed, 4 Dec 2019 at 16:29, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >
> > > From: Chris Wilson <ickle@braswell.alporthouse.com>
> > >
> > > Call i915_user_extensions() to validate the arg->extensions pointer, and
> > > so return consistent error numbers for the future.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > index 37aabbfa869a..3a3f30bc8ac7 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > @@ -17,6 +17,7 @@
> > >  #include "i915_gem_object.h"
> > >  #include "i915_gem_mman.h"
> > >  #include "i915_trace.h"
> > > +#include "i915_user_extensions.h"
> > >  #include "i915_vma.h"
> > >
> > >  static inline bool
> > > @@ -617,9 +618,12 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> > >         struct drm_i915_private *i915 = to_i915(dev);
> > >         struct drm_i915_gem_mmap_offset *args = data;
> > >         enum i915_mmap_type type;
> > > +       int err;
> > >
> > > -       if (args->extensions)
> > > -               return -EINVAL;
> > > +       err = i915_user_extensions(u64_to_user_ptr(args->extensions),
> > > +                                  NULL, 0, NULL);
> > > +       if (err)
> > > +               return err;
> >
> > Don't we have to check get_user first on the ptr?
>
> No, it's checked by i915_user_extensions(). It's a bit redundant in the
> other callers :) Since the first thing we do is while (ext).
>
> I have some memory of starting out with NULL being an error for
> i915_user_extensions, but we settled on while(ext) for a compact
> non-recursive loop.

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> -Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 37aabbfa869a..3a3f30bc8ac7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -17,6 +17,7 @@ 
 #include "i915_gem_object.h"
 #include "i915_gem_mman.h"
 #include "i915_trace.h"
+#include "i915_user_extensions.h"
 #include "i915_vma.h"
 
 static inline bool
@@ -617,9 +618,12 @@  i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_private *i915 = to_i915(dev);
 	struct drm_i915_gem_mmap_offset *args = data;
 	enum i915_mmap_type type;
+	int err;
 
-	if (args->extensions)
-		return -EINVAL;
+	err = i915_user_extensions(u64_to_user_ptr(args->extensions),
+				   NULL, 0, NULL);
+	if (err)
+		return err;
 
 	switch (args->flags) {
 	case I915_MMAP_OFFSET_GTT: