On Tuesday, November 9th, 2021 at 10:24, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote:
> > There are a few details specific to the GETFB2 IOCTL.
> >
> > It's not immediately clear how user-space should check for the
> > number of planes. Suggest using the pitches field.
> >
> > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR.
> > So explicitly tell user-space to not look at it unless the flag is
> > set.
> >
> > Signed-off-by: Simon Ser <contact@xxxxxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx>
> > ---
> > include/uapi/drm/drm.h | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 3b810b53ba8b..9809078b0f51 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -1096,6 +1096,22 @@ extern "C" {
> > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer)
> > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
> >
> > +/**
> > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata.
> > + *
> > + * This queries metadata about a framebuffer. User-space fills
> > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the
> > + * struct as the output.
> > + *
> > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN,
> > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a
> > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of
> > + * planes.
>
> Maybe explain that when actually importing the buffer userspace should
> look for non-zeor handles instead, since some drivers/formats leave a
> silly pitch value behind. Or at least I think that can happen. Just for
> additional robustness?
The IOCTL zeroes out the arrays at the start, so should be fine.
handles will all be zero if user-space is unprivileged, so can't use them to
figure out the number of planes.
> > + *
> > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set
> > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has
> > + * undefined contents.
>
> Uh is this true? We should always clear values to avoid accidental leaks
> and stuff.
See the commit message: the modifier array is zeroed, which means it's filled
with DRM_FORMAT_MOD_LINEAR instead of DRM_FORMAT_MOD_INVALID -- this as good as
undefined contents for me.
Since this is a pretty good footgun, maybe we should change this? But the docs
would still need to tell userspace to not look at the array because older
kernels will still exist.
> > + */
>
> I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can
> document how pitch is supposed to work and all that stuff.
Agreed, maybe will take a stab at this once this one is merged.
