drm/damage_helper: Check if damage clips has valid values

Commit Message

Souza, Jose Dec. 13, 2020, 5:07 p.m. UTC

Userspace can set a damage clip with a negative coordinate, negative
width or height or larger than the plane.
This invalid values could cause issues in some HW or even worst enable
security flaws.

Cc: Gwan-gyeong Mun <[e-mail]>
Cc: Sean Paul <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Deepak Rawat <[e-mail]>
Cc: [e-mail]
Signed-off-by: José Roberto de Souza <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++-
 drivers/gpu/drm/drm_damage_helper.c | 49 +++++++++++++++++++++++------
 include/drm/drm_damage_helper.h     |  4 +--
 3 files changed, 45 insertions(+), 12 deletions(-)

Comments

Simon Ser Dec. 13, 2020, 5:22 p.m. UTC | #1

Can you add some drm_dbg_atomic logs when the damage is invalid, to make it
easier for user-space to understand why an atomic commit failed?

Souza, Jose Dec. 13, 2020, 5:36 p.m. UTC | #2

On Sun, 2020-12-13 at 17:22 +0000, Simon Ser wrote:
> Can you add some drm_dbg_atomic logs when the damage is invalid, to make it
> easier for user-space to understand why an atomic commit failed?

sure, this is enough? will wait for a couple of more days before send another version.


diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 9adb369440ba..b598b137d27f 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -33,6 +33,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_device.h>
+#include <drm/drm_print.h>

 /**
  * DOC: overview
@@ -152,16 +153,25 @@ int drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,

        for (; num_clips; num_clips--, damaged_clips++) {
                if (damaged_clips->x1 < 0 || damaged_clips->x2 < 0 ||
-                   damaged_clips->y1 < 0 || damaged_clips->y2 < 0)
+                   damaged_clips->y1 < 0 || damaged_clips->y2 < 0) {
+                       drm_dbg_atomic(state->dev,
+                                      "Invalid damage clip, negative coordinate\n");
                        return -EINVAL;
+               }

                if (damaged_clips->x2 < damaged_clips->x1 ||
-                   damaged_clips->y2 < damaged_clips->y1)
+                   damaged_clips->y2 < damaged_clips->y1) {
+                       drm_dbg_atomic(state->dev,
+                                      "Invalid damage clip, negative width or height\n");
                        return -EINVAL;
+               }

                if ((damaged_clips->x2 - damaged_clips->x1) > w ||
-                   (damaged_clips->y2 - damaged_clips->y1) > h)
+                   (damaged_clips->y2 - damaged_clips->y1) > h) {
+                       drm_dbg_atomic(state->dev,
+                                      "Invalid damage clip, width or height larger than plane\n");
                        return -EINVAL;
+               }
        }

        return 0;

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ba1507036f26..c6b341ecae2c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -897,7 +897,9 @@  drm_atomic_helper_check_planes(struct drm_device *dev,
 
 		drm_atomic_helper_plane_changed(state, old_plane_state, new_plane_state, plane);
 
-		drm_atomic_helper_check_plane_damage(state, new_plane_state);
+		ret = drm_atomic_helper_check_plane_damage(state, new_plane_state);
+		if (ret)
+			return ret;
 
 		if (!funcs || !funcs->atomic_check)
 			continue;
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 3a4126dc2520..9adb369440ba 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -104,36 +104,67 @@  void drm_plane_enable_fb_damage_clips(struct drm_plane *plane)
 EXPORT_SYMBOL(drm_plane_enable_fb_damage_clips);
 
 /**
- * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check.
+ * drm_atomic_helper_check_plane_damage - Verify plane damage clips on
+ * atomic_check.
  * @state: The driver state object.
- * @plane_state: Plane state for which to verify damage.
+ * @plane_state: Plane state for which to verify damage clips.
  *
- * This helper function makes sure that damage from plane state is discarded
- * for full modeset. If there are more reasons a driver would want to do a full
- * plane update rather than processing individual damage regions, then those
- * cases should be taken care of here.
+ * This helper checks if all damage clips has valid values and makes sure that
+ * damage clips from plane state is discarded for full modeset. If there are
+ * more reasons a driver would want to do a full plane update rather than
+ * processing individual damage regions, then those cases should be taken care
+ * of here.
  *
  * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that
  * full plane update should happen. It also ensure helper iterator will return
  * &drm_plane_state.src as damage.
+ *
+ * Return: Zero on success, negative errno on failure.
  */
-void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
-					  struct drm_plane_state *plane_state)
+int drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
+					 struct drm_plane_state *plane_state)
 {
+	struct drm_mode_rect *damaged_clips;
 	struct drm_crtc_state *crtc_state;
+	unsigned int num_clips, w, h;
+
+	num_clips = drm_plane_get_damage_clips_count(plane_state);
+	if (!num_clips)
+		return 0;
 
 	if (plane_state->crtc) {
 		crtc_state = drm_atomic_get_new_crtc_state(state,
 							   plane_state->crtc);
 
 		if (WARN_ON(!crtc_state))
-			return;
+			return 0;
 
 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 			drm_property_blob_put(plane_state->fb_damage_clips);
 			plane_state->fb_damage_clips = NULL;
+			return 0;
 		}
 	}
+
+	w = drm_rect_width(&plane_state->src) >> 16;
+	h = drm_rect_height(&plane_state->src) >> 16;
+	damaged_clips = drm_plane_get_damage_clips(plane_state);
+
+	for (; num_clips; num_clips--, damaged_clips++) {
+		if (damaged_clips->x1 < 0 || damaged_clips->x2 < 0 ||
+		    damaged_clips->y1 < 0 || damaged_clips->y2 < 0)
+			return -EINVAL;
+
+		if (damaged_clips->x2 < damaged_clips->x1 ||
+		    damaged_clips->y2 < damaged_clips->y1)
+			return -EINVAL;
+
+		if ((damaged_clips->x2 - damaged_clips->x1) > w ||
+		    (damaged_clips->y2 - damaged_clips->y1) > h)
+			return -EINVAL;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
 
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index 40c34a5bf149..5e344d1a2b22 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -65,8 +65,8 @@  struct drm_atomic_helper_damage_iter {
 };
 
 void drm_plane_enable_fb_damage_clips(struct drm_plane *plane);
-void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
-					  struct drm_plane_state *plane_state);
+int drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
+					 struct drm_plane_state *plane_state);
 int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 			      struct drm_file *file_priv, unsigned int flags,
 			      unsigned int color, struct drm_clip_rect *clips,