[PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Rob Clark]

On Wed, Dec 16, 2015 at 12:33 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Laurent,
>
> On 14 December 2015 at 20:33, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
>> Hi Emil,
>>
>> On Monday 07 December 2015 14:13:43 Emil Velikov wrote:
>>> On 4 December 2015 at 22:27, Laurent Pinchart wrote:
>>> > The 8 high order bits of the buffer flags are reserved for internal use.
>>> > Mask them out from the flags passed by userspace.
>>>
>>> Ouch... I know that the Intel guys are pretty rigorous about input
>>> validation, although I wonder how many drivers are (partially or not)
>>> missing these.
>>>
>>> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> > ---
>>> >
>>> >  drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
>>> >  include/uapi/drm/omap_drm.h        | 1 +
>>> >  2 files changed, 7 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 83d63d71062c..e405ab23d7a6
>>> > 100644
>>> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > @@ -551,10 +551,13 @@ static int ioctl_gem_new(struct drm_device *dev,
>>> > void *data,
>>> >                 struct drm_file *file_priv)
>>> >  {
>>> >         struct drm_omap_gem_new *args = data;
>>> > +       u32 flags = args->flags & OMAP_BO_USER_MASK;
>>> > +
>>> >         VERB("%p:%p: size=0x%08x, flags=%08x", dev, file_priv,
>>> > -                       args->size.bytes, args->flags);
>>> > -       return omap_gem_new_handle(dev, file_priv, args->size,
>>> > -                       args->flags, &args->handle);
>>> > +            args->size.bytes, flags);
>>> > +
>>> > +       return omap_gem_new_handle(dev, file_priv, args->size, flags,
>>> > +                                  &args->handle);
>>> >  }
>>> >
>>> >  static int ioctl_gem_cpu_prep(struct drm_device *dev, void *data,
>>> > diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
>>> > index 1d0b1172664e..6852c26e8f78 100644
>>> > --- a/include/uapi/drm/omap_drm.h
>>> > +++ b/include/uapi/drm/omap_drm.h
>>> > @@ -36,6 +36,7 @@ struct drm_omap_param {
>>> >
>>> >  #define OMAP_BO_SCANOUT        0x00000001      /* scanout capable (phys
>>> >  contiguous) */
>>> >  #define OMAP_BO_CACHE_MASK     0x00000006      /* cache type mask, see
>>> >  cache modes */
>>> >  #define OMAP_BO_TILED_MASK     0x00000f00      /* tiled mapping mask, see
>>> >  tiled modes */
>>> > +#define OMAP_BO_USER_MASK      0x00ffffff      /* flags settable by
>>> > userspace */
>>>
>>> Fwiw I'm not too sure that adding the mask here  (UAPI) is a good
>>> idea.
>>
>> Good point, I'll make it private.
>>
> Thanks for the update (hiding the extra define from UAPI). Shame we
> cannot do that for OMAP_BO_TILED_MASK :'-(
>
>>> If you like to keep it, I'd suggest that it encapsulates only what's
>>> currently available. Might even want to move the individual masks in the
>>> respective sections/hunks. Speaking of which - OMAP_BO_TILED_MASK should be
>>> 0x00000300, shouldn't it ?
>>
>> I suppose 0x00000f00 was chosen to reserve a couple of bits for additional
>> tiled modes.
>>
> Perhaps a silly question - what is the reason to have larger mask than
> the actual supported flags ? Looking from other drm modules (and vague
> memories of other subsystems), drivers do not try to be forward
> compatible and reject (should reject that is) unsupported flags.

yeah, it was chosen to reserve some extra bits if needed in the future..

but yeah, current kernel should probably reject unknown tile modes..
which I think should be easy since iirc there are only 3 (which is a
nice maskable number)

BR,
-R

> Am I missing something ?
>
> Thanks
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel