| Message ID | 20210122173546.531842-2-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/dmabuf: don't trust the dma_buf->size | expand |
Quoting Matthew Auld (2021-01-22 17:35:46) > Throw it into a simple helper, and throw a warning if we encounter an > object which has been initialised with an object size that exceeds our > limit of INT_MAX pages. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +------ > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > drivers/gpu/drm/i915/gem/i915_gem_object.h | 26 +++++++++++++++++++++ > drivers/gpu/drm/i915/gem/i915_gem_region.c | 12 +--------- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 +------------ > 5 files changed, 31 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index dc11497f830b..5cc8a0b2387f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -244,14 +244,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > } > } > > - /* > - * XXX: There is a prevalence of the assumption that we fit the > - * object's page count inside a 32bit _signed_ variable. Let's document > - * this and catch if we ever need to fix it. In the meantime, if you do > - * spot such a local variable, please consider fixing! > - */ > - > - if (dma_buf->size >> PAGE_SHIFT > INT_MAX) > + if (i915_gem_object_size_2big(dma_buf->size)) > return ERR_PTR(-E2BIG); > > /* need to attach */ > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 70f798405f7f..d3702ea8c6aa 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -62,6 +62,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > const struct drm_i915_gem_object_ops *ops, > struct lock_class_key *key) > { > + GEM_CHECK_SIZE_OVERFLOW(obj->base.size); > + > __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key); > > spin_lock_init(&obj->vma.lock); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index d0ae834d787a..d9cef56533ca 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -16,6 +16,32 @@ > #include "i915_gem_gtt.h" > #include "i915_vma_types.h" > > +/* > + * XXX: There is a prevalence of the assumption that we fit the > + * object's page count inside a 32bit _signed_ variable. Let's document > + * this and catch if we ever need to fix it. In the meantime, if you do > + * spot such a local variable, please consider fixing! > + * > + * Aside from our own locals (for which we have no excuse!): > + * - sg_table embeds unsigned int for num_pages > + * - get_user_pages*() mixed ints with longs > + */ > +#define GEM_CHECK_SIZE_OVERFLOW(sz) \ > + GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX) > + > +static inline bool i915_gem_object_size_2big(u64 size) > +{ > + struct drm_i915_gem_object *obj; > + > + if (size >> PAGE_SHIFT > INT_MAX) > + return true; Macro here^ Make it easy to grep, everybody gets a warning as a reminder to fix it. And a prize to whoever does manage to trigger the warning. > + > + if (overflows_type(size, obj->base.size)) > + return true; > + > + return false; > +}
On Fri, 22 Jan 2021 at 17:43, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Matthew Auld (2021-01-22 17:35:46) > > Throw it into a simple helper, and throw a warning if we encounter an > > object which has been initialised with an object size that exceeds our > > limit of INT_MAX pages. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +------ > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 26 +++++++++++++++++++++ > > drivers/gpu/drm/i915/gem/i915_gem_region.c | 12 +--------- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 +------------ > > 5 files changed, 31 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > index dc11497f830b..5cc8a0b2387f 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > @@ -244,14 +244,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > > } > > } > > > > - /* > > - * XXX: There is a prevalence of the assumption that we fit the > > - * object's page count inside a 32bit _signed_ variable. Let's document > > - * this and catch if we ever need to fix it. In the meantime, if you do > > - * spot such a local variable, please consider fixing! > > - */ > > - > > - if (dma_buf->size >> PAGE_SHIFT > INT_MAX) > > + if (i915_gem_object_size_2big(dma_buf->size)) > > return ERR_PTR(-E2BIG); > > > > /* need to attach */ > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > index 70f798405f7f..d3702ea8c6aa 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -62,6 +62,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > > const struct drm_i915_gem_object_ops *ops, > > struct lock_class_key *key) > > { > > + GEM_CHECK_SIZE_OVERFLOW(obj->base.size); > > + > > __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key); > > > > spin_lock_init(&obj->vma.lock); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index d0ae834d787a..d9cef56533ca 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -16,6 +16,32 @@ > > #include "i915_gem_gtt.h" > > #include "i915_vma_types.h" > > > > +/* > > + * XXX: There is a prevalence of the assumption that we fit the > > + * object's page count inside a 32bit _signed_ variable. Let's document > > + * this and catch if we ever need to fix it. In the meantime, if you do > > + * spot such a local variable, please consider fixing! > > + * > > + * Aside from our own locals (for which we have no excuse!): > > + * - sg_table embeds unsigned int for num_pages > > + * - get_user_pages*() mixed ints with longs > > + */ > > +#define GEM_CHECK_SIZE_OVERFLOW(sz) \ > > + GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX) > > + > > +static inline bool i915_gem_object_size_2big(u64 size) > > +{ > > + struct drm_i915_gem_object *obj; > > + > > + if (size >> PAGE_SHIFT > INT_MAX) > > + return true; > > Macro here^ > > Make it easy to grep, everybody gets a warning as a reminder to fix it. > And a prize to whoever does manage to trigger the warning. But this code is user triggable, like with igt("create-massive")? What am I missing? > > > + > > + if (overflows_type(size, obj->base.size)) > > + return true; > > + > > + return false; > > +} > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Matthew Auld (2021-01-22 17:54:16) > On Fri, 22 Jan 2021 at 17:43, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Matthew Auld (2021-01-22 17:35:46) > > > Throw it into a simple helper, and throw a warning if we encounter an > > > object which has been initialised with an object size that exceeds our > > > limit of INT_MAX pages. > > > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +------ > > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 26 +++++++++++++++++++++ > > > drivers/gpu/drm/i915/gem/i915_gem_region.c | 12 +--------- > > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 +------------ > > > 5 files changed, 31 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > > index dc11497f830b..5cc8a0b2387f 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > > @@ -244,14 +244,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > > > } > > > } > > > > > > - /* > > > - * XXX: There is a prevalence of the assumption that we fit the > > > - * object's page count inside a 32bit _signed_ variable. Let's document > > > - * this and catch if we ever need to fix it. In the meantime, if you do > > > - * spot such a local variable, please consider fixing! > > > - */ > > > - > > > - if (dma_buf->size >> PAGE_SHIFT > INT_MAX) > > > + if (i915_gem_object_size_2big(dma_buf->size)) > > > return ERR_PTR(-E2BIG); > > > > > > /* need to attach */ > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > > index 70f798405f7f..d3702ea8c6aa 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > > @@ -62,6 +62,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > > > const struct drm_i915_gem_object_ops *ops, > > > struct lock_class_key *key) > > > { > > > + GEM_CHECK_SIZE_OVERFLOW(obj->base.size); > > > + > > > __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key); > > > > > > spin_lock_init(&obj->vma.lock); > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > index d0ae834d787a..d9cef56533ca 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > @@ -16,6 +16,32 @@ > > > #include "i915_gem_gtt.h" > > > #include "i915_vma_types.h" > > > > > > +/* > > > + * XXX: There is a prevalence of the assumption that we fit the > > > + * object's page count inside a 32bit _signed_ variable. Let's document > > > + * this and catch if we ever need to fix it. In the meantime, if you do > > > + * spot such a local variable, please consider fixing! > > > + * > > > + * Aside from our own locals (for which we have no excuse!): > > > + * - sg_table embeds unsigned int for num_pages > > > + * - get_user_pages*() mixed ints with longs > > > + */ > > > +#define GEM_CHECK_SIZE_OVERFLOW(sz) \ > > > + GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX) > > > + > > > +static inline bool i915_gem_object_size_2big(u64 size) > > > +{ > > > + struct drm_i915_gem_object *obj; > > > + > > > + if (size >> PAGE_SHIFT > INT_MAX) > > > + return true; > > > > Macro here^ > > > > Make it easy to grep, everybody gets a warning as a reminder to fix it. > > And a prize to whoever does manage to trigger the warning. > > But this code is user triggable, like with igt("create-massive")? What > am I missing? GEM_WARN_ON() only emits a warning in debug builds, and we try very hard not to let that be enabled in production. -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index dc11497f830b..5cc8a0b2387f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -244,14 +244,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, } } - /* - * XXX: There is a prevalence of the assumption that we fit the - * object's page count inside a 32bit _signed_ variable. Let's document - * this and catch if we ever need to fix it. In the meantime, if you do - * spot such a local variable, please consider fixing! - */ - - if (dma_buf->size >> PAGE_SHIFT > INT_MAX) + if (i915_gem_object_size_2big(dma_buf->size)) return ERR_PTR(-E2BIG); /* need to attach */ diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 70f798405f7f..d3702ea8c6aa 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -62,6 +62,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, const struct drm_i915_gem_object_ops *ops, struct lock_class_key *key) { + GEM_CHECK_SIZE_OVERFLOW(obj->base.size); + __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key); spin_lock_init(&obj->vma.lock); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index d0ae834d787a..d9cef56533ca 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -16,6 +16,32 @@ #include "i915_gem_gtt.h" #include "i915_vma_types.h" +/* + * XXX: There is a prevalence of the assumption that we fit the + * object's page count inside a 32bit _signed_ variable. Let's document + * this and catch if we ever need to fix it. In the meantime, if you do + * spot such a local variable, please consider fixing! + * + * Aside from our own locals (for which we have no excuse!): + * - sg_table embeds unsigned int for num_pages + * - get_user_pages*() mixed ints with longs + */ +#define GEM_CHECK_SIZE_OVERFLOW(sz) \ + GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX) + +static inline bool i915_gem_object_size_2big(u64 size) +{ + struct drm_i915_gem_object *obj; + + if (size >> PAGE_SHIFT > INT_MAX) + return true; + + if (overflows_type(size, obj->base.size)) + return true; + + return false; +} + void i915_gem_init__objects(struct drm_i915_private *i915); struct drm_i915_gem_object *i915_gem_object_alloc(void); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c index 3e3dad22a683..77dfa908f156 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c @@ -161,17 +161,7 @@ i915_gem_object_create_region(struct intel_memory_region *mem, GEM_BUG_ON(!size); GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_MIN_ALIGNMENT)); - /* - * XXX: There is a prevalence of the assumption that we fit the - * object's page count inside a 32bit _signed_ variable. Let's document - * this and catch if we ever need to fix it. In the meantime, if you do - * spot such a local variable, please consider fixing! - */ - - if (size >> PAGE_SHIFT > INT_MAX) - return ERR_PTR(-E2BIG); - - if (overflows_type(size, obj->base.size)) + if (i915_gem_object_size_2big(size)) return ERR_PTR(-E2BIG); obj = i915_gem_object_alloc(); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index f2eaed6aca3d..3e4785c2dfa2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -770,21 +770,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, I915_USERPTR_UNSYNCHRONIZED)) return -EINVAL; - /* - * XXX: There is a prevalence of the assumption that we fit the - * object's page count inside a 32bit _signed_ variable. Let's document - * this and catch if we ever need to fix it. In the meantime, if you do - * spot such a local variable, please consider fixing! - * - * Aside from our own locals (for which we have no excuse!): - * - sg_table embeds unsigned int for num_pages - * - get_user_pages*() mixed ints with longs - */ - - if (args->user_size >> PAGE_SHIFT > INT_MAX) - return -E2BIG; - - if (overflows_type(args->user_size, obj->base.size)) + if (i915_gem_object_size_2big(args->user_size)) return -E2BIG; if (!args->user_size)
Throw it into a simple helper, and throw a warning if we encounter an object which has been initialised with an object size that exceeds our limit of INT_MAX pages. Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +------ drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 26 +++++++++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_region.c | 12 +--------- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 +------------ 5 files changed, 31 insertions(+), 34 deletions(-)