drm plane fixes and additional checks

drm plane fixes and additional checks

[ville.syrjala]

This set has a bunch of fixes and additional checks on top of Jesse's
drm plane series.

Jesse, feel free to squash some or all of these with your patches.

[PATCH 1/7] drm: Zero initialize drm_mode_fb_cmd2 in legacy addfb ioctl handler

[ville.syrjala]

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

Avoids having junk in offsets[] and pitches[] arrays.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 39cccb4..7058ed3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1967,7 +1967,7 @@ int drm_mode_addfb(struct drm_device *dev,
 		   void *data, struct drm_file *file_priv)
 {
 	struct drm_mode_fb_cmd *or = data;
-	struct drm_mode_fb_cmd2 r;
+	struct drm_mode_fb_cmd2 r = {};
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_framebuffer *fb;
 	int ret = 0;
-- 
1.7.3.4

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

Re: [PATCH 1/7] drm: Zero initialize drm_mode_fb_cmd2 in legacy addfb ioctl handler

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]

On Fri, 11 Nov 2011 18:04:01 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Avoids having junk in offsets[] and pitches[] arrays.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 39cccb4..7058ed3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1967,7 +1967,7 @@ int drm_mode_addfb(struct drm_device *dev,
>  		   void *data, struct drm_file *file_priv)
>  {
>  	struct drm_mode_fb_cmd *or = data;
> -	struct drm_mode_fb_cmd2 r;
> +	struct drm_mode_fb_cmd2 r = {};
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_framebuffer *fb;
>  	int ret = 0;

Yep, doesn't hurt...

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 2/7] drm: plane: Fix size of formats memcpy()

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 7058ed3..d3b884e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -563,7 +563,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		return -ENOMEM;
 	}
 
-	memcpy(plane->format_types, formats, format_count);
+	memcpy(plane->format_types, formats, sizeof(uint32_t) * format_count);
 	plane->format_count = format_count;
 	plane->possible_crtcs = possible_crtcs;
 
-- 
1.7.3.4

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

Re: [PATCH 2/7] drm: plane: Fix size of formats memcpy()

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 906 bytes --]

On Fri, 11 Nov 2011 18:04:02 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 7058ed3..d3b884e 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -563,7 +563,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> -	memcpy(plane->format_types, formats, format_count);
> +	memcpy(plane->format_types, formats, sizeof(uint32_t) * format_count);
>  	plane->format_count = format_count;
>  	plane->possible_crtcs = possible_crtcs;
>  

Duh thank you.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 3/7] drm: plane: Clear plane.crtc and plane.fb after disable_plane()

[ville.syrjala]

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

These are the only indication to user space that the plane was disabled.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d3b884e..70f5747 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1677,6 +1677,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	/* No fb means shut it down */
 	if (!plane_req->fb_id) {
 		plane->funcs->disable_plane(plane);
+		plane->crtc = NULL;
+		plane->fb = NULL;
 		goto out;
 	}
 
-- 
1.7.3.4

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

Re: [PATCH 3/7] drm: plane: Clear plane.crtc and plane.fb after disable_plane()

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1034 bytes --]

On Fri, 11 Nov 2011 18:04:03 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> These are the only indication to user space that the plane was disabled.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d3b884e..70f5747 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1677,6 +1677,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	/* No fb means shut it down */
>  	if (!plane_req->fb_id) {
>  		plane->funcs->disable_plane(plane);
> +		plane->crtc = NULL;
> +		plane->fb = NULL;
>  		goto out;
>  	}
>  

A 0 return code should also indicate that the function succeeded, but
this doesn't hurt.

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 4/7] drm: plane: Check source coordinates

[ville.syrjala]

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

Make sure the source coordinates stay within the buffer.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 70f5747..098cc50 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
 	int ret = 0;
+	unsigned int fb_width, fb_height;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	}
 	fb = obj_to_fb(obj);
 
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
+
+	/* Make sure source coordinates are inside the fb. */
+	if (plane_req->src_w > fb_width ||
+	    plane_req->src_x > fb_width - plane_req->src_w ||
+	    plane_req->src_h > fb_height ||
+	    plane_req->src_y > fb_height - plane_req->src_h) {
+		DRM_DEBUG_KMS("Invalid source coordinates "
+			      "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%06u\n",
+			      plane_req->src_w >> 16,
+			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
+			      plane_req->src_h >> 16,
+			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
+			      plane_req->src_x >> 16,
+			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
+			      plane_req->src_y >> 16,
+			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 plane_req->crtc_x, plane_req->crtc_y,
 					 plane_req->crtc_w, plane_req->crtc_h,
-- 
1.7.3.4

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

Re: [PATCH 4/7] drm: plane: Check source coordinates

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2237 bytes --]

On Fri, 11 Nov 2011 18:04:04 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make sure the source coordinates stay within the buffer.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 70f5747..098cc50 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb;
>  	int ret = 0;
> +	unsigned int fb_width, fb_height;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	}
>  	fb = obj_to_fb(obj);
>  
> +	fb_width = fb->width << 16;
> +	fb_height = fb->height << 16;
> +
> +	/* Make sure source coordinates are inside the fb. */
> +	if (plane_req->src_w > fb_width ||
> +	    plane_req->src_x > fb_width - plane_req->src_w ||
> +	    plane_req->src_h > fb_height ||
> +	    plane_req->src_y > fb_height - plane_req->src_h) {
> +		DRM_DEBUG_KMS("Invalid source coordinates "
> +			      "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%06u\n",
> +			      plane_req->src_w >> 16,
> +			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
> +			      plane_req->src_h >> 16,
> +			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
> +			      plane_req->src_x >> 16,
> +			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
> +			      plane_req->src_y >> 16,
> +			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	ret = plane->funcs->update_plane(plane, crtc, fb,
>  					 plane_req->crtc_x, plane_req->crtc_y,
>  					 plane_req->crtc_w, plane_req->crtc_h,

Good sanity check (saves the drivers from having to do it), but I
wonder if we can use a better return value like ENOSPC or something to
make it easier for userspace to figure out.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/7] drm: plane: Check source coordinates

[Ville Syrjälä]

On Fri, Nov 11, 2011 at 08:24:18AM -0800, Jesse Barnes wrote:
> On Fri, 11 Nov 2011 18:04:04 +0200
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make sure the source coordinates stay within the buffer.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c |   23 +++++++++++++++++++++++
> >  1 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 70f5747..098cc50 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb;
> >  	int ret = 0;
> > +	unsigned int fb_width, fb_height;
> >  
> >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  		return -EINVAL;
> > @@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> >  	}
> >  	fb = obj_to_fb(obj);
> >  
> > +	fb_width = fb->width << 16;
> > +	fb_height = fb->height << 16;
> > +
> > +	/* Make sure source coordinates are inside the fb. */
> > +	if (plane_req->src_w > fb_width ||
> > +	    plane_req->src_x > fb_width - plane_req->src_w ||
> > +	    plane_req->src_h > fb_height ||
> > +	    plane_req->src_y > fb_height - plane_req->src_h) {
> > +		DRM_DEBUG_KMS("Invalid source coordinates "
> > +			      "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%06u\n",
> > +			      plane_req->src_w >> 16,
> > +			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
> > +			      plane_req->src_h >> 16,
> > +			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
> > +			      plane_req->src_x >> 16,
> > +			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
> > +			      plane_req->src_y >> 16,
> > +			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> >  					 plane_req->crtc_x, plane_req->crtc_y,
> >  					 plane_req->crtc_w, plane_req->crtc_h,
> 
> Good sanity check (saves the drivers from having to do it), but I
> wonder if we can use a better return value like ENOSPC or something to
> make it easier for userspace to figure out.

Yeah, getting EINVAL for every kind of failure is rather annoying. The
only issue I have with ENOSPC is the strerror() output. It doesn't
exactly fit this use case. But if there's nothing better I'm OK with
ENOSPC.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/7] drm: plane: Check source coordinates

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2971 bytes --]

On Fri, 11 Nov 2011 19:18:52 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Nov 11, 2011 at 08:24:18AM -0800, Jesse Barnes wrote:
> > On Fri, 11 Nov 2011 18:04:04 +0200
> > ville.syrjala@linux.intel.com wrote:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Make sure the source coordinates stay within the buffer.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c |   23 +++++++++++++++++++++++
> > >  1 files changed, 23 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 70f5747..098cc50 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> > >  	struct drm_crtc *crtc;
> > >  	struct drm_framebuffer *fb;
> > >  	int ret = 0;
> > > +	unsigned int fb_width, fb_height;
> > >  
> > >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >  		return -EINVAL;
> > > @@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> > >  	}
> > >  	fb = obj_to_fb(obj);
> > >  
> > > +	fb_width = fb->width << 16;
> > > +	fb_height = fb->height << 16;
> > > +
> > > +	/* Make sure source coordinates are inside the fb. */
> > > +	if (plane_req->src_w > fb_width ||
> > > +	    plane_req->src_x > fb_width - plane_req->src_w ||
> > > +	    plane_req->src_h > fb_height ||
> > > +	    plane_req->src_y > fb_height - plane_req->src_h) {
> > > +		DRM_DEBUG_KMS("Invalid source coordinates "
> > > +			      "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%06u\n",
> > > +			      plane_req->src_w >> 16,
> > > +			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
> > > +			      plane_req->src_h >> 16,
> > > +			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
> > > +			      plane_req->src_x >> 16,
> > > +			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
> > > +			      plane_req->src_y >> 16,
> > > +			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> > >  					 plane_req->crtc_x, plane_req->crtc_y,
> > >  					 plane_req->crtc_w, plane_req->crtc_h,
> > 
> > Good sanity check (saves the drivers from having to do it), but I
> > wonder if we can use a better return value like ENOSPC or something to
> > make it easier for userspace to figure out.
> 
> Yeah, getting EINVAL for every kind of failure is rather annoying. The
> only issue I have with ENOSPC is the strerror() output. It doesn't
> exactly fit this use case. But if there's nothing better I'm OK with
> ENOSPC.

Yeah, dunno what's best.  Check out include/asm-generic/errno-base.h
and errno.h; maybe you'll find a better fit.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 5/7] drm: plane: Check crtc coodrinates against integer overflows in setplane ioctl

[ville.syrjala]

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

Help drivers a little by guaranteeing that crtc_x+crtc_w and
crtc_y+crtc_h don't overflow.

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 098cc50..2410a9a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1725,6 +1725,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	/* 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);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	ret = plane->funcs->update_plane(plane, crtc, fb,
 					 plane_req->crtc_x, plane_req->crtc_y,
 					 plane_req->crtc_w, plane_req->crtc_h,
-- 
1.7.3.4

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

Re: [PATCH 5/7] drm: plane: Check crtc coodrinates against integer overflows in setplane ioctl

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1640 bytes --]

On Fri, 11 Nov 2011 18:04:05 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Help drivers a little by guaranteeing that crtc_x+crtc_w and
> crtc_y+crtc_h don't overflow.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 098cc50..2410a9a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1725,6 +1725,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> +	/* 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);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	ret = plane->funcs->update_plane(plane, crtc, fb,
>  					 plane_req->crtc_x, plane_req->crtc_y,
>  					 plane_req->crtc_w, plane_req->crtc_h,

Not sure this helps much in practice, since the drivers will have to
validate the target CRTC rect against the actual pipe dimensions anyway.

But it doesn't hurt either, so:
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 5/7] drm: plane: Check crtc coodrinates against integer overflows in setplane ioctl

[Ville Syrjälä]

On Fri, Nov 11, 2011 at 09:01:46AM -0800, Jesse Barnes wrote:
> On Fri, 11 Nov 2011 18:04:05 +0200
> ville.syrjala@linux.intel.com wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Help drivers a little by guaranteeing that crtc_x+crtc_w and
> > crtc_y+crtc_h don't overflow.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 098cc50..2410a9a 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -1725,6 +1725,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> >  		goto out;
> >  	}
> >  
> > +	/* 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);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> >  					 plane_req->crtc_x, plane_req->crtc_y,
> >  					 plane_req->crtc_w, plane_req->crtc_h,
> 
> Not sure this helps much in practice, since the drivers will have to
> validate the target CRTC rect against the actual pipe dimensions anyway.

My master plan is that drivers would just stick these into a drm_region
(introduced in my other patchset) and clip that to the pipe dimensions.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/7] drm: plane: Check crtc coodrinates against integer overflows in setplane ioctl

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 2173 bytes --]

On Fri, 11 Nov 2011 19:11:39 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Nov 11, 2011 at 09:01:46AM -0800, Jesse Barnes wrote:
> > On Fri, 11 Nov 2011 18:04:05 +0200
> > ville.syrjala@linux.intel.com wrote:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Help drivers a little by guaranteeing that crtc_x+crtc_w and
> > > crtc_y+crtc_h don't overflow.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c |   12 ++++++++++++
> > >  1 files changed, 12 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 098cc50..2410a9a 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -1725,6 +1725,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> > >  		goto out;
> > >  	}
> > >  
> > > +	/* 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);
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > >  	ret = plane->funcs->update_plane(plane, crtc, fb,
> > >  					 plane_req->crtc_x, plane_req->crtc_y,
> > >  					 plane_req->crtc_w, plane_req->crtc_h,
> > 
> > Not sure this helps much in practice, since the drivers will have to
> > validate the target CRTC rect against the actual pipe dimensions anyway.
> 
> My master plan is that drivers would just stick these into a drm_region
> (introduced in my other patchset) and clip that to the pipe dimensions.

Ok that makes sense.  The new patchset looks really nice; having
helpers to support new chipsets and for regions should make things a
lot cleaner.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 6/7] drm: plane: Make 'formats' parameter to drm_plane_init() const

[ville.syrjala]

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

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    2 +-
 include/drm/drm_crtc.h     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2410a9a..27d46b1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -548,7 +548,7 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
 int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs,
 		   const struct drm_plane_funcs *funcs,
-		   uint32_t *formats, uint32_t format_count)
+		   const uint32_t *formats, uint32_t format_count)
 {
 	mutex_lock(&dev->mode_config.mutex);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 84db125..49dc288 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -727,7 +727,7 @@ extern int drm_plane_init(struct drm_device *dev,
 			  struct drm_plane *plane,
 			  unsigned long possible_crtcs,
 			  const struct drm_plane_funcs *funcs,
-			  uint32_t *formats, uint32_t format_count);
+			  const uint32_t *formats, uint32_t format_count);
 extern void drm_plane_cleanup(struct drm_plane *plane);
 
 extern void drm_encoder_cleanup(struct drm_encoder *encoder);
-- 
1.7.3.4

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

Re: [PATCH 6/7] drm: plane: Make 'formats' parameter to drm_plane_init() const

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1689 bytes --]

On Fri, 11 Nov 2011 18:04:06 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |    2 +-
>  include/drm/drm_crtc.h     |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 2410a9a..27d46b1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -548,7 +548,7 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
>  int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs,
>  		   const struct drm_plane_funcs *funcs,
> -		   uint32_t *formats, uint32_t format_count)
> +		   const uint32_t *formats, uint32_t format_count)
>  {
>  	mutex_lock(&dev->mode_config.mutex);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 84db125..49dc288 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -727,7 +727,7 @@ extern int drm_plane_init(struct drm_device *dev,
>  			  struct drm_plane *plane,
>  			  unsigned long possible_crtcs,
>  			  const struct drm_plane_funcs *funcs,
> -			  uint32_t *formats, uint32_t format_count);
> +			  const uint32_t *formats, uint32_t format_count);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  
>  extern void drm_encoder_cleanup(struct drm_encoder *encoder);

Yeah good fix.  I made the structure const at ickle's request (once I
stopped assigning it directly) but didn't update the prototype.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 7/7] drm: plane: Check that the fb pixel format is supported by the plane

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 27d46b1..04680bc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1655,6 +1655,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	struct drm_framebuffer *fb;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
+	int i;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1703,6 +1704,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	}
 	fb = obj_to_fb(obj);
 
+	/* Check whether this plane supports the fb pixel format. */
+	for (i = 0; i < plane->format_count; i++)
+		if (fb->pixel_format == plane->format_types[i])
+			break;
+	if (i == plane->format_count) {
+		DRM_DEBUG_KMS("Invalid pixel format %x\n", fb->pixel_format);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	fb_width = fb->width << 16;
 	fb_height = fb->height << 16;
 
-- 
1.7.3.4

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

Re: [PATCH 7/7] drm: plane: Check that the fb pixel format is supported by the plane

[Jesse Barnes]

[-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --]

On Fri, 11 Nov 2011 18:04:07 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 27d46b1..04680bc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1655,6 +1655,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	struct drm_framebuffer *fb;
>  	int ret = 0;
>  	unsigned int fb_width, fb_height;
> +	int i;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1703,6 +1704,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	}
>  	fb = obj_to_fb(obj);
>  
> +	/* Check whether this plane supports the fb pixel format. */
> +	for (i = 0; i < plane->format_count; i++)
> +		if (fb->pixel_format == plane->format_types[i])
> +			break;
> +	if (i == plane->format_count) {
> +		DRM_DEBUG_KMS("Invalid pixel format %x\n", fb->pixel_format);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	fb_width = fb->width << 16;
>  	fb_height = fb->height << 16;
>  

Yeah it's reasonable to hoist this up into generic code.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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