Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 98D41C433F5 for ; Thu, 2 Dec 2021 17:13:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1359865AbhLBRRF (ORCPT ); Thu, 2 Dec 2021 12:17:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46636 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235136AbhLBRRD (ORCPT ); Thu, 2 Dec 2021 12:17:03 -0500 Received: from mail-ot1-x329.google.com (mail-ot1-x329.google.com [IPv6:2607:f8b0:4864:20::329]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 36ABBC06174A for ; Thu, 2 Dec 2021 09:13:41 -0800 (PST) Received: by mail-ot1-x329.google.com with SMTP id 35-20020a9d08a6000000b00579cd5e605eso613656otf.0 for ; Thu, 02 Dec 2021 09:13:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=AajSOfABZmizcM6W0Pmxe8k6X7Mucgg0abkivyflOos=; b=bIlQhALzsSpyrYW9oR4N0/2vKAB+viUUzyc3WxBGoTrY1n1CuWkJAJWPyk//ixBkqO Q8mizzVUZP6eHGSldLFeptQbPnchOCZS0L5bdMOYKq7vmpUhSpYTE4rLByZvBCpx3rsd GBJDhjxb9oB7frIH62viMTVgoO2uX1p/kugcDiQKEArIHIE1qeF9onCNFc1zKr2IkZdN ExoRWlOlUChn5wzXFJ9s4M5Csp76CTawiysoyf2iZE7dhn1MuaNPTXvQRGAcvbS4gbdn 4WyAfbS8Y5LS7K7HFcqsUuIPo59XZp0ov4xEUuHTribO/RQRrpvWFsMIY7+ZG+PqfTTA 27tw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=AajSOfABZmizcM6W0Pmxe8k6X7Mucgg0abkivyflOos=; b=ewug5WkMQlM8p5oh8o0jhMMcuGu7xgV7wu3BiMMbkyMLAY/gTObpwYrVHAd3U+kv1/ UQWeE5BtGsxesmAkIwiI+9FYsDlJDqYHySIGzApo9XMJotY/smjT399TrCITHmGwStvG sJvFnMOoN6FBz6VJ1UXDQtU+SkjHDk38gjOv7eNBdFTJ9sGvb7HTJX6Ad37o3Ezg6XSf hDt3v4hR7AZ1zwBIptmlLjPo7JDbdUhjbM4aldAPJzc4MeYGzLHHzE3Ssuw+RWJGWXaB /ZpR4vS8ntyoeoJ+sHyYlCFcgyv4ukdQ6eZgNPqkVIcIm1UlVO46JLyFU90O0Xq7FUHX GtkA== X-Gm-Message-State: AOAM530flzfQMMbnEWq5FqLdnYFI1Styi1R2UoVmVz6bOcEfDYAdP0Bj QBr/lJ3Nnu+h/arvYACHexnlmibXl5r7T3m0m6g= X-Google-Smtp-Source: ABdhPJyY0ZZWWmrlcECjWfIA9f9tGUxAjcFzMuc9v82NaipBZRDDsrY/MKV5w4zTGRjqkwjOCU8VNEXDZDW/t9WcfR0= X-Received: by 2002:a9d:67c1:: with SMTP id c1mr12467723otn.299.1638465220553; Thu, 02 Dec 2021 09:13:40 -0800 (PST) MIME-Version: 1.0 References: <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 Date: Thu, 2 Dec 2021 12:13:29 -0500 Message-ID: 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?= Cc: Zhou Qingyang , David Airlie , "Pan, Xinhui" , Kangjie Lu , LKML , amd-gfx list , Maling list - DRI developers , Alex Deucher Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Applied. Thanks! Alex On Wed, Dec 1, 2021 at 10:16 AM Christian K=C3=B6nig 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 > > --- > > 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 > . 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); >