On Thu, Mar 14, 2013 at 09:50:04PM -0700, Ben Widawsky wrote:
On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
In order to prevent a potential NULL deference with hostile userspace, we need to check whether the ioctl was passed an invalid args pointer.
Reported-by: Tommi Rantala tt.rantala@gmail.com Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow... Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 365e41a..9f5602e 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data, struct drm_i915_gem_exec_object2 *exec2_list = NULL; int ret, i;
- if (args->buffer_count < 1) {
- if (args == NULL)
return -EINVAL;- if (args->buffer_count < 1 ||
args->buffer_count > INT_MAX / sizeof(*exec2_list)) {@@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, struct drm_i915_gem_exec_object2 *exec2_list = NULL; int ret;
- if (args == NULL)
return -EINVAL;- if (args->buffer_count < 1 ||
args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
args->buffer_count > INT_MAX / sizeof(*exec2_list)) {Why did you change UINT_MAX to INT_MAX?
Because we check later against INT_MAX, and I didn't like the confusion. If we are going to pick an arbitrary limit, lets at least be consistent.
TBH, I'm confused what we're trying to achieve, and why we need anything other than: if (!args->buffer_count)
Because we then promptly do a u32 multiply and we need to be sure that userspace can't trigger an overflow there and cause us to read unallocated memory later.
I'm also not seeing how the NULL checks are needed since at least it seems to be for execbuffer (IOW) we could never have NULL args.
That's what I thought too. Looking at the stack trace, the empirical evidence is that we need the check. -Chris