| Message ID | 20201013111839.96637-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/execbuf: don't allow zero batch_len | expand |
Quoting Matthew Auld (2020-10-13 12:18:39) > As per the ABI batch_len is u32, however if the batch_len is left unset, > then the kernel will just assume batch_len is the size of the whole > batch object, however since the vma->size is u64, while the batch_len is > just u32 we can end up with batch_len = 0 if we are given too large batch > object(e.g 1ULL << 32), which doesn't look the intended behaviour and > probably leads to explosions on some HW. > > Testcase: igt/gem_exec_params/larger-than-life-batch > Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings") Nah. That's setting exec_len used for dispatch, not for parsing, which is still using i915_gem_execbuffer_parse(engine, &shadow_exec_entry, params->batch->obj, eb, args->batch_start_offset, args->batch_len, drm_is_current_master(file)); (and args->batch_len is straight from userspace and passed onwards) It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches") where we are using the user value of batch_len for allocating the shadow object and parsing. Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 4b09bcd70cf4..80c738c72e6e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -869,8 +869,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > return -EINVAL; > } > > - if (eb->batch_len == 0) > + if (eb->batch_len == 0) { > eb->batch_len = eb->batch->vma->size - eb->batch_start_offset; if (overflows_type(eb->batch->vma->size - eb->batch_start_offset, eb->batch_len)) It should not have caused the cmdparser any trouble though, it should have been quite happy to copy nothing and reject the batch for reaching the end too early (with a very slim chance of a stale MI_BATCH_BUFFER_END to the rescue). intel_gt_get_buffer_pool() looks suspect given a size of 0, it will either give the largest object it has cached or break upon creating/allocating internal pages. In terms of HW fail, only gen2 used the parameter and it has a very limited batch/GTT size. -Chris
Quoting Chris Wilson (2020-10-13 12:58:31) > Quoting Matthew Auld (2020-10-13 12:18:39) > > As per the ABI batch_len is u32, however if the batch_len is left unset, > > then the kernel will just assume batch_len is the size of the whole > > batch object, however since the vma->size is u64, while the batch_len is > > just u32 we can end up with batch_len = 0 if we are given too large batch > > object(e.g 1ULL << 32), which doesn't look the intended behaviour and > > probably leads to explosions on some HW. > > > > Testcase: igt/gem_exec_params/larger-than-life-batch > > Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings") > > Nah. That's setting exec_len used for dispatch, not for parsing, which > is still using > > i915_gem_execbuffer_parse(engine, &shadow_exec_entry, > params->batch->obj, > eb, > args->batch_start_offset, > args->batch_len, > drm_is_current_master(file)); > (and args->batch_len is straight from userspace and passed onwards) > > It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > where we are using the user value of batch_len for allocating the shadow > object and parsing. > > Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > index 4b09bcd70cf4..80c738c72e6e 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > > @@ -869,8 +869,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > > return -EINVAL; > > } > > > > - if (eb->batch_len == 0) > > + if (eb->batch_len == 0) { > > eb->batch_len = eb->batch->vma->size - eb->batch_start_offset; > > if (overflows_type(eb->batch->vma->size - eb->batch_start_offset, eb->batch_len)) And we shouldn't fail here. So bump to u64, and since the vma->size cannot be larger than the GTT, we will be safe for gen2 where it makes a difference. -Chris
On 13/10/2020 12:58, Chris Wilson wrote: > Quoting Matthew Auld (2020-10-13 12:18:39) >> As per the ABI batch_len is u32, however if the batch_len is left unset, >> then the kernel will just assume batch_len is the size of the whole >> batch object, however since the vma->size is u64, while the batch_len is >> just u32 we can end up with batch_len = 0 if we are given too large batch >> object(e.g 1ULL << 32), which doesn't look the intended behaviour and >> probably leads to explosions on some HW. >> >> Testcase: igt/gem_exec_params/larger-than-life-batch >> Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings") > > Nah. That's setting exec_len used for dispatch, not for parsing, which > is still using > > i915_gem_execbuffer_parse(engine, &shadow_exec_entry, > params->batch->obj, > eb, > args->batch_start_offset, > args->batch_len, > drm_is_current_master(file)); > (and args->batch_len is straight from userspace and passed onwards) > > It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > where we are using the user value of batch_len for allocating the shadow > object and parsing. > > Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches") On the topic of that patch, why is it looking at args->batch_len(might be zero), even though it looks like it is trying to move the eb->batch_len calculation to before we call eb_use_cmdparser(), so it can use it(the commit message seems to suggest that?), but then it only looks at the args version anyway. I don't get it. > >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> index 4b09bcd70cf4..80c738c72e6e 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> @@ -869,8 +869,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) >> return -EINVAL; >> } >> >> - if (eb->batch_len == 0) >> + if (eb->batch_len == 0) { >> eb->batch_len = eb->batch->vma->size - eb->batch_start_offset; > > if (overflows_type(eb->batch->vma->size - eb->batch_start_offset, eb->batch_len)) > > It should not have caused the cmdparser any trouble though, it should > have been quite happy to copy nothing and reject the batch for reaching > the end too early (with a very slim chance of a stale > MI_BATCH_BUFFER_END to the rescue). > > intel_gt_get_buffer_pool() looks suspect given a size of 0, it will > either give the largest object it has cached or break upon > creating/allocating internal pages. > > In terms of HW fail, only gen2 used the parameter and it has a very > limited batch/GTT size. > -Chris >
Quoting Matthew Auld (2020-10-13 15:07:46) > On 13/10/2020 12:58, Chris Wilson wrote: > > Quoting Matthew Auld (2020-10-13 12:18:39) > >> As per the ABI batch_len is u32, however if the batch_len is left unset, > >> then the kernel will just assume batch_len is the size of the whole > >> batch object, however since the vma->size is u64, while the batch_len is > >> just u32 we can end up with batch_len = 0 if we are given too large batch > >> object(e.g 1ULL << 32), which doesn't look the intended behaviour and > >> probably leads to explosions on some HW. > >> > >> Testcase: igt/gem_exec_params/larger-than-life-batch > >> Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings") > > > > Nah. That's setting exec_len used for dispatch, not for parsing, which > > is still using > > > > i915_gem_execbuffer_parse(engine, &shadow_exec_entry, > > params->batch->obj, > > eb, > > args->batch_start_offset, > > args->batch_len, > > drm_is_current_master(file)); > > (and args->batch_len is straight from userspace and passed onwards) > > > > It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > > where we are using the user value of batch_len for allocating the shadow > > object and parsing. > > > > Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches") > > On the topic of that patch, why is it looking at args->batch_len(might > be zero), even though it looks like it is trying to move the > eb->batch_len calculation to before we call eb_use_cmdparser(), so it > can use it(the commit message seems to suggest that?), but then it only > looks at the args version anyway. I don't get it. iirc, it was so that we could change the order around and later modify eb.batch_len before eb_use_cmdparser() [so eb.batch_len no longer would be zero, defeating the cheat]. -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 4b09bcd70cf4..80c738c72e6e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -869,8 +869,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) return -EINVAL; } - if (eb->batch_len == 0) + if (eb->batch_len == 0) { eb->batch_len = eb->batch->vma->size - eb->batch_start_offset; + if (unlikely(eb->batch_len == 0)) { + drm_dbg(&i915->drm, "Attempting to use too large batch\n"); + return -EINVAL; + } + } return 0;
As per the ABI batch_len is u32, however if the batch_len is left unset, then the kernel will just assume batch_len is the size of the whole batch object, however since the vma->size is u64, while the batch_len is just u32 we can end up with batch_len = 0 if we are given too large batch object(e.g 1ULL << 32), which doesn't look the intended behaviour and probably leads to explosions on some HW. Testcase: igt/gem_exec_params/larger-than-life-batch Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)