[PATCH 0/3] drm: Basic mode sanity checks

[PATCH 0/3] drm: Basic mode sanity checks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We had a bug in i915 land recently where X passed in a zeroed mode with
mode_valid=1 to setcrtc. That didn't go down so well and caused a
div-by-zero in i915.

For a long time I've been thinking that we need some real checks to
validate the modes we feed into the hardware. I started to sketch out
something for that but it quickly turned into a bit of a nightmare
with the whole panel fitter, stereo 3D, SDVO etc. special cases we
have in i915.

So I gave up on that for now, and instead cooked up this small series
to add some basic sanity checks to the mode validation, and also apply
the same sanity checks to user provided modes. This is enough to
prevent the div-by-zero in i915 with buggy X.

The risk is that we start to reject some modes that more or less worked
by accident before. Given how lax we've been in handling the panel fitter
on i915 for instance, this could be a real problem if people have been
useing custom modes that have been filled out only partially. But I'm
hoping the number of users doing that is so small that we can risk it.

The other concern is drivers which might also provide funky modes from
their .get_modes(). I tried to look at all the drivers a bit, and most
produce modes via EDID, CVT, DMT, or drm_display_mode_from_videomode().
All of those look safe, except mode->clock might be an issue with
drm_display_mode_from_videomode(), but hopefully there's some kind of
clock provided in most cases. I didn't dig through all the nooks and
crannies in all drivers though, so may have missed something.

Ville Syrjälä (3):
  drm: Reorganize probed mode validation
  drm: Perform basic sanity checks on probed modes
  drm: Do basic sanity checks for user modes

 drivers/gpu/drm/drm_crtc.c         |  6 ++++
 drivers/gpu/drm/drm_modes.c        | 56 ++++++++++++++++++++++++++++++--------
 drivers/gpu/drm/drm_probe_helper.c | 43 ++++++++++++++---------------
 include/drm/drm_modes.h            |  6 ++--
 4 files changed, 74 insertions(+), 37 deletions(-)

-- 
2.0.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: Reorganize probed mode validation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make drm_mode_validate_size() and drm_mode_validate_flag() deal with a
single mode instead of having each iterate through the mode list.

The hope is that in the future we might be able to share various mode
validation functions between modeset and get_modes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c        | 24 +++++++++++------------
 drivers/gpu/drm/drm_probe_helper.c | 40 +++++++++++++++++---------------------
 include/drm/drm_modes.h            |  5 ++---
 3 files changed, 32 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index d1b7d20..61a4a9a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -907,8 +907,7 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
 /**
  * drm_mode_validate_size - make sure modes adhere to size constraints
- * @dev: DRM device
- * @mode_list: list of modes to check
+ * @mode: mode to check
  * @maxX: maximum width
  * @maxY: maximum height
  *
@@ -916,20 +915,21 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
  * limitations of the DRM device/connector. If a mode is too big its status
  * memeber is updated with the appropriate validation failure code. The list
  * itself is not changed.
+ *
+ * Returns:
+ * The mode status
  */
-void drm_mode_validate_size(struct drm_device *dev,
-			    struct list_head *mode_list,
-			    int maxX, int maxY)
+enum drm_mode_status
+drm_mode_validate_size(const struct drm_display_mode *mode,
+		       int maxX, int maxY)
 {
-	struct drm_display_mode *mode;
+	if (maxX > 0 && mode->hdisplay > maxX)
+		return MODE_VIRTUAL_X;
 
-	list_for_each_entry(mode, mode_list, head) {
-		if (maxX > 0 && mode->hdisplay > maxX)
-			mode->status = MODE_VIRTUAL_X;
+	if (maxY > 0 && mode->vdisplay > maxY)
+		return MODE_VIRTUAL_Y;
 
-		if (maxY > 0 && mode->vdisplay > maxY)
-			mode->status = MODE_VIRTUAL_Y;
-	}
+	return MODE_OK;
 }
 EXPORT_SYMBOL(drm_mode_validate_size);
 
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6857e9a..c637bd9 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -58,28 +58,23 @@
 static bool drm_kms_helper_poll = true;
 module_param_named(poll, drm_kms_helper_poll, bool, 0600);
 
-static void drm_mode_validate_flag(struct drm_connector *connector,
-				   int flags)
+static enum drm_mode_status
+drm_mode_validate_flag(const struct drm_display_mode *mode,
+		       int flags)
 {
-	struct drm_display_mode *mode;
+	if ((mode->flags & DRM_MODE_FLAG_INTERLACE) &&
+	    !(flags & DRM_MODE_FLAG_INTERLACE))
+		return MODE_NO_INTERLACE;
 
-	if (flags == (DRM_MODE_FLAG_DBLSCAN | DRM_MODE_FLAG_INTERLACE |
-		      DRM_MODE_FLAG_3D_MASK))
-		return;
+	if ((mode->flags & DRM_MODE_FLAG_DBLSCAN) &&
+	    !(flags & DRM_MODE_FLAG_DBLSCAN))
+		return MODE_NO_DBLESCAN;
 
-	list_for_each_entry(mode, &connector->modes, head) {
-		if ((mode->flags & DRM_MODE_FLAG_INTERLACE) &&
-				!(flags & DRM_MODE_FLAG_INTERLACE))
-			mode->status = MODE_NO_INTERLACE;
-		if ((mode->flags & DRM_MODE_FLAG_DBLSCAN) &&
-				!(flags & DRM_MODE_FLAG_DBLSCAN))
-			mode->status = MODE_NO_DBLESCAN;
-		if ((mode->flags & DRM_MODE_FLAG_3D_MASK) &&
-				!(flags & DRM_MODE_FLAG_3D_MASK))
-			mode->status = MODE_NO_STEREO;
-	}
+	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) &&
+	    !(flags & DRM_MODE_FLAG_3D_MASK))
+		return MODE_NO_STEREO;
 
-	return;
+	return MODE_OK;
 }
 
 static int drm_helper_probe_add_cmdline_mode(struct drm_connector *connector)
@@ -163,18 +158,19 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 
 	drm_mode_connector_list_update(connector, merge_type_bits);
 
-	if (maxX && maxY)
-		drm_mode_validate_size(dev, &connector->modes, maxX, maxY);
-
 	if (connector->interlace_allowed)
 		mode_flags |= DRM_MODE_FLAG_INTERLACE;
 	if (connector->doublescan_allowed)
 		mode_flags |= DRM_MODE_FLAG_DBLSCAN;
 	if (connector->stereo_allowed)
 		mode_flags |= DRM_MODE_FLAG_3D_MASK;
-	drm_mode_validate_flag(connector, mode_flags);
 
 	list_for_each_entry(mode, &connector->modes, head) {
+		mode->status = drm_mode_validate_size(mode, maxX, maxY);
+
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_flag(mode, mode_flags);
+
 		if (mode->status == MODE_OK && connector_funcs->mode_valid)
 			mode->status = connector_funcs->mode_valid(connector,
 								   mode);
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 91d0582..6c53114 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -217,9 +217,8 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2);
 
 /* for use by the crtc helper probe functions */
-void drm_mode_validate_size(struct drm_device *dev,
-			    struct list_head *mode_list,
-			    int maxX, int maxY);
+enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
+					    int maxX, int maxY);
 void drm_mode_prune_invalid(struct drm_device *dev,
 			    struct list_head *mode_list, bool verbose);
 void drm_mode_sort(struct list_head *mode_list);
-- 
2.0.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Perform basic sanity checks on probed modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure the timings of probed modes at least pass some very basic
sanity checks.

The checks include:
 - clock,hdisplay,vdisplay are non zero
 - sync pulse fits within the blanking period
 - htotal,vtotal are big enough

I have not checked all the drivers to see if the modes the generate
might violate these constraints. I'm hoping not, because that would mean
either abandoning the idea of doing this from the core code, or fixing
the drivers.

