Messages in this thread | | | Subject | Re: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms() | From | Christian König <> | Date | Tue, 30 Nov 2021 16:37:37 +0100 |
| |
Am 30.11.21 um 16:33 schrieb Zhou Qingyang: > In radeon_driver_open_kms(), radeon_vm_bo_add() is assigned to > vm->ib_bo_va and passes and used in radeon_vm_bo_set_addr(). In > radeon_vm_bo_set_addr(), there is a dereference of vm->ib_bo_va, > which could lead to a NULL pointer dereference on failure of > radeon_vm_bo_add(). > > Fix this bug by adding a check of vm->ib_bo_va. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_RADEON=m show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: cc9e67e3d700 ("drm/radeon: fix VM IB handling") > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu> > --- > Changes in v2: > - Improve the error handling into goto style > > drivers/gpu/drm/radeon/radeon_kms.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c > index 482fb0ae6cb5..e49a9d160e52 100644 > --- a/drivers/gpu/drm/radeon/radeon_kms.c > +++ b/drivers/gpu/drm/radeon/radeon_kms.c > @@ -649,6 +649,8 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) > { > struct radeon_device *rdev = dev->dev_private; > int r; > + struct radeon_fpriv *fpriv; > + struct radeon_vm *vm;
Please keep variables like "i" or "r" declared last.
> > file_priv->driver_priv = NULL; > > @@ -660,8 +662,6 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) > > /* new gpu have virtual address space support */ > if (rdev->family >= CHIP_CAYMAN) { > - struct radeon_fpriv *fpriv; > - struct radeon_vm *vm; > > fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL); > if (unlikely(!fpriv)) { > @@ -673,34 +673,38 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) > vm = &fpriv->vm; > r = radeon_vm_init(rdev, vm); > if (r) { > - kfree(fpriv); > - goto out_suspend; > + goto out_fpriv; > } > > r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false); > if (r) { > - radeon_vm_fini(rdev, vm); > - kfree(fpriv); > - goto out_suspend; > + goto out_vm_fini; > } > > /* map the ib pool buffer read only into > * virtual address space */ > vm->ib_bo_va = radeon_vm_bo_add(rdev, vm, > rdev->ring_tmp_bo.bo); > + if (!vm->ib_bo_va) { > + r = -ENOMEM; > + goto out_vm_fini; > + } > + > r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va, > RADEON_VA_IB_OFFSET, > RADEON_VM_PAGE_READABLE | > RADEON_VM_PAGE_SNOOPED); > if (r) { > - radeon_vm_fini(rdev, vm); > - kfree(fpriv); > - goto out_suspend; > + goto out_vm_fini; > } > } > file_priv->driver_priv = fpriv; > } >
That here won't work.
> +out_vm_fini: > + radeon_vm_fini(rdev, vm); > +out_fpriv: > + kfree(fpriv);
You are finishing the VM and freeing up the memory in the good case now as well.
Christian.
> out_suspend: > pm_runtime_mark_last_busy(dev->dev); > pm_runtime_put_autosuspend(dev->dev);
| |