On 28/04/16 10:02, Chris Wilson wrote:
On Thu, Apr 28, 2016 at 11:54:04AM +0300, Jani Nikula wrote:
On Thu, 28 Apr 2016, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
On Fri, 20 Nov 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
On Fri, Nov 20, 2015 at 03:11:04PM +0000, Chris Wilson wrote:
The offset within and the length of the command sequence to execute are
supplied by the user with respect to the batch buffer. We should be
validating that region is wholly contained within the batch buffer;
make it so.
Reported-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a4c243cec4aa..e38284c1b89f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1462,6 +1462,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
/* take note of the batch buffer before we might reorder the lists */
batch_obj = eb_get_batch(eb);
+ if (args->batch_len > batch_obj->base.size ||
+ args->batch_start_offset > batch_obj->base.size - args->batch_len) {
lgtm. No possibility of overflow doing it that way.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
I don't know what I fat fingered with the previous mail, but I just
stumbled upon this patch and noticed it never made it. Is this still
valid?
Yup, I'd completely forgotten about this patch and it we don't have the
safeguard in the kernel yet.
-Chris
Hmm .. this will allow (len == 0) as long as start is in the range
[0..size). But later, i915_gem_ringbuffer_submission() will interpret
length 0 as meaning "sizeof batch_bo", which would be out-of-bounds if
start != 0. Relevant code is:
exec_len = args->batch_len;
exec_start = params->batch_obj_vm_offset +
params->args_batch_start_offset;
if (exec_len == 0)
exec_len = params->batch_obj->base.size;
Should we permit len == 0 iff start == 0? Or take it to mean "from start
to sizeof bo", and maybe put the replacement in the new check code
rather than later, in submission()?
Of course batch length is not actually used on later hardware, and so
the execlists version doesn't make this substitution, since any hardware
that can do execlists only uses the BB-START form without a length.
Nonetheless we might still want to validate and interpret this in a
uniform manner ...
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
