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

[Daniel Vetter <daniel@ffwll.ch>]

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