I'm not entirely sure about limiting the sync pulse to the blanking
period. Intel hardware doesn't support such things, but some other
hardware might. However at least HDMI doesn't allow having sync pulse
edges within the active period, so I'm thinking the check is probably
OK to have in the common code.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c        | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c |  5 ++++-
 include/drm/drm_modes.h            |  1 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 61a4a9a..543f8c6 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -906,6 +906,38 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
 
 /**
+ * drm_mode_validate_basic - make sure the mode is somewhat sane
+ * @mode: mode to check
+ *
+ * Check that the mode timings are at least somewhat reasonable.
+ * Any hardware specific limits are left up for each driver to check.
+ *
+ * Returns:
+ * The mode status
+ */
+enum drm_mode_status
+drm_mode_validate_basic(const struct drm_display_mode *mode)
+{
+	if (mode->clock == 0)
+		return MODE_CLOCK_LOW;
+
+	if (mode->hdisplay == 0 ||
+	    mode->hsync_start < mode->hdisplay ||
+	    mode->hsync_end < mode->hsync_start ||
+	    mode->htotal < mode->hsync_end)
+		return MODE_H_ILLEGAL;
+
+	if (mode->vdisplay == 0 ||
+	    mode->vsync_start < mode->vdisplay ||
+	    mode->vsync_end < mode->vsync_start ||
+	    mode->vtotal < mode->vsync_end)
+		return MODE_V_ILLEGAL;
+
+	return MODE_OK;
+}
+EXPORT_SYMBOL(drm_mode_validate_basic);
+
+/**
  * drm_mode_validate_size - make sure modes adhere to size constraints
  * @mode: mode to check
  * @maxX: maximum width
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index c637bd9..0b86ab7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -166,7 +166,10 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
 		mode_flags |= DRM_MODE_FLAG_3D_MASK;
 
 	list_for_each_entry(mode, &connector->modes, head) {
-		mode->status = drm_mode_validate_size(mode, maxX, maxY);
+		mode->status = drm_mode_validate_basic(mode);
+
+		if (mode->status == MODE_OK)
+			mode->status = drm_mode_validate_size(mode, maxX, maxY);
 
 		if (mode->status == MODE_OK)
 			mode->status = drm_mode_validate_flag(mode, mode_flags);
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 6c53114..a36a5bf 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -217,6 +217,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
 					const struct drm_display_mode *mode2);
 
 /* for use by the crtc helper probe functions */
+enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode);
 enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
 					    int maxX, int maxY);
 void drm_mode_prune_invalid(struct drm_device *dev,
-- 
2.0.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm: Do basic sanity checks for user modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently userspace is allowed to pass in basiclly any kind of garbage
to setcrtc. Try to catch the cases where the timings make no sense
by passing the mode through drm_mode_validate_basic().

One concern here is that we now start to block some modes that have
worked in the past. It's at least possible with when using i915 with
LVDS/eDP. Previously we've just ignored everything but hdisplay/vdisplay
from the user mode and just overwritten the rest with the panel fixed
mode. So if someone has been passing a mode with just those populated
that would now stop working. If that is a real problem, we can't add
these checks to the core code and each driver would have to have its
own sanity checks. So fingers crossed...

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e79c8d3..9a1d0e8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2612,6 +2612,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
+		mode->status = drm_mode_validate_basic(mode);
+		if (mode->status != MODE_OK) {
+			ret = -EINVAL;
+			goto out;
+		}
+
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
 
 		ret = drm_crtc_check_viewport(crtc, crtc_req->x, crtc_req->y,
-- 
2.0.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Basic mode sanity checks

[Alex Deucher]

On Wed, Dec 17, 2014 at 6:56 AM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We had a bug in i915 land recently where X passed in a zeroed mode with
> mode_valid=1 to setcrtc. That didn't go down so well and caused a
> div-by-zero in i915.
>
> For a long time I've been thinking that we need some real checks to
> validate the modes we feed into the hardware. I started to sketch out
> something for that but it quickly turned into a bit of a nightmare
> with the whole panel fitter, stereo 3D, SDVO etc. special cases we
> have in i915.
>
> So I gave up on that for now, and instead cooked up this small series
> to add some basic sanity checks to the mode validation, and also apply
> the same sanity checks to user provided modes. This is enough to
> prevent the div-by-zero in i915 with buggy X.
>
> The risk is that we start to reject some modes that more or less worked
> by accident before. Given how lax we've been in handling the panel fitter
> on i915 for instance, this could be a real problem if people have been
> useing custom modes that have been filled out only partially. But I'm
> hoping the number of users doing that is so small that we can risk it.
>
> The other concern is drivers which might also provide funky modes from
> their .get_modes(). I tried to look at all the drivers a bit, and most
> produce modes via EDID, CVT, DMT, or drm_display_mode_from_videomode().
> All of those look safe, except mode->clock might be an issue with
> drm_display_mode_from_videomode(), but hopefully there's some kind of
> clock provided in most cases. I didn't dig through all the nooks and
> crannies in all drivers though, so may have missed something.

For the series:

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Ville Syrjälä (3):
>   drm: Reorganize probed mode validation
>   drm: Perform basic sanity checks on probed modes
>   drm: Do basic sanity checks for user modes
>
>  drivers/gpu/drm/drm_crtc.c         |  6 ++++
>  drivers/gpu/drm/drm_modes.c        | 56 ++++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/drm_probe_helper.c | 43 ++++++++++++++---------------
>  include/drm/drm_modes.h            |  6 ++--
>  4 files changed, 74 insertions(+), 37 deletions(-)
>
> --
> 2.0.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Basic mode sanity checks

[Daniel Vetter]

On Wed, Dec 17, 2014 at 12:09:47PM -0500, Alex Deucher wrote:
> On Wed, Dec 17, 2014 at 6:56 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We had a bug in i915 land recently where X passed in a zeroed mode with
> > mode_valid=1 to setcrtc. That didn't go down so well and caused a
> > div-by-zero in i915.
> >
> > For a long time I've been thinking that we need some real checks to
> > validate the modes we feed into the hardware. I started to sketch out
> > something for that but it quickly turned into a bit of a nightmare
> > with the whole panel fitter, stereo 3D, SDVO etc. special cases we
> > have in i915.
> >
> > So I gave up on that for now, and instead cooked up this small series
> > to add some basic sanity checks to the mode validation, and also apply
> > the same sanity checks to user provided modes. This is enough to
> > prevent the div-by-zero in i915 with buggy X.
> >
> > The risk is that we start to reject some modes that more or less worked
> > by accident before. Given how lax we've been in handling the panel fitter
> > on i915 for instance, this could be a real problem if people have been
> > useing custom modes that have been filled out only partially. But I'm
> > hoping the number of users doing that is so small that we can risk it.
> >
> > The other concern is drivers which might also provide funky modes from
> > their .get_modes(). I tried to look at all the drivers a bit, and most
> > produce modes via EDID, CVT, DMT, or drm_display_mode_from_videomode().
> > All of those look safe, except mode->clock might be an issue with
> > drm_display_mode_from_videomode(), but hopefully there's some kind of
> > clock provided in most cases. I didn't dig through all the nooks and
> > crannies in all drivers though, so may have missed something.
> 
> For the series:
> 
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

All pulled into my drm misc branch.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Basic mode sanity checks

[Adam Jackson]

On Wed, 2014-12-17 at 18:30 +0100, Daniel Vetter wrote:

> All pulled into my drm misc branch.

Ugh, wish I'd caught this earlier.  I'm not entirely thrilled with
requiring non-zero clock, it's not a concept that makes sense on virtual
hardware.

- ajax

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Basic mode sanity checks

[Ville Syrjälä]

On Fri, Dec 19, 2014 at 10:46:02AM -0500, Adam Jackson wrote:
> On Wed, 2014-12-17 at 18:30 +0100, Daniel Vetter wrote:
> 
> > All pulled into my drm misc branch.
> 
> Ugh, wish I'd caught this earlier.  I'm not entirely thrilled with
> requiring non-zero clock, it's not a concept that makes sense on virtual
> hardware.

I'm thinking none of the timing information makes much sense in such
cases. Well, except hdisplay/vdisplay but that's only because our API
sucks a bit and doesn't have the concept of crtc input vs. output size.
hdisplay/vdisplay are used to mean one or the other, depending on the
driver/hardware/output.

That said, I could be convinced to drop the clock check if you think it
can cause problems in the common code.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel