MIME-Version: 1.0
References: <e2685075-fbc5-6f36-907f-76b6f76a59ce@amd.com> <20211201151310.177671-1-zhou1615@umn.edu>
 <77fca7d2-b1d8-fe11-322a-3d32f40f6f65@amd.com>
In-Reply-To: <77fca7d2-b1d8-fe11-322a-3d32f40f6f65@amd.com>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Thu, 2 Dec 2021 12:13:29 -0500
Message-ID: <CADnq5_M61Ob29ftgNB7L1eEAb_St1WL1wLEF3C4wXSEFP+3BMw@mail.gmail.com>
Subject: Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference
 in radeon_driver_open_kms()
To:     =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
Cc:     Zhou Qingyang <zhou1615@umn.edu>, David Airlie <airlied@linux.ie>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>, Kangjie Lu <kjlu@umn.edu>,
        LKML <linux-kernel@vger.kernel.org>,
        amd-gfx list <amd-gfx@lists.freedesktop.org>,
        Maling list - DRI developers 
        <dri-devel@lists.freedesktop.org>,
        Alex Deucher <alexander.deucher@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Applied.  Thanks!

Alex

On Wed, Dec 1, 2021 at 10:16 AM Christian K=C3=B6nig
<christian.koenig@amd.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=3Dm 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 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/rade=
on/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 *f=
ile_priv)
> >   {
> >       struct radeon_device *rdev =3D dev->dev_private;
> > +     struct radeon_fpriv *fpriv;
> > +     struct radeon_vm *vm;
> >       int r;
> >
> >       file_priv->driver_priv =3D 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 >=3D CHIP_CAYMAN) {
> > -             struct radeon_fpriv *fpriv;
> > -             struct radeon_vm *vm;
> >
> >               fpriv =3D 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 =3D &fpriv->vm;
> >                       r =3D radeon_vm_init(rdev, vm);
> > -                     if (r) {
> > -                             kfree(fpriv);
> > -                             goto out_suspend;
> > -                     }
> > +                     if (r)
> > +                             goto out_fpriv;
> >
> >                       r =3D radeon_bo_reserve(rdev->ring_tmp_bo.bo, fal=
se);
> > -                     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 =3D radeon_vm_bo_add(rdev, vm,
> >                                                       rdev->ring_tmp_bo=
.bo);
> > +                     if (!vm->ib_bo_va) {
> > +                             r =3D -ENOMEM;
> > +                             goto out_vm_fini;
> > +                     }
> > +
> >                       r =3D 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 =3D fpriv;
> >       }
> >
> > +     if (!r)
>
> I think that test is unecessary now, maybe double check.
>
> Either way patch Reviewed-by: Christian K=C3=B6nig
> <christian.koenig@amd.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);
>