| Message ID | 20180905102205.5472-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Reject unknown legacy bpp and depth for drm_mode_addfb ioctl | expand |
Quoting Chris Wilson (2018-09-05 11:22:05) > 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. > > v2: Check both bpp and depth match the builtin pixel format, and > introduce a canonical DRM_FORMAT_INVALID to reserve 0 against any future > fourcc. > > Testcase: igt/kms_addfb_basic/legacy-format > 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> > --- > drivers/gpu/drm/drm_fourcc.c | 33 +++++++++++++++++++++---------- > drivers/gpu/drm/drm_framebuffer.c | 7 ++++++- > include/uapi/drm/drm_fourcc.h | 3 +++ > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 35c1e2742c27..d9dadbc43327 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -45,32 +45,45 @@ static char printable_char(int c) > */ > uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) > { > - uint32_t fmt; > + uint32_t fmt = DRM_FORMAT_INVALID; > > switch (bpp) { > case 8: > - fmt = DRM_FORMAT_C8; > + if (depth == 0) > + fmt = DRM_FORMAT_C8; Of course the downside with increasing specificity is if any userspace was already feeding in garbage depth that just happens to work :( -Chris
On Wed, Sep 05, 2018 at 11:22:05AM +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. > > v2: Check both bpp and depth match the builtin pixel format, and > introduce a canonical DRM_FORMAT_INVALID to reserve 0 against any future > fourcc. > > Testcase: igt/kms_addfb_basic/legacy-format > 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> I checked a bunch of randomly selected userspace pieces, and if there's hairy stuff going on then it's mis-selected bpp. > --- > drivers/gpu/drm/drm_fourcc.c | 33 +++++++++++++++++++++---------- > drivers/gpu/drm/drm_framebuffer.c | 7 ++++++- > include/uapi/drm/drm_fourcc.h | 3 +++ > 3 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 35c1e2742c27..d9dadbc43327 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -45,32 +45,45 @@ static char printable_char(int c) > */ > uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) > { > - uint32_t fmt; > + uint32_t fmt = DRM_FORMAT_INVALID; > > switch (bpp) { > case 8: > - fmt = DRM_FORMAT_C8; > + if (depth == 0) > + fmt = DRM_FORMAT_C8; Michel Dänzer thinks this should be depth == 8 here. I grepped around in -modesetting, seems outright not supported. Apparently amd drivers do support it, and set 8/8. With that addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > break; > case 16: > - if (depth == 15) > + switch (depth) { > + case 15: > fmt = DRM_FORMAT_XRGB1555; > - else > + break; > + case 16: > fmt = DRM_FORMAT_RGB565; > + break; > + default: > + break; > + } > break; > case 24: > - fmt = DRM_FORMAT_RGB888; > + if (depth == 24) > + fmt = DRM_FORMAT_RGB888; > break; > case 32: > - if (depth == 24) > + switch (depth) { > + case 24: > fmt = DRM_FORMAT_XRGB8888; > - else if (depth == 30) > + break; > + case 30: > fmt = DRM_FORMAT_XRGB2101010; > - else > + break; > + case 32: > fmt = DRM_FORMAT_ARGB8888; > + break; > + default: > + break; > + } > break; > default: > - DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n"); > - fmt = DRM_FORMAT_XRGB8888; > break; > } > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 781af1d42d76..636f626c5828 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -112,12 +112,17 @@ 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 == DRM_FORMAT_INVALID) { > + DRM_DEBUG("bad (bpp:%d, depth:%d)\n", or->bpp, or->depth); > + 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 && > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index 2ed46e9ae16a..139632b87181 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -71,6 +71,9 @@ extern "C" { > > #define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */ > > +/* Reserve 0 for the invalid format specifier */ > +#define DRM_FORMAT_INVALID 0 > + > /* color index */ > #define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */ > > -- > 2.19.0.rc1 >
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 35c1e2742c27..d9dadbc43327 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -45,32 +45,45 @@ static char printable_char(int c) */ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) { - uint32_t fmt; + uint32_t fmt = DRM_FORMAT_INVALID; switch (bpp) { case 8: - fmt = DRM_FORMAT_C8; + if (depth == 0) + fmt = DRM_FORMAT_C8; break; case 16: - if (depth == 15) + switch (depth) { + case 15: fmt = DRM_FORMAT_XRGB1555; - else + break; + case 16: fmt = DRM_FORMAT_RGB565; + break; + default: + break; + } break; case 24: - fmt = DRM_FORMAT_RGB888; + if (depth == 24) + fmt = DRM_FORMAT_RGB888; break; case 32: - if (depth == 24) + switch (depth) { + case 24: fmt = DRM_FORMAT_XRGB8888; - else if (depth == 30) + break; + case 30: fmt = DRM_FORMAT_XRGB2101010; - else + break; + case 32: fmt = DRM_FORMAT_ARGB8888; + break; + default: + break; + } break; default: - DRM_ERROR("bad bpp, assuming x8r8g8b8 pixel format\n"); - fmt = DRM_FORMAT_XRGB8888; break; } diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..636f626c5828 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -112,12 +112,17 @@ 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 == DRM_FORMAT_INVALID) { + DRM_DEBUG("bad (bpp:%d, depth:%d)\n", or->bpp, or->depth); + 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 && diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 2ed46e9ae16a..139632b87181 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -71,6 +71,9 @@ extern "C" { #define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */ +/* Reserve 0 for the invalid format specifier */ +#define DRM_FORMAT_INVALID 0 + /* color index */ #define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
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. v2: Check both bpp and depth match the builtin pixel format, and introduce a canonical DRM_FORMAT_INVALID to reserve 0 against any future fourcc. Testcase: igt/kms_addfb_basic/legacy-format 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> --- drivers/gpu/drm/drm_fourcc.c | 33 +++++++++++++++++++++---------- drivers/gpu/drm/drm_framebuffer.c | 7 ++++++- include/uapi/drm/drm_fourcc.h | 3 +++ 3 files changed, 32 insertions(+), 11 deletions(-)