[PATCH 0/4] framebuffer_init checks

[PATCH 0/4] framebuffer_init checks

[Daniel Vetter]

Hi all,

That little patch turned into a bit more, and on top of it there's now also a
new testcase in igt: kms_addfb. It checks for most of the evil stuff you can
feed to addfb.

Cheers, Daniel

Daniel Vetter (4):
  drm/i915: grab dev->struct_mutex around framebuffer_init
  drm/i915: prevent tiling changes on framebuffer backing storage
  drm/i915: Use unsigned long for obj->user_pin_count
  drm/i915: check gem bo size when creating framebuffers

 drivers/gpu/drm/i915/i915_drv.h        |  5 ++++-
 drivers/gpu/drm/i915/i915_gem_tiling.c |  2 +-
 drivers/gpu/drm/i915/intel_display.c   | 33 ++++++++++++++++++++++++---------
 3 files changed, 29 insertions(+), 11 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] drm/i915: grab dev->struct_mutex around framebuffer_init

[Daniel Vetter]

We look at gem state (like obj->tiling/obj->stride), we better have
the relevant locks.

Right now this doesn't matter much since most of these checks are
a curtesy to safe buggy userspace, but I'd like to freeze the tiling
once we have framebuffer objects attached. And then locking matters.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 00271b1..70f4091 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7276,14 +7276,21 @@ intel_framebuffer_create(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		goto err;
+
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(&obj->base);
-		kfree(intel_fb);
-		return ERR_PTR(ret);
-	}
+	mutex_unlock(&dev->struct_mutex);
+	if (ret)
+		goto err;
 
 	return &intel_fb->base;
+err:
+	drm_gem_object_unreference_unlocked(&obj->base);
+	kfree(intel_fb);
+
+	return ERR_PTR(ret);
 }
 
 static u32
