Date:   Thu, 2 Dec 2021 12:13:29 -0500
From:   Alex Deucher <alexdeucher@...il.com>
To:     Christian König <christian.koenig@....com>
Cc:     Zhou Qingyang <zhou1615@....edu>, David Airlie <airlied@...ux.ie>,
        "Pan, Xinhui" <Xinhui.Pan@....com>, Kangjie Lu <kjlu@....edu>,
        LKML <linux-kernel@...r.kernel.org>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        Alex Deucher <alexander.deucher@....com>
Subject: Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference
 in radeon_driver_open_kms()

Applied.  Thanks!

Alex

On Wed, Dec 1, 2021 at 10:16 AM Christian König
<christian.koenig@....com> 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@....edu>
> > ---
> > 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@....com>. 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);
>

Powered by blists - more mailing lists