Re: [Intel-gfx] [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 12:32:00PM -0700, Kees Cook wrote: > On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter wrote: > > On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter wrote: > >> On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: > >>> On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: > >>> > It is possible to wrap the counter used to allocate the buffer for > >>> > relocation copies. This could lead to heap writing overflows. > >>> > > >>> > CVE-2013-0913 > >>> > > >>> > v3: collapse test, improve comment > >>> > v2: move check into validate_exec_list > >>> > > >>> > Signed-off-by: Kees Cook > >>> > Reported-by: Pinkie Pie > >>> > Cc: sta...@vger.kernel.org > >>> > >>> Looks good to me. The only bikeshed that remains is whether we should > >>> just collapse the two variables into one, but the current 'max - count' > >>> is more idiomatic and so preferrable. > >>> Reviewed-by: Chris Wilson > >> > >> Picked up for -fixes, thanks for the patch. > > > > I've forgotten to dump my wishlist: Can I have an i-g-t for this? For > > this bug here specifically an execbuf with just one buffer with too > > many relocs plus another execbuf with two buffers with relocation so > > that the 2nd relocation list will overflow should be sufficient. > > Sure thing. Where do these live? (Or what docs should I read for > this?) I'm assuming i-g-t means "intel graphics test"? :) Close :) GPU Tools. The tests lives in the tests directory of: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/ -- Damien -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Intel-gfx] [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 12:32:00PM -0700, Kees Cook wrote: On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote: On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the patch. I've forgotten to dump my wishlist: Can I have an i-g-t for this? For this bug here specifically an execbuf with just one buffer with too many relocs plus another execbuf with two buffers with relocation so that the 2nd relocation list will overflow should be sufficient. Sure thing. Where do these live? (Or what docs should I read for this?) I'm assuming i-g-t means intel graphics test? :) Close :) GPU Tools. The tests lives in the tests directory of: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/ -- Damien -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter wrote: > On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter wrote: >> On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: >>> On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: >>> > It is possible to wrap the counter used to allocate the buffer for >>> > relocation copies. This could lead to heap writing overflows. >>> > >>> > CVE-2013-0913 >>> > >>> > v3: collapse test, improve comment >>> > v2: move check into validate_exec_list >>> > >>> > Signed-off-by: Kees Cook >>> > Reported-by: Pinkie Pie >>> > Cc: sta...@vger.kernel.org >>> >>> Looks good to me. The only bikeshed that remains is whether we should >>> just collapse the two variables into one, but the current 'max - count' >>> is more idiomatic and so preferrable. >>> Reviewed-by: Chris Wilson >> >> Picked up for -fixes, thanks for the patch. > > I've forgotten to dump my wishlist: Can I have an i-g-t for this? For > this bug here specifically an execbuf with just one buffer with too > many relocs plus another execbuf with two buffers with relocation so > that the 2nd relocation list will overflow should be sufficient. Sure thing. Where do these live? (Or what docs should I read for this?) I'm assuming i-g-t means "intel graphics test"? :) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter wrote: > On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: >> On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: >> > It is possible to wrap the counter used to allocate the buffer for >> > relocation copies. This could lead to heap writing overflows. >> > >> > CVE-2013-0913 >> > >> > v3: collapse test, improve comment >> > v2: move check into validate_exec_list >> > >> > Signed-off-by: Kees Cook >> > Reported-by: Pinkie Pie >> > Cc: sta...@vger.kernel.org >> >> Looks good to me. The only bikeshed that remains is whether we should >> just collapse the two variables into one, but the current 'max - count' >> is more idiomatic and so preferrable. >> Reviewed-by: Chris Wilson > > Picked up for -fixes, thanks for the patch. I've forgotten to dump my wishlist: Can I have an i-g-t for this? For this bug here specifically an execbuf with just one buffer with too many relocs plus another execbuf with two buffers with relocation so that the 2nd relocation list will overflow should be sufficient. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the patch. I've forgotten to dump my wishlist: Can I have an i-g-t for this? For this bug here specifically an execbuf with just one buffer with too many relocs plus another execbuf with two buffers with relocation so that the 2nd relocation list will overflow should be sufficient. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote: On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the patch. I've forgotten to dump my wishlist: Can I have an i-g-t for this? For this bug here specifically an execbuf with just one buffer with too many relocs plus another execbuf with two buffers with relocation so that the 2nd relocation list will overflow should be sufficient. Sure thing. Where do these live? (Or what docs should I read for this?) I'm assuming i-g-t means intel graphics test? :) -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: > On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: > > It is possible to wrap the counter used to allocate the buffer for > > relocation copies. This could lead to heap writing overflows. > > > > CVE-2013-0913 > > > > v3: collapse test, improve comment > > v2: move check into validate_exec_list > > > > Signed-off-by: Kees Cook > > Reported-by: Pinkie Pie > > Cc: sta...@vger.kernel.org > > Looks good to me. The only bikeshed that remains is whether we should > just collapse the two variables into one, but the current 'max - count' > is more idiomatic and so preferrable. > Reviewed-by: Chris Wilson Picked up for -fixes, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: > It is possible to wrap the counter used to allocate the buffer for > relocation copies. This could lead to heap writing overflows. > > CVE-2013-0913 > > v3: collapse test, improve comment > v2: move check into validate_exec_list > > Signed-off-by: Kees Cook > Reported-by: Pinkie Pie > Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -Chris -- Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
CVE-2013-0913
v3: collapse test, improve comment
v2: move check into validate_exec_list
Signed-off-by: Kees Cook
Reported-by: Pinkie Pie
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b3a40ee..094ba41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int relocs_total = 0;
+ int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
for (i = 0; i < count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -740,10 +742,13 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].flags & __EXEC_OBJECT_UNKNOWN_FLAGS)
return -EINVAL;
- /* First check for malicious input causing overflow */
- if (exec[i].relocation_count >
- INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
+ /* First check for malicious input causing overflow in
+* the worst case where we need to allocate the entire
+* relocation tree as a single array.
+*/
+ if (exec[i].relocation_count > relocs_max - relocs_total)
return -EINVAL;
+ relocs_total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
CVE-2013-0913
v3: collapse test, improve comment
v2: move check into validate_exec_list
Signed-off-by: Kees Cook keesc...@chromium.org
Reported-by: Pinkie Pie
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b3a40ee..094ba41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int relocs_total = 0;
+ int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
for (i = 0; i count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -740,10 +742,13 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].flags __EXEC_OBJECT_UNKNOWN_FLAGS)
return -EINVAL;
- /* First check for malicious input causing overflow */
- if (exec[i].relocation_count
- INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
+ /* First check for malicious input causing overflow in
+* the worst case where we need to allocate the entire
+* relocation tree as a single array.
+*/
+ if (exec[i].relocation_count relocs_max - relocs_total)
return -EINVAL;
+ relocs_total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