@@ -9981,6 +9988,8 @@ int intel_framebuffer_init(struct drm_device *dev,
 	int pitch_limit;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (obj->tiling_mode == I915_TILING_Y) {
 		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
-- 
1.8.1.4

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

[PATCH 2/4] drm/i915: prevent tiling changes on framebuffer backing storage

[Daniel Vetter]

Assuming that all framebuffer related metadata is invariant simplifies
our userspace input data checking. And current userspace always first
updates the tiling of an object before creating a framebuffer with it.

This allows us to upconvert a check in pin_and_fence to a WARN.

In the future it should also be helpful to know which buffer objects
are potential scanout targets for e.g. frontbuffer rendering tracking
and similar things.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h        | 3 +++
 drivers/gpu/drm/i915/i915_gem_tiling.c | 2 +-
 drivers/gpu/drm/i915/intel_display.c   | 7 +++----
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 59fcde6..7fa017b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1560,6 +1560,9 @@ struct drm_i915_gem_object {
 	/** Current tiling stride for the object, if it's tiled. */
 	uint32_t stride;
 
+	/** References from framebuffers, locks out tiling changes. */
+	unsigned long framebuffer_references;
+
 	/** Record of address bit 17 of each page at last unbind. */
 	unsigned long *bit_17;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index ac9ebe9..b139053 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -308,7 +308,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	if (obj->pin_count) {
+	if (obj->pin_count || obj->framebuffer_references) {
 		drm_gem_object_unreference_unlocked(&obj->base);
 		return -EBUSY;
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70f4091..f385efb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1916,10 +1916,7 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 		alignment = 0;
 		break;
 	case I915_TILING_Y:
-		/* Despite that we check this in framebuffer_init userspace can
-		 * screw us over and change the tiling after the fact. Only
-		 * pinned buffers can't change their tiling. */
-		DRM_DEBUG_DRIVER("Y tiled not allowed for scan out buffers\n");
+		WARN(1, "Y tiled bo slipped through, driver bug!\n");
 		return -EINVAL;
 	default:
 		BUG();
@@ -9954,6 +9951,7 @@ static void intel_setup_outputs(struct drm_device *dev)
 void intel_framebuffer_fini(struct intel_framebuffer *fb)
 {
 	drm_framebuffer_cleanup(&fb->base);
+	WARN_ON(!fb->obj->framebuffer_references--);
 	drm_gem_object_unreference_unlocked(&fb->obj->base);
 }
 
@@ -10080,6 +10078,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 
 	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
 	intel_fb->obj = obj;
+	intel_fb->obj->framebuffer_references++;
 
 	ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
 	if (ret) {
-- 
1.8.1.4

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

Re: [PATCH 2/4] drm/i915: prevent tiling changes on framebuffer backing storage

[Chris Wilson]

On Wed, Oct 09, 2013 at 09:23:52PM +0200, Daniel Vetter wrote:
> Assuming that all framebuffer related metadata is invariant simplifies
> our userspace input data checking. And current userspace always first
> updates the tiling of an object before creating a framebuffer with it.

Userspace already changes the tiling layout whilst keeping the fb id.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: prevent tiling changes on framebuffer backing storage

[Daniel Vetter]

On Wed, Oct 09, 2013 at 10:29:39PM +0100, Chris Wilson wrote:
> On Wed, Oct 09, 2013 at 09:23:52PM +0200, Daniel Vetter wrote:
> > Assuming that all framebuffer related metadata is invariant simplifies
> > our userspace input data checking. And current userspace always first
> > updates the tiling of an object before creating a framebuffer with it.
> 
> Userspace already changes the tiling layout whilst keeping the fb id.

How exactly does that work? You can't really change the fb pitch without
creating a new one ... That leaves switching from tiled to untiled and
back I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: prevent tiling changes on framebuffer backing storage

[Daniel Vetter]

On Thu, Oct 10, 2013 at 12:02:17AM +0200, Daniel Vetter wrote:
> On Wed, Oct 09, 2013 at 10:29:39PM +0100, Chris Wilson wrote:
> > On Wed, Oct 09, 2013 at 09:23:52PM +0200, Daniel Vetter wrote:
> > > Assuming that all framebuffer related metadata is invariant simplifies
> > > our userspace input data checking. And current userspace always first
> > > updates the tiling of an object before creating a framebuffer with it.
> > 
> > Userspace already changes the tiling layout whilst keeping the fb id.
> 
> How exactly does that work? You can't really change the fb pitch without
> creating a new one ... That leaves switching from tiled to untiled and
> back I think.

Meh, didn't read sna carefully enough. On a second read we only seem to
have the code added

commit 0dd20381364aabede2e1306945abe21d57c1d7b4
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sun Sep 29 11:19:46 2013 +0100

    sna: Resize an existing framebuffer if possible

That seems to just be for the cache, should be able to cope with failures
and we can fix it by moving the rmfb up before the set_tiling. It's also
only 2 weeks old.

So if there's nothing else I've missed I strongly vote to break the
pre-release over saner intefaces - allowing tiling to change kinda wreaks
a bit havoc with out in-kernel checks ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: prevent tiling changes on framebuffer backing storage

[Ville Syrjälä]

On Thu, Oct 10, 2013 at 12:09:48AM +0200, Daniel Vetter wrote:
> On Thu, Oct 10, 2013 at 12:02:17AM +0200, Daniel Vetter wrote:
> > On Wed, Oct 09, 2013 at 10:29:39PM +0100, Chris Wilson wrote:
> > > On Wed, Oct 09, 2013 at 09:23:52PM +0200, Daniel Vetter wrote:
> > > > Assuming that all framebuffer related metadata is invariant simplifies
> > > > our userspace input data checking. And current userspace always first
> > > > updates the tiling of an object before creating a framebuffer with it.
> > > 
> > > Userspace already changes the tiling layout whilst keeping the fb id.
> > 
> > How exactly does that work? You can't really change the fb pitch without
> > creating a new one ... That leaves switching from tiled to untiled and
> > back I think.
> 
> Meh, didn't read sna carefully enough. On a second read we only seem to
> have the code added
> 
> commit 0dd20381364aabede2e1306945abe21d57c1d7b4
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Sun Sep 29 11:19:46 2013 +0100
> 
>     sna: Resize an existing framebuffer if possible
> 
> That seems to just be for the cache, should be able to cope with failures
> and we can fix it by moving the rmfb up before the set_tiling. It's also
> only 2 weeks old.
> 
> So if there's nothing else I've missed I strongly vote to break the
> pre-release over saner intefaces - allowing tiling to change kinda wreaks
> a bit havoc with out in-kernel checks ...

It would be a bit simpler if we can trust that things don't change
after we've created the fb.

-- 
Ville Syrjälä
Intel OTC

[PATCH 3/4] drm/i915: Use unsigned long for obj->user_pin_count

[Daniel Vetter]

At least on linux sizeof(long) == sizeof(void*) and the thinking
is that you can grab about as many references as there's memory.

Doesn't really matter, just a bit of OCD since the fixed size data
type in a pure in-kernel datastructure look off.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fa017b..f39a6b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1567,7 +1567,7 @@ struct drm_i915_gem_object {
 	unsigned long *bit_17;
 
 	/** User space pin count and filp owning the pin */
-	uint32_t user_pin_count;
+	unsigned long user_pin_count;
 	struct drm_file *pin_filp;
 
 	/** for phy allocated objects */
-- 
1.8.1.4

Re: [PATCH 3/4] drm/i915: Use unsigned long for obj->user_pin_count

[Ville Syrjälä]

On Wed, Oct 09, 2013 at 09:23:53PM +0200, Daniel Vetter wrote:
> At least on linux sizeof(long) == sizeof(void*) and the thinking
> is that you can grab about as many references as there's memory.

Well, there can be more memory than there is address space.

Unchecked counters always leave me a bit uneasy, so an explicit check
is what I'd prefer to see.

> 
> Doesn't really matter, just a bit of OCD since the fixed size data
> type in a pure in-kernel datastructure look off.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7fa017b..f39a6b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1567,7 +1567,7 @@ struct drm_i915_gem_object {
>  	unsigned long *bit_17;
>  
>  	/** User space pin count and filp owning the pin */
> -	uint32_t user_pin_count;
> +	unsigned long user_pin_count;
>  	struct drm_file *pin_filp;
>  
>  	/** for phy allocated objects */
> -- 
> 1.8.1.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/4] drm/i915: Use unsigned long for obj->user_pin_count

[Daniel Vetter]

On Thu, Oct 10, 2013 at 10:56 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Oct 09, 2013 at 09:23:53PM +0200, Daniel Vetter wrote:
>> At least on linux sizeof(long) == sizeof(void*) and the thinking
>> is that you can grab about as many references as there's memory.
>
> Well, there can be more memory than there is address space.
>
> Unchecked counters always leave me a bit uneasy, so an explicit check
> is what I'd prefer to see.

Not for normal kernel references, but this one here is just userspace.
I'll add a UINT_MAX check to the pin ioctl to plug that leak. That'll
also make this patch a notch more useful. I'm not going to write a
testcase for this though since Ben will complain about the runtime of
it ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Use unsigned long for obj->user_pin_count

[Daniel Vetter]

At least on linux sizeof(long) == sizeof(void*) and the thinking
is that you can grab about as many references as there's memory.

Doesn't really matter, just a bit of OCD since the fixed size data
type in a pure in-kernel datastructure look off.

v2: Ville asked for an overflow check since no one prevents userspace
from incrementing the pin count forever.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 drivers/gpu/drm/i915/i915_gem.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fa017b..f39a6b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1567,7 +1567,7 @@ struct drm_i915_gem_object {
 	unsigned long *bit_17;
 
 	/** User space pin count and filp owning the pin */
-	uint32_t user_pin_count;
+	unsigned long user_pin_count;
 	struct drm_file *pin_filp;
 
 	/** for phy allocated objects */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 71dd030..e8271d3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3929,6 +3929,11 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	if (obj->user_pin_count == UINT_MAX) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	if (obj->user_pin_count == 0) {
 		ret = i915_gem_obj_ggtt_pin(obj, args->alignment, true, false);
 		if (ret)
-- 
1.8.1.4

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

Re: [PATCH] drm/i915: Use unsigned long for obj->user_pin_count

[Chris Wilson]

On Thu, Oct 10, 2013 at 01:29:37PM +0200, Daniel Vetter wrote:
> At least on linux sizeof(long) == sizeof(void*) and the thinking
> is that you can grab about as many references as there's memory.
> 
> Doesn't really matter, just a bit of OCD since the fixed size data
> type in a pure in-kernel datastructure look off.
> 
> v2: Ville asked for an overflow check since no one prevents userspace
> from incrementing the pin count forever.

UINT_MAX? You just changed the type to unsigned long.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Use unsigned long for obj->user_pin_count

[Daniel Vetter]

At least on linux sizeof(long) == sizeof(void*) and the thinking
is that you can grab about as many references as there's memory.

Doesn't really matter, just a bit of OCD since the fixed size data
type in a pure in-kernel datastructure look off.

v2: Ville asked for an overflow check since no one prevents userspace
from incrementing the pin count forever.

v3: s/INT/LONG/, noticed by Chris.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 drivers/gpu/drm/i915/i915_gem.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fa017b..f39a6b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1567,7 +1567,7 @@ struct drm_i915_gem_object {
 	unsigned long *bit_17;
 
 	/** User space pin count and filp owning the pin */
-	uint32_t user_pin_count;
+	unsigned long user_pin_count;
 	struct drm_file *pin_filp;
 
 	/** for phy allocated objects */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 71dd030..6988f81 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3929,6 +3929,11 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	if (obj->user_pin_count == ULONG_MAX) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	if (obj->user_pin_count == 0) {
 		ret = i915_gem_obj_ggtt_pin(obj, args->alignment, true, false);
 		if (ret)
-- 
1.8.1.4

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

Re: [PATCH] drm/i915: Use unsigned long for obj->user_pin_count

[Chris Wilson]

On Thu, Oct 10, 2013 at 02:46:37PM +0200, Daniel Vetter wrote:
> At least on linux sizeof(long) == sizeof(void*) and the thinking
> is that you can grab about as many references as there's memory.
> 
> Doesn't really matter, just a bit of OCD since the fixed size data
> type in a pure in-kernel datastructure look off.
> 
> v2: Ville asked for an overflow check since no one prevents userspace
> from incrementing the pin count forever.
> 
> v3: s/INT/LONG/, noticed by Chris.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/4] drm/i915: check gem bo size when creating framebuffers

[Daniel Vetter]

It's better to catch such fallout early, and this way we can rely on
the checking done by the drm core on fb->heigh/width at modeset time.

If we ever support planar formats on intel we might want to look into
a common helper to do all this, but for now this is good enough.

v2: Take tiling into account, requested by Ville.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f385efb..75970db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9983,6 +9983,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
 			   struct drm_i915_gem_object *obj)
 {
+	int aligned_height;
 	int pitch_limit;
 	int ret;
 
@@ -10076,6 +10077,12 @@ int intel_framebuffer_init(struct drm_device *dev,
 	if (mode_cmd->offsets[0] != 0)
 		return -EINVAL;
 
+	/* X-tiled is always 8 rows high. */
+	aligned_height = ALIGN(mode_cmd->height, obj->tiling_mode ? 8 : 1);
+	/* FIXME drm helper for size checks (especially planar formats)? */
+	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
+		return -EINVAL;
+
 	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
 	intel_fb->obj = obj;
 	intel_fb->obj->framebuffer_references++;
-- 
1.8.1.4

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

[PATCH] drm/i915: check gem bo size when creating framebuffers

[Daniel Vetter]

It's better to catch such fallout early, and this way we can rely on
the checking done by the drm core on fb->heigh/width at modeset time.

If we ever support planar formats on intel we might want to look into
a common helper to do all this, but for now this is good enough.

v2: Take tiling into account, requested by Ville.

v3: Fix tile height on gen2, spotted by Ville.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f385efb..c897ebf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9983,6 +9983,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
 			   struct drm_i915_gem_object *obj)
 {
+	int aligned_height, tile_height;
 	int pitch_limit;
 	int ret;
 
@@ -10076,6 +10077,13 @@ int intel_framebuffer_init(struct drm_device *dev,
 	if (mode_cmd->offsets[0] != 0)
 		return -EINVAL;
 
+	tile_height = IS_GEN2(dev) ? 16 : 8;
+	aligned_height = ALIGN(mode_cmd->height,
+			       obj->tiling_mode ? tile_height : 1);
+	/* FIXME drm helper for size checks (especially planar formats)? */
+	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
+		return -EINVAL;
+
 	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
 	intel_fb->obj = obj;
 	intel_fb->obj->framebuffer_references++;
-- 
1.8.1.4

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

Re: [PATCH 0/4] framebuffer_init checks

[Ville Syrjälä]

On Wed, Oct 09, 2013 at 09:23:50PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> That little patch turned into a bit more, and on top of it there's now also a
> new testcase in igt: kms_addfb. It checks for most of the evil stuff you can
> feed to addfb.

For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Cheers, Daniel
> 
> Daniel Vetter (4):
>   drm/i915: grab dev->struct_mutex around framebuffer_init
>   drm/i915: prevent tiling changes on framebuffer backing storage
>   drm/i915: Use unsigned long for obj->user_pin_count
>   drm/i915: check gem bo size when creating framebuffers
> 
>  drivers/gpu/drm/i915/i915_drv.h        |  5 ++++-
>  drivers/gpu/drm/i915/i915_gem_tiling.c |  2 +-
>  drivers/gpu/drm/i915/intel_display.c   | 33 ++++++++++++++++++++++++---------
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> -- 
> 1.8.1.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/4] framebuffer_init checks

[Daniel Vetter]

On Thu, Oct 10, 2013 at 02:42:40PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 09, 2013 at 09:23:50PM +0200, Daniel Vetter wrote:
> > Hi all,
> > 
> > That little patch turned into a bit more, and on top of it there's now also a
> > new testcase in igt: kms_addfb. It checks for most of the evil stuff you can
> > feed to addfb.
> 
> For the series:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

All merged, thanks for your review. For patch 2 I've added a note about
how exactly it'll affect shipped userspace and smashed Chris' grumpy irc
r-b on top.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch