[PATCH v2 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()
On 32-bit systems, a large args->num_cliprects from userspace via ioctl
may overflow the allocation size, leading to out-of-bounds access.
This vulnerability was introduced in commit 432e58ed ("drm/i915: Avoid
allocation for execbuffer object list").
Signed-off-by: Xi Wang
Cc: Chris Wilson
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7c50e58..de43194 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,6 +1133,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
return -EINVAL;
}
+ if (args->num_cliprects > UINT_MAX / sizeof(*cliprects)) {
+ DRM_DEBUG("execbuf with %u cliprects\n",
+ args->num_cliprects);
+ return -EINVAL;
+ }
cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
GFP_KERNEL);
if (cliprects == NULL) {
--
1.7.5.4
[PATCH v2 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On 32-bit systems, a large args->buffer_count from userspace via ioctl
may overflow the allocation size, leading to out-of-bounds access.
This vulnerability was introduced in commit 8408c282 ("drm/i915:
First try a normal large kmalloc for the temporary exec buffers").
Signed-off-by: Xi Wang
Cc: Chris Wilson
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f51a696..7c50e58 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1404,7 +1404,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
struct drm_i915_gem_exec_object2 *exec2_list = NULL;
int ret;
- if (args->buffer_count < 1) {
+ if (args->buffer_count < 1 ||
+ args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
return -EINVAL;
}
--
1.7.5.4
[PATCH v2 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On 32-bit systems, a large args-buffer_count from userspace via ioctl
may overflow the allocation size, leading to out-of-bounds access.
This vulnerability was introduced in commit 8408c282 (drm/i915:
First try a normal large kmalloc for the temporary exec buffers).
Signed-off-by: Xi Wang xi.w...@gmail.com
Cc: Chris Wilson ch...@chris-wilson.co.uk
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f51a696..7c50e58 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1404,7 +1404,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
struct drm_i915_gem_exec_object2 *exec2_list = NULL;
int ret;
- if (args-buffer_count 1) {
+ if (args-buffer_count 1 ||
+ args-buffer_count UINT_MAX / sizeof(*exec2_list)) {
DRM_DEBUG(execbuf2 with %d buffers\n, args-buffer_count);
return -EINVAL;
}
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()
On 32-bit systems, a large args-num_cliprects from userspace via ioctl
may overflow the allocation size, leading to out-of-bounds access.
This vulnerability was introduced in commit 432e58ed (drm/i915: Avoid
allocation for execbuffer object list).
Signed-off-by: Xi Wang xi.w...@gmail.com
Cc: Chris Wilson ch...@chris-wilson.co.uk
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7c50e58..de43194 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,6 +1133,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
return -EINVAL;
}
+ if (args-num_cliprects UINT_MAX / sizeof(*cliprects)) {
+ DRM_DEBUG(execbuf with %u cliprects\n,
+ args-num_cliprects);
+ return -EINVAL;
+ }
cliprects = kmalloc(args-num_cliprects * sizeof(*cliprects),
GFP_KERNEL);
if (cliprects == NULL) {
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/savage: fix integer overflows in savage_bci_cmdbuf()
Since cmdbuf->size and cmdbuf->nbox are from userspace, a large value
would overflow the allocation size, leading to out-of-bounds access.
Signed-off-by: Xi Wang
---
drivers/gpu/drm/savage/savage_state.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/savage/savage_state.c
b/drivers/gpu/drm/savage/savage_state.c
index 031aaaf..b6d8608 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -988,7 +988,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data,
struct drm_file *file_
* for locking on FreeBSD.
*/
if (cmdbuf->size) {
- kcmd_addr = kmalloc(cmdbuf->size * 8, GFP_KERNEL);
+ kcmd_addr = kmalloc_array(cmdbuf->size, 8, GFP_KERNEL);
if (kcmd_addr == NULL)
return -ENOMEM;
@@ -1015,8 +1015,8 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data,
struct drm_file *file_
cmdbuf->vb_addr = kvb_addr;
}
if (cmdbuf->nbox) {
- kbox_addr = kmalloc(cmdbuf->nbox * sizeof(struct drm_clip_rect),
- GFP_KERNEL);
+ kbox_addr = kmalloc_array(cmdbuf->nbox, sizeof(struct
drm_clip_rect),
+ GFP_KERNEL);
if (kbox_addr == NULL) {
ret = -ENOMEM;
goto done;
--
1.7.5.4
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 3:40 PM, Chris Wilson wrote:
> On Fri, 6 Apr 2012 14:17:41 -0400, Xi Wang wrote:
>>
>> Why an attempt to vmalloc? The overflow check in drm_malloc_ab()
>> will simply return NULL and fail the ioctl with -ENOMEM.
>
> It's an invalid value for the ioctl and should be treated as such, not
> making ENOMEM more ambiguous.
We could copy and paste the overflow check so as to return -EINVAL.
I just doubt how much that would help --- you can find existing usages
in other functions, for example, in i915_gem_execbuffer():
/* Copy in the exec list from userland */
exec_list = drm_malloc_ab(sizeof(*exec_list), args->buffer_count);
exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count);
if (exec_list == NULL || exec2_list == NULL) {
DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
args->buffer_count);
drm_free_large(exec_list);
drm_free_large(exec2_list);
return -ENOMEM;
}
Should we fix all these as well by repeating the checks and returning
-EINVAL? I am worried about the code bloat / readability price you
would pay for getting a different error code.
BTW, I've also seen code using E2BIG. Any documented guideline?
- xi
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 10:44 AM, Chris Wilson wrote: > That I agreed with, I just disagree with how you chose to handle it. > Rather than continue on and attempt to vmalloc a large array we should > just fail the ioctl with EINVAL. Why an attempt to vmalloc? The overflow check in drm_malloc_ab() will simply return NULL and fail the ioctl with -ENOMEM. - xi
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 9:54 AM, Chris Wilson wrote: > On Fri, 6 Apr 2012 09:46:46 -0400, Xi Wang wrote: >> On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote: >> >>> On Fri, 6 Apr 2012 08:58:18 -0400, Xi Wang wrote: >>>> A large args->buffer_count from userspace may overflow the allocation >>>> size, leading to out-of-bounds access. >>>> >>>> Use kmalloc_array() to avoid that. >>> >>> I can safely say that exec list larger than 4GiB is going to be an >>> illegal operation and would rather the ioctl failed outright with >>> EINVAL. >> >> On 32-bit platform? > > On any platform. The largest it can legally be is a few tens of megabytes. IDGI. First we come to i915_gem_execbuffer2() from ioctl: exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, ...); args->buffer_count is passed from userspace so it can be any value. Let it overflow the 32-bit multiplication and turn the call to: exec2_list = kmalloc(0, ...); Then the subsequent call to i915_gem_do_execbuffer(..., exec2_list) may read exec2_list, which is out of bounds. - xi
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote: > On Fri, 6 Apr 2012 08:58:18 -0400, Xi Wang wrote: >> A large args->buffer_count from userspace may overflow the allocation >> size, leading to out-of-bounds access. >> >> Use kmalloc_array() to avoid that. > > I can safely say that exec list larger than 4GiB is going to be an > illegal operation and would rather the ioctl failed outright with > EINVAL. On 32-bit platform? - xi
[PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()
A large args->num_cliprects from userspace may overflow the allocation
size, leading to out-of-bounds access.
| i915_gem_do_execbuffer()
| i915_gem_execbuffer()
Use kmalloc_array() to avoid that.
Signed-off-by: Xi Wang
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 19962bd..607be3d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,8 +1133,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
return -EINVAL;
}
- cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
- GFP_KERNEL);
+ cliprects = kmalloc_array(args->num_cliprects,
sizeof(*cliprects),
+ GFP_KERNEL);
if (cliprects == NULL) {
ret = -ENOMEM;
goto pre_mutex_err;
--
1.7.5.4
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
A large args->buffer_count from userspace may overflow the allocation size, leading to out-of-bounds access. Use kmalloc_array() to avoid that. Signed-off-by: Xi Wang --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index f51a696..19962bd 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1409,8 +1409,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, return -EINVAL; } - exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, -GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); + exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list), + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); if (exec2_list == NULL) exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count); -- 1.7.5.4
[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
A large args-buffer_count from userspace may overflow the allocation size, leading to out-of-bounds access. Use kmalloc_array() to avoid that. Signed-off-by: Xi Wang xi.w...@gmail.com --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index f51a696..19962bd 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1409,8 +1409,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, return -EINVAL; } - exec2_list = kmalloc(sizeof(*exec2_list)*args-buffer_count, -GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); + exec2_list = kmalloc_array(args-buffer_count, sizeof(*exec2_list), + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY); if (exec2_list == NULL) exec2_list = drm_malloc_ab(sizeof(*exec2_list), args-buffer_count); -- 1.7.5.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()
A large args-num_cliprects from userspace may overflow the allocation
size, leading to out-of-bounds access.
| i915_gem_do_execbuffer()
| i915_gem_execbuffer()
Use kmalloc_array() to avoid that.
Signed-off-by: Xi Wang xi.w...@gmail.com
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 19962bd..607be3d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,8 +1133,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
return -EINVAL;
}
- cliprects = kmalloc(args-num_cliprects * sizeof(*cliprects),
- GFP_KERNEL);
+ cliprects = kmalloc_array(args-num_cliprects,
sizeof(*cliprects),
+ GFP_KERNEL);
if (cliprects == NULL) {
ret = -ENOMEM;
goto pre_mutex_err;
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 10:44 AM, Chris Wilson wrote: That I agreed with, I just disagree with how you chose to handle it. Rather than continue on and attempt to vmalloc a large array we should just fail the ioctl with EINVAL. Why an attempt to vmalloc? The overflow check in drm_malloc_ab() will simply return NULL and fail the ioctl with -ENOMEM. - xi ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
On Apr 6, 2012, at 3:40 PM, Chris Wilson wrote:
On Fri, 6 Apr 2012 14:17:41 -0400, Xi Wang xi.w...@gmail.com wrote:
Why an attempt to vmalloc? The overflow check in drm_malloc_ab()
will simply return NULL and fail the ioctl with -ENOMEM.
It's an invalid value for the ioctl and should be treated as such, not
making ENOMEM more ambiguous.
We could copy and paste the overflow check so as to return -EINVAL.
I just doubt how much that would help --- you can find existing usages
in other functions, for example, in i915_gem_execbuffer():
/* Copy in the exec list from userland */
exec_list = drm_malloc_ab(sizeof(*exec_list), args-buffer_count);
exec2_list = drm_malloc_ab(sizeof(*exec2_list), args-buffer_count);
if (exec_list == NULL || exec2_list == NULL) {
DRM_DEBUG(Failed to allocate exec list for %d buffers\n,
args-buffer_count);
drm_free_large(exec_list);
drm_free_large(exec2_list);
return -ENOMEM;
}
Should we fix all these as well by repeating the checks and returning
-EINVAL? I am worried about the code bloat / readability price you
would pay for getting a different error code.
BTW, I've also seen code using E2BIG. Any documented guideline?
- xi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/savage: fix integer overflows in savage_bci_cmdbuf()
Since cmdbuf-size and cmdbuf-nbox are from userspace, a large value
would overflow the allocation size, leading to out-of-bounds access.
Signed-off-by: Xi Wang xi.w...@gmail.com
---
drivers/gpu/drm/savage/savage_state.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/savage/savage_state.c
b/drivers/gpu/drm/savage/savage_state.c
index 031aaaf..b6d8608 100644
--- a/drivers/gpu/drm/savage/savage_state.c
+++ b/drivers/gpu/drm/savage/savage_state.c
@@ -988,7 +988,7 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data,
struct drm_file *file_
* for locking on FreeBSD.
*/
if (cmdbuf-size) {
- kcmd_addr = kmalloc(cmdbuf-size * 8, GFP_KERNEL);
+ kcmd_addr = kmalloc_array(cmdbuf-size, 8, GFP_KERNEL);
if (kcmd_addr == NULL)
return -ENOMEM;
@@ -1015,8 +1015,8 @@ int savage_bci_cmdbuf(struct drm_device *dev, void *data,
struct drm_file *file_
cmdbuf-vb_addr = kvb_addr;
}
if (cmdbuf-nbox) {
- kbox_addr = kmalloc(cmdbuf-nbox * sizeof(struct drm_clip_rect),
- GFP_KERNEL);
+ kbox_addr = kmalloc_array(cmdbuf-nbox, sizeof(struct
drm_clip_rect),
+ GFP_KERNEL);
if (kbox_addr == NULL) {
ret = -ENOMEM;
goto done;
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -fixes] vmwgfx: fix incorrect VRAM size check in vmw_kms_fb_create()
Commit e133e737 didn't correctly fix the integer overflow issue.
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > dev_priv->vram_size)) {
+ if (unlikely(required_size > (u64) dev_priv->vram_size)) {
Note that both pitch and height are u32. Their product is still u32 and
would overflow before being assigned to required_size. A correct way is
to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd->pitch * (u64)mode_cmd->height;
This patch calls the existing vmw_kms_validate_mode_vram() for
validation.
Signed-off-by: Xi Wang
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 8aa1dbb..f94b33a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1093,7 +1093,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1102,8 +1101,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > (u64) dev_priv->vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd->pitch,
+ mode_cmd->height)) {
DRM_ERROR("VRAM size is too small for requested mode.\n");
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
[PATCH RESEND] vmwgfx: fix incorrect vram size check in vmw_kms_fb_create()
On Dec 21, 2011, at 4:30 AM, David Airlie wrote: > > This patch doesn't apply with git am, please regenerate it, > > Thomas, can you ack as well please? > > Is this for -fixes or -next? Sorry this patch is for vmwgfx.git. I will redo a version that can be applied to the mainline kernel. - xi
[PATCH RESEND] vmwgfx: fix incorrect vram size check in vmw_kms_fb_create()
The previous commit didn't correctly fix the integer overflow issue.
http://git.kernel.org/linus/e133e737
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size dev_priv-vram_size)) {
+ if (unlikely(required_size (u64) dev_priv-vram_size)) {
Note that both pitch and height are u32, their product is still u32 and
would overflow before being assigned to required_size. A correct way is
to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd-pitch * (u64)mode_cmd-height;
This patch calls an existing function vmw_kms_validate_mode_vram()
for validation.
Signed-off-by: Xi Wang xi.w...@gmail.com
---
vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/vmwgfx_kms.c b/vmwgfx_kms.c
index b87afdf..6b8857e 100644
--- a/vmwgfx_kms.c
+++ b/vmwgfx_kms.c
@@ -1101,7 +1101,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1110,8 +1109,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size (u64) dev_priv-vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd-pitch,
+ mode_cmd-height)) {
DRM_ERROR(VRAM size is too small for requested mode.\n);
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RESEND] vmwgfx: fix incorrect vram size check in vmw_kms_fb_create()
On Dec 21, 2011, at 4:30 AM, David Airlie wrote: This patch doesn't apply with git am, please regenerate it, Thomas, can you ack as well please? Is this for -fixes or -next? Sorry this patch is for vmwgfx.git. I will redo a version that can be applied to the mainline kernel. - xi ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -fixes] vmwgfx: fix incorrect VRAM size check in vmw_kms_fb_create()
Commit e133e737 didn't correctly fix the integer overflow issue.
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size dev_priv-vram_size)) {
+ if (unlikely(required_size (u64) dev_priv-vram_size)) {
Note that both pitch and height are u32. Their product is still u32 and
would overflow before being assigned to required_size. A correct way is
to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd-pitch * (u64)mode_cmd-height;
This patch calls the existing vmw_kms_validate_mode_vram() for
validation.
Signed-off-by: Xi Wang xi.w...@gmail.com
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 8aa1dbb..f94b33a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1093,7 +1093,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1102,8 +1101,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size (u64) dev_priv-vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd-pitch,
+ mode_cmd-height)) {
DRM_ERROR(VRAM size is too small for requested mode.\n);
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH RESEND] vmwgfx: fix incorrect vram size check in vmw_kms_fb_create()
The previous commit didn't correctly fix the integer overflow issue.
http://git.kernel.org/linus/e133e737
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > dev_priv->vram_size)) {
+ if (unlikely(required_size > (u64) dev_priv->vram_size)) {
Note that both pitch and height are u32, their product is still u32 and
would overflow before being assigned to required_size. A correct way is
to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd->pitch * (u64)mode_cmd->height;
This patch calls an existing function vmw_kms_validate_mode_vram()
for validation.
Signed-off-by: Xi Wang
---
vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/vmwgfx_kms.c b/vmwgfx_kms.c
index b87afdf..6b8857e 100644
--- a/vmwgfx_kms.c
+++ b/vmwgfx_kms.c
@@ -1101,7 +1101,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1110,8 +1109,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > (u64) dev_priv->vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd->pitch,
+ mode_cmd->height)) {
DRM_ERROR("VRAM size is too small for requested mode.\n");
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
[PATCH] drm/vmwgfx: fix incorrect VRAM size check in vmw_kms_fb_create()
The commit e133e737 didn't correctly fix the overflow issue.
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size dev_priv-vram_size)) {
+ if (unlikely(required_size (u64) dev_priv-vram_size)) {
Since pitch and height are u32, their product is still 32-bit and
would overflow. Converting the result to u64 cannot help. A correct
way is to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd-pitch * (u64)mode_cmd-height;
This fix calls vmw_kms_validate_mode_vram() for validation.
Signed-off-by: Xi Wang xi.w...@gmail.com
Cc: Thomas Hellstrom thellst...@vmware.com
Cc: Dave Airlie airl...@redhat.com
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 37d4054..582a4d7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1003,7 +1003,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1012,8 +1011,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd-pitch * mode_cmd-height;
- if (unlikely(required_size (u64) dev_priv-vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd-pitch,
+ mode_cmd-height)) {
DRM_ERROR(VRAM size is too small for requested mode.\n);
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vmwgfx: fix incorrect VRAM size check in vmw_kms_fb_create()
The commit e133e737 didn't correctly fix the overflow issue.
- unsigned int required_size;
+ u64 required_size;
...
required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > dev_priv->vram_size)) {
+ if (unlikely(required_size > (u64) dev_priv->vram_size)) {
Since pitch and height are u32, their product is still 32-bit and
would overflow. Converting the result to u64 cannot help. A correct
way is to convert pitch and height to u64 before the multiplication.
required_size = (u64)mode_cmd->pitch * (u64)mode_cmd->height;
This fix calls vmw_kms_validate_mode_vram() for validation.
Signed-off-by: Xi Wang
Cc: Thomas Hellstrom
Cc: Dave Airlie
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 37d4054..582a4d7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1003,7 +1003,6 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
struct vmw_surface *surface = NULL;
struct vmw_dma_buffer *bo = NULL;
struct ttm_base_object *user_obj;
- u64 required_size;
int ret;
/**
@@ -1012,8 +1011,9 @@ static struct drm_framebuffer *vmw_kms_fb_create(struct
drm_device *dev,
* requested framebuffer.
*/
- required_size = mode_cmd->pitch * mode_cmd->height;
- if (unlikely(required_size > (u64) dev_priv->vram_size)) {
+ if (!vmw_kms_validate_mode_vram(dev_priv,
+ mode_cmd->pitch,
+ mode_cmd->height)) {
DRM_ERROR("VRAM size is too small for requested mode.\n");
return ERR_PTR(-ENOMEM);
}
--
1.7.5.4
vmwgfx: strange loop in vmw_kms_update_layout_ioctl()
Hi,
I came across this code snippet at vmwgfx_kms.c:2000. The loop variable i is
never used and rects is checked again and again. Should it be something like
rects[i].x instead of rects->x? Thanks.
for (i = 0; i < arg->num_outputs; ++i) {
if (rects->x < 0 ||
rects->y < 0 ||
rects->x + rects->w > mode_config->max_width ||
rects->y + rects->h > mode_config->max_height) {
DRM_ERROR("Invalid GUI layout.\n");
ret = -EINVAL;
goto out_free;
}
}
- xi
vmwgfx: strange loop in vmw_kms_update_layout_ioctl()
Hi,
I came across this code snippet at vmwgfx_kms.c:2000. The loop variable i is
never used and rects is checked again and again. Should it be something like
rects[i].x instead of rects-x? Thanks.
for (i = 0; i arg-num_outputs; ++i) {
if (rects-x 0 ||
rects-y 0 ||
rects-x + rects-w mode_config-max_width ||
rects-y + rects-h mode_config-max_height) {
DRM_ERROR(Invalid GUI layout.\n);
ret = -EINVAL;
goto out_free;
}
}
- xi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: integer overflow in drm_mode_dirtyfb_ioctl()
There is a potential integer overflow in drm_mode_dirtyfb_ioctl()
if userspace passes in a large num_clips. The call to kmalloc would
allocate a small buffer, and the call to fb->funcs->dirty may result
in a memory corruption.
Reported-by: Haogang Chen
Signed-off-by: Xi Wang
---
drivers/gpu/drm/drm_crtc.c |4
include/drm/drm_mode.h |2 ++
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 405c63b..8323fc3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1873,6 +1873,10 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
}
if (num_clips && clips_ptr) {
+ if (num_clips < 0 || num_clips > DRM_MODE_FB_DIRTY_MAX_CLIPS) {
+ ret = -EINVAL;
+ goto out_err1;
+ }
clips = kzalloc(num_clips * sizeof(*clips), GFP_KERNEL);
if (!clips) {
ret = -ENOMEM;
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index d30bedf..ddd46db 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -235,6 +235,8 @@ struct drm_mode_fb_cmd {
#define DRM_MODE_FB_DIRTY_ANNOTATE_FILL 0x02
#define DRM_MODE_FB_DIRTY_FLAGS 0x03
+#define DRM_MODE_FB_DIRTY_MAX_CLIPS 256
+
/*
* Mark a region of a framebuffer as dirty.
*
--
1.7.5.4
[PATCH] drm: integer overflow in drm_mode_dirtyfb_ioctl()
There is a potential integer overflow in drm_mode_dirtyfb_ioctl()
if userspace passes in a large num_clips. The call to kmalloc would
allocate a small buffer, and the call to fb-funcs-dirty may result
in a memory corruption.
Reported-by: Haogang Chen haogangc...@gmail.com
Signed-off-by: Xi Wang xi.w...@gmail.com
---
drivers/gpu/drm/drm_crtc.c |4
include/drm/drm_mode.h |2 ++
2 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 405c63b..8323fc3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1873,6 +1873,10 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
}
if (num_clips clips_ptr) {
+ if (num_clips 0 || num_clips DRM_MODE_FB_DIRTY_MAX_CLIPS) {
+ ret = -EINVAL;
+ goto out_err1;
+ }
clips = kzalloc(num_clips * sizeof(*clips), GFP_KERNEL);
if (!clips) {
ret = -ENOMEM;
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index d30bedf..ddd46db 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -235,6 +235,8 @@ struct drm_mode_fb_cmd {
#define DRM_MODE_FB_DIRTY_ANNOTATE_FILL 0x02
#define DRM_MODE_FB_DIRTY_FLAGS 0x03
+#define DRM_MODE_FB_DIRTY_MAX_CLIPS 256
+
/*
* Mark a region of a framebuffer as dirty.
*
--
1.7.5.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
