| Message ID | 1456929209-18822-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 02, 2016 at 02:33:29PM +0000, Matthew Auld wrote: > When binding pages for a partial view we should check that the offset + > size is valid relative to the size of the gem object. > > v2: Don't use pages->nents to determine the page count (Tvrtko Ursulin) > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 7b8de85..2c49d043 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3493,6 +3493,10 @@ intel_partial_pages(const struct i915_ggtt_view *view, > struct sg_page_iter obj_sg_iter; > int ret = -ENOMEM; > > + if (view->params.partial.offset + view->params.partial.size > Handle overflow? Why do it here and not at creation? What bug are you fixing? > + obj->base.size >> PAGE_SHIFT) > + return ERR_PTR(-EINVAL); Is this a user error? Or just an internal programming bug. -Chris
> Handle overflow? Okay, good idea. > Why do it here and not at creation? We could, given that we currently only exercise partial views in the gem fault handler code, but as Joonas mentioned we are expecting further use of partial views, so it makes sense to have the check in only one place. > What bug are you fixing? afaik this doesn't fix a bug, but it does seem like a reasonable sanity check to add, given more widespread use of partial views. > Is this a user error? Or just an internal programming bug. I think if we were to ever hit this it would be indicative of an internal programming bug.
On Thu, Mar 03, 2016 at 11:27:47AM +0000, Auld, Matthew wrote: > > Handle overflow? > > Okay, good idea. > > > Why do it here and not at creation? > > We could, given that we currently only exercise partial views in the gem fault handler code, but as Joonas mentioned we are expecting further use of partial views, so it makes sense to have the check in only one place. More use of broken code? Please review the patches to fix the current implementation first! -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7b8de85..2c49d043 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3493,6 +3493,10 @@ intel_partial_pages(const struct i915_ggtt_view *view, struct sg_page_iter obj_sg_iter; int ret = -ENOMEM; + if (view->params.partial.offset + view->params.partial.size > + obj->base.size >> PAGE_SHIFT) + return ERR_PTR(-EINVAL); + st = kmalloc(sizeof(*st), GFP_KERNEL); if (!st) goto err_st_alloc;
When binding pages for a partial view we should check that the offset + size is valid relative to the size of the gem object. v2: Don't use pages->nents to determine the page count (Tvrtko Ursulin) Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++++ 1 file changed, 4 insertions(+)