| Message ID | 20171114183258.16976-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2017-11-14 18:32:50) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently userspace is allowed to feed in any king of garbage in the > high bits of the mode flags/type, as are drivers when probing modes. > Reject any mode with bogus flags/type. > > Hopefully this won't break any current userspace... > > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > Cc: Adam Jackson <ajax@redhat.com> > Cc: Keith Packard <keithp@keithp.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_modes.c | 4 ++++ > include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 1a72883b836e..f99ba963fb3e 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); > enum drm_mode_status > drm_mode_validate_basic(const struct drm_display_mode *mode) > { > + if (mode->type & ~DRM_MODE_TYPE_ALL || > + mode->flags & ~DRM_MODE_FLAG_ALL) > + return MODE_BAD; I had to read this twice to realise they were different masks. (If the start and end of a word match expectations, the eye skips the middle.) Can we split this up into two separate ifs, so the reader doesn't fall into this trap :) -Chris
On Tue, Nov 14, 2017 at 06:42:06PM +0000, Chris Wilson wrote: > Quoting Ville Syrjala (2017-11-14 18:32:50) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently userspace is allowed to feed in any king of garbage in the > > high bits of the mode flags/type, as are drivers when probing modes. > > Reject any mode with bogus flags/type. > > > > Hopefully this won't break any current userspace... > > > > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > > Cc: Adam Jackson <ajax@redhat.com> > > Cc: Keith Packard <keithp@keithp.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_modes.c | 4 ++++ > > include/uapi/drm/drm_mode.h | 24 ++++++++++++++++++++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > > index 1a72883b836e..f99ba963fb3e 100644 > > --- a/drivers/gpu/drm/drm_modes.c > > +++ b/drivers/gpu/drm/drm_modes.c > > @@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); > > enum drm_mode_status > > drm_mode_validate_basic(const struct drm_display_mode *mode) > > { > > + if (mode->type & ~DRM_MODE_TYPE_ALL || > > + mode->flags & ~DRM_MODE_FLAG_ALL) > > + return MODE_BAD; > > I had to read this twice to realise they were different masks. (If the > start and end of a word match expectations, the eye skips the middle.) > Can we split this up into two separate ifs, so the reader doesn't fall > into this trap :) Sure, I can split that up.
Hi, Ville, On 11/14/2017 07:32 PM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently userspace is allowed to feed in any king of garbage in the > high bits of the mode flags/type, as are drivers when probing modes. > Reject any mode with bogus flags/type. > > Hopefully this won't break any current userspace... Unfortunately this breaks xf86-video-vmware which currently leaves the mode->type uninitialized :(. That code is inherited from the old gallium xorg state tracker. I don't know whether it has spread elsewhere but the symptoms are that the X server frequently dies failing to set screen resources, a very uninformative error. I'll fix the xf86-video-vmware driver, but I guess we need to back out the mode->type check. /Thomas
On Wed, Mar 21, 2018 at 09:45:09PM +0100, Thomas Hellstrom wrote: > Hi, Ville, > > On 11/14/2017 07:32 PM, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Currently userspace is allowed to feed in any king of garbage in the > > high bits of the mode flags/type, as are drivers when probing modes. > > Reject any mode with bogus flags/type. > > > > Hopefully this won't break any current userspace... > > Unfortunately this breaks xf86-video-vmware which currently leaves the > mode->type uninitialized :(. > That code is inherited from the old gallium xorg state tracker. I don't > know whether it has spread elsewhere but the symptoms are that the X > server frequently dies failing to set screen resources, a very > uninformative error. > > I'll fix the xf86-video-vmware driver, but I guess we need to back out > the mode->type check. Dang. So the flags check is fine but type check is not?
On 03/21/2018 09:51 PM, Ville Syrjälä wrote: > On Wed, Mar 21, 2018 at 09:45:09PM +0100, Thomas Hellstrom wrote: >> Hi, Ville, >> >> On 11/14/2017 07:32 PM, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> Currently userspace is allowed to feed in any king of garbage in the >>> high bits of the mode flags/type, as are drivers when probing modes. >>> Reject any mode with bogus flags/type. >>> >>> Hopefully this won't break any current userspace... >> Unfortunately this breaks xf86-video-vmware which currently leaves the >> mode->type uninitialized :(. >> That code is inherited from the old gallium xorg state tracker. I don't >> know whether it has spread elsewhere but the symptoms are that the X >> server frequently dies failing to set screen resources, a very >> uninformative error. >> >> I'll fix the xf86-video-vmware driver, but I guess we need to back out >> the mode->type check. > Dang. So the flags check is fine but type check is not? > Yes, we copy the flags field untranslated from xorg's DisplayModePtr::Flags field which seems to be what xf86-video-modesetting does as well, so I guess we should be OK there. /Thomas
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 1a72883b836e..f99ba963fb3e 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1036,6 +1036,10 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode) { + if (mode->type & ~DRM_MODE_TYPE_ALL || + mode->flags & ~DRM_MODE_FLAG_ALL) + return MODE_BAD; + if ((mode->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX) return MODE_BAD; diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5597a87154e5..004db470b477 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -46,6 +46,14 @@ extern "C" { #define DRM_MODE_TYPE_USERDEF (1<<5) #define DRM_MODE_TYPE_DRIVER (1<<6) +#define DRM_MODE_TYPE_ALL (DRM_MODE_TYPE_BUILTIN | \ + DRM_MODE_TYPE_CLOCK_C | \ + DRM_MODE_TYPE_CRTC_C | \ + DRM_MODE_TYPE_PREFERRED | \ + DRM_MODE_TYPE_DEFAULT | \ + DRM_MODE_TYPE_USERDEF | \ + DRM_MODE_TYPE_DRIVER) + /* Video mode flags */ /* bit compatible with the xrandr RR_ definitions (bits 0-13) * @@ -99,6 +107,22 @@ extern "C" { #define DRM_MODE_FLAG_PIC_AR_16_9 \ (DRM_MODE_PICTURE_ASPECT_16_9<<19) +#define DRM_MODE_FLAG_ALL (DRM_MODE_FLAG_PHSYNC | \ + DRM_MODE_FLAG_NHSYNC | \ + DRM_MODE_FLAG_PVSYNC | \ + DRM_MODE_FLAG_NVSYNC | \ + DRM_MODE_FLAG_INTERLACE | \ + DRM_MODE_FLAG_DBLSCAN | \ + DRM_MODE_FLAG_CSYNC | \ + DRM_MODE_FLAG_PCSYNC | \ + DRM_MODE_FLAG_NCSYNC | \ + DRM_MODE_FLAG_HSKEW | \ + DRM_MODE_FLAG_BCAST | \ + DRM_MODE_FLAG_PIXMUX | \ + DRM_MODE_FLAG_DBLCLK | \ + DRM_MODE_FLAG_CLKDIV2 | \ + DRM_MODE_FLAG_3D_MASK) + /* DPMS flags */ /* bit compatible with the xorg definitions. */ #define DRM_MODE_DPMS_ON 0