[PATCH] drm: check for garbage in unused addfb2 fields

[PATCH] drm: check for garbage in unused addfb2 fields

[Daniel Vetter]

Unfortunately old userspace didn't clear this properly, but since
we've added fb modifiers that's fixed. Checking properly that unused
fields is important for abi extensions, and just right now there's a
bunch of discussions going on about how exactly the additional aux
planes for render compression should be specified.

So let's first make sure that the values in those additional fields
can be indeed used.

Cc: Thierry Reding <thierry.reding@gmail.com>
Testcase: igt/kms_addfb/unused-*
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 2e26988a9762..8ad582f4026b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3308,6 +3308,32 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 		}
 	}
 
+	for (i = num_planes; i < 4; i++) {
+		if (r->modifier[i]) {
+			DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i);
+			return -EINVAL;
+		}
+
+		/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */
+		if (!(r->flags & DRM_MODE_FB_MODIFIERS))
+			continue;
+
+		if (r->handles[i]) {
+			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
+			return -EINVAL;
+		}
+
+		if (r->pitches[i]) {
+			DRM_DEBUG_KMS("non-zero pitch for unused plane %d\n", i);
+			return -EINVAL;
+		}
+
+		if (r->offsets[i]) {
+			DRM_DEBUG_KMS("non-zero offset for unused plane %d\n", i);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.1.4

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

Re: [PATCH] drm: check for garbage in unused addfb2 fields

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6444
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  234/234              234/234
ILK                                  262/262              262/262
SNB                 -1              282/282              281/282
IVB                                  300/300              300/300
BYT                                  254/254              254/254
BDW                                  275/275              275/275
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(17)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
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: check for garbage in unused addfb2 fields

[Thierry Reding]

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

On Wed, May 20, 2015 at 04:53:53PM +0200, Daniel Vetter wrote:
> Unfortunately old userspace didn't clear this properly, but since
> we've added fb modifiers that's fixed. Checking properly that unused
> fields is important for abi extensions, and just right now there's a
> bunch of discussions going on about how exactly the additional aux
> planes for render compression should be specified.
> 
> So let's first make sure that the values in those additional fields
> can be indeed used.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Testcase: igt/kms_addfb/unused-*
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

I think this is fine for now. There's still some unclarity about how
exactly to pass in auxiliary planes, but in order to do so we need to
prevent userspace from passing in garbage, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 2e26988a9762..8ad582f4026b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3308,6 +3308,32 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  		}
>  	}
>  
> +	for (i = num_planes; i < 4; i++) {
> +		if (r->modifier[i]) {
> +			DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i);
> +			return -EINVAL;
> +		}
> +
> +		/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */
> +		if (!(r->flags & DRM_MODE_FB_MODIFIERS))
> +			continue;
> +
> +		if (r->handles[i]) {
> +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
> +			return -EINVAL;
> +		}
> +
> +		if (r->pitches[i]) {
> +			DRM_DEBUG_KMS("non-zero pitch for unused plane %d\n", i);
> +			return -EINVAL;
> +		}
> +
> +		if (r->offsets[i]) {
> +			DRM_DEBUG_KMS("non-zero offset for unused plane %d\n", i);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.4
> 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 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] drm: check for garbage in unused addfb2 fields

[Daniel Vetter]

On Tue, May 26, 2015 at 02:38:44PM +0200, Thierry Reding wrote:
> On Wed, May 20, 2015 at 04:53:53PM +0200, Daniel Vetter wrote:
> > Unfortunately old userspace didn't clear this properly, but since
> > we've added fb modifiers that's fixed. Checking properly that unused
> > fields is important for abi extensions, and just right now there's a
> > bunch of discussions going on about how exactly the additional aux
> > planes for render compression should be specified.
> > 
> > So let's first make sure that the values in those additional fields
> > can be indeed used.
> > 
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Testcase: igt/kms_addfb/unused-*
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> 
> I think this is fine for now. There's still some unclarity about how
> exactly to pass in auxiliary planes, but in order to do so we need to
> prevent userspace from passing in garbage, so:
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>

Thanks for the review, patch merged.
-Daniel

> 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 2e26988a9762..8ad582f4026b 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3308,6 +3308,32 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >  		}
> >  	}
> >  
> > +	for (i = num_planes; i < 4; i++) {
> > +		if (r->modifier[i]) {
> > +			DRM_DEBUG_KMS("non-zero modifier for unused plane %d\n", i);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */
> > +		if (!(r->flags & DRM_MODE_FB_MODIFIERS))
> > +			continue;
> > +
> > +		if (r->handles[i]) {
> > +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (r->pitches[i]) {
> > +			DRM_DEBUG_KMS("non-zero pitch for unused plane %d\n", i);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (r->offsets[i]) {
> > +			DRM_DEBUG_KMS("non-zero offset for unused plane %d\n", i);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.1.4
> > 



-- 
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