| Message ID | 20180904205319.11174-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Reject unknown legacy bpp and dpeth for drm_mode_addfb ioctl | expand |
On Tue, Sep 04, 2018 at 09:53:19PM +0100, Chris Wilson wrote: > Since this is handling user provided bpp and depth, we need to sanity > check and propagate the EINVAL back rather than assume what the insane > client intended and fill the logs with DRM_ERROR. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > So I am presuming that r.pixel_format == 0 is rejected elsewhere for the > internal users (as if any would deliberately provoke the error)! Could maybe add a DRM_FORMAT_INVALID at the end of drm_fourcc.h, and then switch over the various format/modifier tables to being zero terminated. Well DRM_FORMAT_MOD_INVALID can't be 0 because that means linear. Anyway, I digress, this loks good. And yes drm_internal_framebuffer_create makes sure you have a real fourcc code, not a figment of your imagination (or more profane, stack garbage). Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Same as with the previous one, igt would be sweet on top. -Daniel > --- > drivers/gpu/drm/drm_fourcc.c | 4 ++-- > drivers/gpu/drm/drm_framebuffer.c | 5 ++++- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 35c1e2742c27..34595c5b55c9 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -69,8 +69,8 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) > fmt = DRM_FORMAT_ARGB8888; > break; > default: > - DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n"); > - fmt = DRM_FORMAT_XRGB8888; > + DRM_DEBUG("bad bpp [%d] and depth [%d]\n", bpp, depth); > + fmt = 0; > break; > } > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 781af1d42d76..7641bddfe367 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -112,12 +112,15 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or, > struct drm_mode_fb_cmd2 r = {}; > int ret; > > + r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); > + if (!r.pixel_format) > + return -EINVAL; > + > /* convert to new format and call new ioctl */ > r.fb_id = or->fb_id; > r.width = or->width; > r.height = or->height; > r.pitches[0] = or->pitch; > - r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); > r.handles[0] = or->handle; > > if (r.pixel_format == DRM_FORMAT_XRGB2101010 && > -- > 2.19.0.rc1 >
Quoting Daniel Vetter (2018-09-04 22:46:33) > On Tue, Sep 04, 2018 at 09:53:19PM +0100, Chris Wilson wrote: > > Since this is handling user provided bpp and depth, we need to sanity > > check and propagate the EINVAL back rather than assume what the insane > > client intended and fill the logs with DRM_ERROR. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > So I am presuming that r.pixel_format == 0 is rejected elsewhere for the > > internal users (as if any would deliberately provoke the error)! > > Could maybe add a DRM_FORMAT_INVALID at the end of drm_fourcc.h, and then > switch over the various format/modifier tables to being zero terminated. > Well DRM_FORMAT_MOD_INVALID can't be 0 because that means linear. Anyway, > I digress, this loks good. > > And yes drm_internal_framebuffer_create makes sure you have a real fourcc > code, not a figment of your imagination (or more profane, stack garbage). > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Same as with the previous one, igt would be sweet on top. And the first thing one does with the test case is realise that we never check that depth is valid for the bpp based pixel format. -Chris
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 35c1e2742c27..34595c5b55c9 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -69,8 +69,8 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) fmt = DRM_FORMAT_ARGB8888; break; default: - DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n"); - fmt = DRM_FORMAT_XRGB8888; + DRM_DEBUG("bad bpp [%d] and depth [%d]\n", bpp, depth); + fmt = 0; break; } diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..7641bddfe367 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -112,12 +112,15 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or, struct drm_mode_fb_cmd2 r = {}; int ret; + r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); + if (!r.pixel_format) + return -EINVAL; + /* convert to new format and call new ioctl */ r.fb_id = or->fb_id; r.width = or->width; r.height = or->height; r.pitches[0] = or->pitch; - r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth); r.handles[0] = or->handle; if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
Since this is handling user provided bpp and depth, we need to sanity check and propagate the EINVAL back rather than assume what the insane client intended and fill the logs with DRM_ERROR. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- So I am presuming that r.pixel_format == 0 is rejected elsewhere for the internal users (as if any would deliberately provoke the error)! --- drivers/gpu/drm/drm_fourcc.c | 4 ++-- drivers/gpu/drm/drm_framebuffer.c | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-)