Applied. Thanks!
Alex
On Wed, Dec 1, 2021 at 10:16 AM Christian König
<christian.koenig@xxxxxxx> wrote:
>
> Am 01.12.21 um 16:13 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@xxxxxxx>
> > ---
> > Changes in v5:
> > - Use conditions to avoid unnecessary initialization
> >
> > Changes in v4:
> > - Initialize the variables to silence warning
> >
> > Changes in v3:
> > - Fix the bug that good case will also be freed
> > - Improve code style
> >
> > Changes in v2:
> > - Improve the error handling into goto style
> >
> > drivers/gpu/drm/radeon/radeon_kms.c | 36 ++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> > index 482fb0ae6cb5..66aee48fd09d 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -648,6 +648,8 @@ void radeon_driver_lastclose_kms(struct drm_device *dev)
> > int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> > {
> > struct radeon_device *rdev = dev->dev_private;
> > + struct radeon_fpriv *fpriv;
> > + struct radeon_vm *vm;
> > int r;
> >
> > 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)) {
> > @@ -672,35 +672,39 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> > if (rdev->accel_working) {
> > vm = &fpriv->vm;
> > r = radeon_vm_init(rdev, vm);
> > - if (r) {
> > - kfree(fpriv);
> > - goto out_suspend;
> > - }
> > + if (r)
> > + 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;
> > - }
> > + if (r)
> > + 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;
> > - }
> > + if (r)
> > + goto out_vm_fini;
> > }
> > file_priv->driver_priv = fpriv;
> > }
> >
> > + if (!r)
>
> I think that test is unecessary now, maybe double check.
>
> Either way patch Reviewed-by: Christian König
> <christian.koenig@xxxxxxx>. Alex will probably pick it up now.
>
> Thanks for the help,
> Christian.
>
> > + goto out_suspend;
> > +
> > +out_vm_fini:
> > + radeon_vm_fini(rdev, vm);
> > +out_fpriv:
> > + kfree(fpriv);
> > out_suspend:
> > pm_runtime_mark_last_busy(dev->dev);
> > pm_runtime_put_autosuspend(dev->dev);
>
