[PATCH] drm: Add integer overflow checking to transitional plane helpers

[PATCH] drm: Add integer overflow checking to transitional plane helpers

[Matt Roper]

Add tests for destination rectangle integer overflow before calling the
driver's check function.  This will ensure that the transitional plane
helpers match the behavior of the full atomic helpers by always
returning -ERANGE for planes positioned beyond INT_MAX.

Note that the legacy SetPlane ioctl itself also includes similar tests
for integer overflow, so the only case where this check really matters
is when legacy cursor operations get routed through the universal plane
interface internally.

This issue was first noticed with i915 commit:

        commit ff42e093e9c9c17a6e1d6aab24875a36795f926e
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Mon Mar 2 16:35:20 2015 +0100

            Revert "drm/i915: Switch planes from transitional helpers to full
            atomic helpers"

The above revert switched us from full atomic helpers back to the
transitional helpers, and in doing so we lost the overflow checking here
for universal cursor updates.  Even though such extreme cursor positions
are unlikely to actually happen in the wild, we still don't want there
to be a change of behavior when drivers switch from transitional helpers
to full helpers.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 33807e0..1e9e105 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -417,6 +417,20 @@ int drm_plane_helper_commit(struct drm_plane *plane,
 	for (i = 0; i < 2; i++)
 		crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL;
 
+	/*
+	 * Give drivers some help against integer overflows (and match the
+	 * behavior of the full atomic helpers).
+	 */
+	if (plane_state->crtc_w > INT_MAX ||
+	    plane_state->crtc_x > INT_MAX - (int32_t) plane_state->crtc_w ||
+	    plane_state->crtc_h > INT_MAX ||
+	    plane_state->crtc_y > INT_MAX - (int32_t) plane_state->crtc_h) {
+		DRM_DEBUG_ATOMIC("Invalid CRTC coordinates %ux%u+%d+%d\n",
+				 plane_state->crtc_w, plane_state->crtc_h,
+				 plane_state->crtc_x, plane_state->crtc_y);
+		return -ERANGE;
+	}
+
 	if (plane_funcs->atomic_check) {
 		ret = plane_funcs->atomic_check(plane, plane_state);
 		if (ret)
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add integer overflow checking to transitional plane helpers

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6130
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              272/272              270/272
ILK                                  302/302              302/302
SNB                                  303/303              303/303
IVB                                  338/338              338/338
BYT                 -1              287/287              286/287
HSW                                  361/361              361/361
BDW                                  308/308              308/308
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gem_tiled_pread_pwrite      FAIL(3)PASS(11)      FAIL(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(5)PASS(8)      CRASH(1)PASS(1)
*BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(18)      FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add integer overflow checking to transitional plane helpers

[Daniel Vetter]

On Fri, Apr 03, 2015 at 02:27:46PM -0700, Matt Roper wrote:
> Add tests for destination rectangle integer overflow before calling the
> driver's check function.  This will ensure that the transitional plane
> helpers match the behavior of the full atomic helpers by always
> returning -ERANGE for planes positioned beyond INT_MAX.
> 
> Note that the legacy SetPlane ioctl itself also includes similar tests
> for integer overflow, so the only case where this check really matters
> is when legacy cursor operations get routed through the universal plane
> interface internally.
> 
> This issue was first noticed with i915 commit:
> 
>         commit ff42e093e9c9c17a6e1d6aab24875a36795f926e
>         Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>         Date:   Mon Mar 2 16:35:20 2015 +0100
> 
>             Revert "drm/i915: Switch planes from transitional helpers to full
>             atomic helpers"
> 
> The above revert switched us from full atomic helpers back to the
> transitional helpers, and in doing so we lost the overflow checking here
> for universal cursor updates.  Even though such extreme cursor positions
> are unlikely to actually happen in the wild, we still don't want there
> to be a change of behavior when drivers switch from transitional helpers
> to full helpers.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Since this is just for the cursor ioctl ... shouldn't we instead move
these checks from drm_mode_setplane to __setplane_internal? That way other
drivers also can rely upon these guarantees when implementing universal
cursor support and we won't have a mismatch in what kind of plane
properties can sneak through to drivers.
-Daniel

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 33807e0..1e9e105 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -417,6 +417,20 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>  	for (i = 0; i < 2; i++)
>  		crtc_funcs[i] = crtc[i] ? crtc[i]->helper_private : NULL;
>  
> +	/*
> +	 * Give drivers some help against integer overflows (and match the
> +	 * behavior of the full atomic helpers).
> +	 */
> +	if (plane_state->crtc_w > INT_MAX ||
> +	    plane_state->crtc_x > INT_MAX - (int32_t) plane_state->crtc_w ||
> +	    plane_state->crtc_h > INT_MAX ||
> +	    plane_state->crtc_y > INT_MAX - (int32_t) plane_state->crtc_h) {
> +		DRM_DEBUG_ATOMIC("Invalid CRTC coordinates %ux%u+%d+%d\n",
> +				 plane_state->crtc_w, plane_state->crtc_h,
> +				 plane_state->crtc_x, plane_state->crtc_y);
> +		return -ERANGE;
> +	}
> +
>  	if (plane_funcs->atomic_check) {
>  		ret = plane_funcs->atomic_check(plane, plane_state);
>  		if (ret)
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Make integer overflow checking cover universal cursor updates (v2)

[Matt Roper]

Our legacy SetPlane updates perform integer overflow checking on a
plane's destination rectangle in drm_mode_setplane(), and atomic updates
handled as part of a drm_atomic_state transaction do the same checking
in drm_atomic_plane_check().  However legacy cursor updates that get
routed through universal plane interfaces may bypass this overflow
checking if the driver's .update_plane is serviced by the transitional
plane helpers rather than the full atomic plane helpers.

Move the check for destination rectangle integer overflow from the
drm_mode_setplane() to __setplane_internal() so that it also covers
cursor operations.

This fixes an issue first noticed with i915 commit:

        commit ff42e093e9c9c17a6e1d6aab24875a36795f926e
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Mon Mar 2 16:35:20 2015 +0100

            Revert "drm/i915: Switch planes from transitional helpers to full
            atomic helpers"

The above revert switched us from full atomic helpers back to the
transitional helpers, and in doing so we lost the overflow checking here
for universal cursor updates.  Even though such extreme cursor positions
are unlikely to actually happen in the wild, we still don't want there
to be a change of behavior when drivers switch from transitional helpers
to full helpers.

v2: Move check from setplane ioctl to setplane_internal rather than
    adding an additional copy of the checks to the transitional plane
    helpers.  (Daniel)

Cc: Daniel Vetter <daniel@ffwll.ch>
Testcase: igt/kms_cursor_crc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b914882..160647a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2507,6 +2507,17 @@ static int __setplane_internal(struct drm_plane *plane,
 		goto out;
 	}
 
+	/* Give drivers some help against integer overflows */
+	if (crtc_w > INT_MAX ||
+	    crtc_x > INT_MAX - (int32_t) crtc_w ||
+	    crtc_h > INT_MAX ||
+	    crtc_y > INT_MAX - (int32_t) crtc_h) {
+		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
+			      crtc_w, crtc_h, crtc_x, crtc_y);
+		return -ERANGE;
+	}
+
+
 	fb_width = fb->width << 16;
 	fb_height = fb->height << 16;
 
@@ -2591,17 +2602,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	/* Give drivers some help against integer overflows */
-	if (plane_req->crtc_w > INT_MAX ||
-	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
-	    plane_req->crtc_h > INT_MAX ||
-	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
-		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      plane_req->crtc_w, plane_req->crtc_h,
-			      plane_req->crtc_x, plane_req->crtc_y);
-		return -ERANGE;
-	}
-
 	/*
 	 * First, find the plane, crtc, and fb objects.  If not available,
 	 * we don't bother to call the driver.
-- 
1.8.5.1

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

Re: [PATCH] drm: Make integer overflow checking cover universal cursor updates (v2)

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6187
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -8              276/276              268/276
ILK                                  301/301              301/301
SNB                 -22              316/316              294/316
IVB                 -1              328/328              327/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt@gem_tiled_pread_pwrite      PASS(4)      FAIL(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(4)      CRASH(2)
 PNV  igt@gem_userptr_blits@coherency-unsync      CRASH(2)PASS(5)      CRASH(2)
*PNV  igt@gem_userptr_blits@forked-sync-swapping-mempressure-interruptible      PASS(2)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_linear_blits      FAIL(4)PASS(8)      FAIL(2)
 PNV  igt@gen3_render_mixed_blits      FAIL(5)PASS(7)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(5)PASS(8)      FAIL(2)
 PNV  igt@gen3_render_tiledy_blits      FAIL(4)PASS(7)      FAIL(2)
 SNB  igt@kms_cursor_crc@cursor-size-change      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_flip_event_leak      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@primary-rotation      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@sprite-rotation      NSPT(3)PASS(3)      NSPT(2)
 SNB  igt@pm_rpm@cursor      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@cursor-dpms      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-non-lpsp      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@drm-resources-equal      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences-dpms      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-execbuf      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-cpu      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-gtt      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-pread      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@i2c      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp-stress-no-wait      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@pci-d3-state      NSPT(3)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@rte      NSPT(3)PASS(1)      NSPT(2)
 IVB  igt@gem_pwrite_pread@uncached-copy-performance      DMESG_WARN(1)PASS(8)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Make integer overflow checking cover universal cursor updates (v2)

[Daniel Vetter]

On Mon, Apr 13, 2015 at 11:06:13AM -0700, Matt Roper wrote:
> Our legacy SetPlane updates perform integer overflow checking on a
> plane's destination rectangle in drm_mode_setplane(), and atomic updates
> handled as part of a drm_atomic_state transaction do the same checking
> in drm_atomic_plane_check().  However legacy cursor updates that get
> routed through universal plane interfaces may bypass this overflow
> checking if the driver's .update_plane is serviced by the transitional
> plane helpers rather than the full atomic plane helpers.
> 
> Move the check for destination rectangle integer overflow from the
> drm_mode_setplane() to __setplane_internal() so that it also covers
> cursor operations.
> 
> This fixes an issue first noticed with i915 commit:
> 
>         commit ff42e093e9c9c17a6e1d6aab24875a36795f926e
>         Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>         Date:   Mon Mar 2 16:35:20 2015 +0100
> 
>             Revert "drm/i915: Switch planes from transitional helpers to full
>             atomic helpers"
> 
> The above revert switched us from full atomic helpers back to the
> transitional helpers, and in doing so we lost the overflow checking here
> for universal cursor updates.  Even though such extreme cursor positions
> are unlikely to actually happen in the wild, we still don't want there
> to be a change of behavior when drivers switch from transitional helpers
> to full helpers.
> 
> v2: Move check from setplane ioctl to setplane_internal rather than
>     adding an additional copy of the checks to the transitional plane
>     helpers.  (Daniel)
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Testcase: igt/kms_cursor_crc
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b914882..160647a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2507,6 +2507,17 @@ static int __setplane_internal(struct drm_plane *plane,
>  		goto out;
>  	}
>  
> +	/* Give drivers some help against integer overflows */
> +	if (crtc_w > INT_MAX ||
> +	    crtc_x > INT_MAX - (int32_t) crtc_w ||
> +	    crtc_h > INT_MAX ||
> +	    crtc_y > INT_MAX - (int32_t) crtc_h) {
> +		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> +			      crtc_w, crtc_h, crtc_x, crtc_y);
> +		return -ERANGE;
> +	}
> +
> +
>  	fb_width = fb->width << 16;
>  	fb_height = fb->height << 16;
>  
> @@ -2591,17 +2602,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
>  
> -	/* Give drivers some help against integer overflows */
> -	if (plane_req->crtc_w > INT_MAX ||
> -	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
> -	    plane_req->crtc_h > INT_MAX ||
> -	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
> -		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> -			      plane_req->crtc_w, plane_req->crtc_h,
> -			      plane_req->crtc_x, plane_req->crtc_y);
> -		return -ERANGE;
> -	}
> -
>  	/*
>  	 * First, find the plane, crtc, and fb objects.  If not available,
>  	 * we don't bother to call the driver.
> -- 
> 1.8.5.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx