| Message ID | 20211203063257.52053-1-pallavi.mishra@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix possible null ptr dereferences | expand |
On Fri, Dec 03, 2021 at 12:02:57PM +0530, Pallavi Mishra wrote: > add null ptr checks to prevent crash/exceptions. BUG_ON()s aren't going to fix anything. > Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +++ > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 218a9b3037c7..997fe73c205b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area, unsigned long addr, > struct drm_i915_gem_object *obj = > i915_ttm_to_gem(area->vm_private_data); > > + GEM_BUG_ON(!obj); > + > if (i915_gem_object_is_readonly(obj) && write) > return -EACCES; > > @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { > void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) > { > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); > + GEM_BUG_ON(!obj); > > i915_gem_object_release_memory_region(obj); > mutex_destroy(&obj->ttm.get_io_page.lock); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > index 80df9f592407..2b684903a9f5 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct ttm_buffer_object *bo) > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); > int ret; > > + GEM_BUG_ON(!obj); > ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE); > if (ret) > return ret; > @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct i915_ttm_memcpy_arg *arg, > > dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type); > src_reg = i915_ttm_region(bo->bdev, bo->resource->mem_type); > - GEM_BUG_ON(!dst_reg || !src_reg); > + GEM_BUG_ON(!dst_reg || !src_reg || !obj); > > arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ? > ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm) : > -- > 2.25.1
On Fri, 2021-12-03 at 13:02 +0200, Ville Syrjälä wrote: > On Fri, Dec 03, 2021 at 12:02:57PM +0530, Pallavi Mishra wrote: > > add null ptr checks to prevent crash/exceptions. > > BUG_ON()s aren't going to fix anything. > > > Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com> Pallavi, The NULL pointer dereferences here are probably all false positives from a static analyzer. However the GEM_BUG_ONs are fine to assert that the assumption really holds and to clearly point out what's going wrong if they are hit in CI tests. But the commit message must reflect that. /Thomas. > > --- > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +++ > > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > index 218a9b3037c7..997fe73c205b 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area, > > unsigned long addr, > > struct drm_i915_gem_object *obj = > > i915_ttm_to_gem(area->vm_private_data); > > > > + GEM_BUG_ON(!obj); > > + > > if (i915_gem_object_is_readonly(obj) && write) > > return -EACCES; > > > > @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops > > i915_gem_ttm_obj_ops = { > > void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) > > { > > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); > > + GEM_BUG_ON(!obj); > > > > i915_gem_object_release_memory_region(obj); > > mutex_destroy(&obj->ttm.get_io_page.lock); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > > index 80df9f592407..2b684903a9f5 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > > @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct > > ttm_buffer_object *bo) > > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); > > int ret; > > > > + GEM_BUG_ON(!obj); > > ret = i915_gem_object_unbind(obj, > > I915_GEM_OBJECT_UNBIND_ACTIVE); > > if (ret) > > return ret; > > @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct > > i915_ttm_memcpy_arg *arg, > > > > dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type); > > src_reg = i915_ttm_region(bo->bdev, bo->resource- > > >mem_type); > > - GEM_BUG_ON(!dst_reg || !src_reg); > > + GEM_BUG_ON(!dst_reg || !src_reg || !obj); > > > > arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ? > > ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm) > > : > > -- > > 2.25.1 >
On Fri, 03 Dec 2021, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > On Fri, 2021-12-03 at 13:02 +0200, Ville Syrjälä wrote: >> On Fri, Dec 03, 2021 at 12:02:57PM +0530, Pallavi Mishra wrote: >> > add null ptr checks to prevent crash/exceptions. >> >> BUG_ON()s aren't going to fix anything. >> >> > Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com> > > Pallavi, > > The NULL pointer dereferences here are probably all false positives > from a static analyzer. However the GEM_BUG_ONs are fine to assert that > the assumption really holds and to clearly point out what's going wrong > if they are hit in CI tests. I think we're massively overusing GEM_BUG_ON() all over the place. BR, Jani. > > But the commit message must reflect that. > > /Thomas. > > >> > --- >> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +++ >> > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++- >> > 2 files changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> > index 218a9b3037c7..997fe73c205b 100644 >> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> > @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area, >> > unsigned long addr, >> > struct drm_i915_gem_object *obj = >> > i915_ttm_to_gem(area->vm_private_data); >> > >> > + GEM_BUG_ON(!obj); >> > + >> > if (i915_gem_object_is_readonly(obj) && write) >> > return -EACCES; >> > >> > @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops >> > i915_gem_ttm_obj_ops = { >> > void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) >> > { >> > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); >> > + GEM_BUG_ON(!obj); >> > >> > i915_gem_object_release_memory_region(obj); >> > mutex_destroy(&obj->ttm.get_io_page.lock); >> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> > index 80df9f592407..2b684903a9f5 100644 >> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> > @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct >> > ttm_buffer_object *bo) >> > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); >> > int ret; >> > >> > + GEM_BUG_ON(!obj); >> > ret = i915_gem_object_unbind(obj, >> > I915_GEM_OBJECT_UNBIND_ACTIVE); >> > if (ret) >> > return ret; >> > @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct >> > i915_ttm_memcpy_arg *arg, >> > >> > dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type); >> > src_reg = i915_ttm_region(bo->bdev, bo->resource- >> > >mem_type); >> > - GEM_BUG_ON(!dst_reg || !src_reg); >> > + GEM_BUG_ON(!dst_reg || !src_reg || !obj); >> > >> > arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ? >> > ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm) >> > : >> > -- >> > 2.25.1 >> > >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 218a9b3037c7..997fe73c205b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -906,6 +906,8 @@ vm_access_ttm(struct vm_area_struct *area, unsigned long addr, struct drm_i915_gem_object *obj = i915_ttm_to_gem(area->vm_private_data); + GEM_BUG_ON(!obj); + if (i915_gem_object_is_readonly(obj) && write) return -EACCES; @@ -966,6 +968,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { void i915_ttm_bo_destroy(struct ttm_buffer_object *bo) { struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); + GEM_BUG_ON(!obj); i915_gem_object_release_memory_region(obj); mutex_destroy(&obj->ttm.get_io_page.lock); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c index 80df9f592407..2b684903a9f5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c @@ -371,6 +371,7 @@ int i915_ttm_move_notify(struct ttm_buffer_object *bo) struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo); int ret; + GEM_BUG_ON(!obj); ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE); if (ret) return ret; @@ -506,7 +507,7 @@ static void i915_ttm_memcpy_init(struct i915_ttm_memcpy_arg *arg, dst_reg = i915_ttm_region(bo->bdev, dst_mem->mem_type); src_reg = i915_ttm_region(bo->bdev, bo->resource->mem_type); - GEM_BUG_ON(!dst_reg || !src_reg); + GEM_BUG_ON(!dst_reg || !src_reg || !obj); arg->dst_iter = !i915_ttm_cpu_maps_iomem(dst_mem) ? ttm_kmap_iter_tt_init(&arg->_dst_iter.tt, dst_ttm) :
add null ptr checks to prevent crash/exceptions. Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +++ drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-)