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

[Emil Velikov]

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.

Am I missing something ?

Thanks
Emil