From: "Christian König" <christian.koenig@amd.com>
To: Qingyang Zhou <zhou1615@umn.edu>
Cc: kernel test robot <lkp@intel.com>,
David Airlie <airlied@linux.ie>,
"Pan, Xinhui" <Xinhui.Pan@amd.com>, Kangjie Lu <kjlu@umn.edu>,
linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org,
Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
Date: Wed, 1 Dec 2021 14:08:06 +0100 [thread overview]
Message-ID: <e2685075-fbc5-6f36-907f-76b6f76a59ce@amd.com> (raw)
In-Reply-To: <CA+Cm_xRjYWun_u7rod9X=c0-WExb798TLGJ6fMJMBqOMxAeuHw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 22394 bytes --]
Hi Zhou,
mhm I see. Problem is that zero initializing things just to silence the
warning is considered really bad practice.
Have you tried to use "goto out_suspend" instead of the "if(r)" at the
end of the good case?
That might silence the compiler warning, but might look a bit better
than initializing all the local variables.
Christian.
Am 01.12.21 um 13:48 schrieb Qingyang Zhou:
> Hi Christian:
>
> The warning is from the kernel test robot, I quote it here. The key
> point is that vm may be used in radeon_vm_fini(rdev, vm) without
> initialization. Although it is not possible in practice, I still add
> initializations to silence the warning of llvm.
>
> ---------- Forwarded message ---------
> From: *kernel test robot* <lkp@intel.com <mailto:lkp@intel.com>>
> Date: Wed, Dec 1, 2021 at 4:32 AM
> Subject: drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable
> 'vm' is used uninitialized whenever 'if' condition is false
> To: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
> Cc: <llvm@lists.linux.dev <mailto:llvm@lists.linux.dev>>,
> <kbuild-all@lists.01.org <mailto:kbuild-all@lists.01.org>>,
> <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>,
> 0day robot <lkp@intel.com <mailto:lkp@intel.com>>
>
>
> tree:
> https://github.com/0day-ci/linux/commits/UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FUPDATE-20211130-233655%2FZhou-Qingyang%2Fdrm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms%2F20211130-231509&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241621159%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=xidj6HQeF%2BN4Iaf%2BlsTNyLtUi5O6RfPQXdIP%2FSiDTaA%3D&reserved=0>
> head: 123fb3d217e89c4388fdb08b63991bac7c324219
> commit: 123fb3d217e89c4388fdb08b63991bac7c324219
> drm/radeon/radeon_kms: Fix a NULL pointer dereference in
> radeon_driver_open_kms()
> date: 5 hours ago
> config: mips-randconfig-r014-20211128
> (https://download.01.org/0day-ci/archive/20211201/202112010420.xkXHciHS-lkp@intel.com/config
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdownload.01.org%2F0day-ci%2Farchive%2F20211201%2F202112010420.xkXHciHS-lkp%40intel.com%2Fconfig&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241631158%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bFtNPX12YoMNV5vmbsU0Yqctl9bM4%2BHwr844eDiNJ9Y%3D&reserved=0>)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241641152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=02UXY8X18bDlrLzP2ShvfHZLzNMhveWG3ax6jaYjQ8g%3D&reserved=0> 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.githubusercontent.com%2Fintel%2Flkp-tests%2Fmaster%2Fsbin%2Fmake.cross&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241641152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=2Ynt8wRS%2FegIFRy7OXHVvkto8skoNpI6n6nB8XPyBFY%3D&reserved=0> -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install mips cross compiling tool for clang build
> # apt-get install binutils-mips-linux-gnu
> #
> https://github.com/0day-ci/linux/commit/123fb3d217e89c4388fdb08b63991bac7c324219
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommit%2F123fb3d217e89c4388fdb08b63991bac7c324219&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241651152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=WiHolpngKWiQ3N2ztZnBMJpHk8K8dVGyG69UVboG1fc%3D&reserved=0>
> git remote add linux-review https://github.com/0day-ci/linux
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241661151%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NneL5vqGdpoQ%2Fmy%2Fr36qZjBr4lmrRszvC1S1hL2xsMM%3D&reserved=0>
> git fetch --no-tags linux-review
> UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
> git checkout 123fb3d217e89c4388fdb08b63991bac7c324219
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/drm/radeon/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com <mailto:lkp@intel.com>>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable 'vm'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> if (rdev->accel_working) {
> ^~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use
> occurs here
> radeon_vm_fini(rdev, vm);
> ^~
> drivers/gpu/drm/radeon/radeon_kms.c:672:3: note: remove the 'if' if
> its condition is always true
> if (rdev->accel_working) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:664:6: warning: variable 'vm'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> if (rdev->family >= CHIP_CAYMAN) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use
> occurs here
> radeon_vm_fini(rdev, vm);
> ^~
> drivers/gpu/drm/radeon/radeon_kms.c:664:2: note: remove the 'if' if
> its condition is always true
> if (rdev->family >= CHIP_CAYMAN) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:653:22: note: initialize the
> variable 'vm' to silence this warning
> struct radeon_vm *vm;
> ^
> = NULL
> 2 warnings generated.
>
>
> vim +672 drivers/gpu/drm/radeon/radeon_kms.c
>
> 771fe6b912fca54 Jerome Glisse 2009-06-05 638
> f482a1419545ded Alex Deucher 2012-07-17 639 /**
> f482a1419545ded Alex Deucher 2012-07-17 640 *
> radeon_driver_open_kms - drm callback for open
> f482a1419545ded Alex Deucher 2012-07-17 641 *
> f482a1419545ded Alex Deucher 2012-07-17 642 * @dev: drm dev
> pointer
> f482a1419545ded Alex Deucher 2012-07-17 643 * @file_priv: drm
> file
> f482a1419545ded Alex Deucher 2012-07-17 644 *
> f482a1419545ded Alex Deucher 2012-07-17 645 * On device open,
> init vm on cayman+ (all asics).
> f482a1419545ded Alex Deucher 2012-07-17 646 * Returns 0 on
> success, error on failure.
> f482a1419545ded Alex Deucher 2012-07-17 647 */
> 771fe6b912fca54 Jerome Glisse 2009-06-05 648 int
> radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> 771fe6b912fca54 Jerome Glisse 2009-06-05 649 {
> 721604a15b934f0 Jerome Glisse 2012-01-05 650 struct
> radeon_device *rdev = dev->dev_private;
> 10ebc0bc09344ab Dave Airlie 2012-09-17 651 int r;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 652 struct radeon_fpriv
> *fpriv;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 653 struct radeon_vm *vm;
> 721604a15b934f0 Jerome Glisse 2012-01-05 654
> 721604a15b934f0 Jerome Glisse 2012-01-05 655
> file_priv->driver_priv = NULL;
> 721604a15b934f0 Jerome Glisse 2012-01-05 656
> 10ebc0bc09344ab Dave Airlie 2012-09-17 657 r =
> pm_runtime_get_sync(dev->dev);
> 9fb10671011143d Aditya Pakki 2020-06-13 658 if (r < 0) {
> 9fb10671011143d Aditya Pakki 2020-06-13 659
> pm_runtime_put_autosuspend(dev->dev);
> 10ebc0bc09344ab Dave Airlie 2012-09-17 660 return r;
> 9fb10671011143d Aditya Pakki 2020-06-13 661 }
> 10ebc0bc09344ab Dave Airlie 2012-09-17 662
> 721604a15b934f0 Jerome Glisse 2012-01-05 663 /* new gpu
> have virtual address space support */
> 721604a15b934f0 Jerome Glisse 2012-01-05 664 if
> (rdev->family >= CHIP_CAYMAN) {
> 721604a15b934f0 Jerome Glisse 2012-01-05 665
> 721604a15b934f0 Jerome Glisse 2012-01-05 666 fpriv =
> kzalloc(sizeof(*fpriv), GFP_KERNEL);
> 721604a15b934f0 Jerome Glisse 2012-01-05 667 if
> (unlikely(!fpriv)) {
> 32c59dc14b72803 Alex Deucher 2016-08-31 668 r =
> -ENOMEM;
> 32c59dc14b72803 Alex Deucher 2016-08-31 669 goto
> out_suspend;
> 721604a15b934f0 Jerome Glisse 2012-01-05 670 }
> 721604a15b934f0 Jerome Glisse 2012-01-05 671
> 544143f9e01a60a Alex Deucher 2015-01-28 @672 if
> (rdev->accel_working) {
> cc9e67e3d7000c1 Christian König 2014-07-18 673 vm =
> &fpriv->vm;
> cc9e67e3d7000c1 Christian König 2014-07-18 674 r =
> radeon_vm_init(rdev, vm);
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 675 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 676
> goto out_fpriv;
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 677 }
> d72d43cfc5847c1 Christian König 2012-10-09 678
> f1e3dc708aaadb9 Christian König 2014-02-20 679 r =
> radeon_bo_reserve(rdev->ring_tmp_bo.bo
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241661151%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=u%2FJICBkdCMwgZH0zsmtS%2F2kQTtkwatlaQ51hnB8C1xo%3D&reserved=0>,
> false);
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 680 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 681
> goto out_vm_fini;
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 682 }
> f1e3dc708aaadb9 Christian König 2014-02-20 683
> d72d43cfc5847c1 Christian König 2012-10-09 684 /* map
> the ib pool buffer read only into
> d72d43cfc5847c1 Christian König 2012-10-09 685 *
> virtual address space */
> cc9e67e3d7000c1 Christian König 2014-07-18 686
> vm->ib_bo_va = radeon_vm_bo_add(rdev, vm,
> d72d43cfc5847c1 Christian König 2012-10-09 687
> rdev->ring_tmp_bo.bo
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241671146%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Xci0wMHbXbhIcvQuZXPCdhhNXCOls%2BjoEhOVUPUQLhE%3D&reserved=0>);
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 688 if
> (!vm->ib_bo_va) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 689
> r = -ENOMEM;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 690
> goto out_vm_fini;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 691 }
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 692
> cc9e67e3d7000c1 Christian König 2014-07-18 693 r =
> radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
> cc9e67e3d7000c1 Christian König 2014-07-18 694
> RADEON_VA_IB_OFFSET,
> d72d43cfc5847c1 Christian König 2012-10-09 695
> RADEON_VM_PAGE_READABLE |
> d72d43cfc5847c1 Christian König 2012-10-09 696
> RADEON_VM_PAGE_SNOOPED);
> 721604a15b934f0 Jerome Glisse 2012-01-05 697 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 698
> goto out_vm_fini;
> 721604a15b934f0 Jerome Glisse 2012-01-05 699 }
> 24f47acc78b0ab5 Jérôme Glisse 2014-05-07 700 }
> 721604a15b934f0 Jerome Glisse 2012-01-05 701
> file_priv->driver_priv = fpriv;
> 721604a15b934f0 Jerome Glisse 2012-01-05 702 }
> 10ebc0bc09344ab Dave Airlie 2012-09-17 703
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 704 out_vm_fini:
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 705
> radeon_vm_fini(rdev, vm);
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 706 out_fpriv:
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 707 kfree(fpriv);
> 32c59dc14b72803 Alex Deucher 2016-08-31 708 out_suspend:
> 10ebc0bc09344ab Dave Airlie 2012-09-17 709
> pm_runtime_mark_last_busy(dev->dev);
> 10ebc0bc09344ab Dave Airlie 2012-09-17 710
> pm_runtime_put_autosuspend(dev->dev);
> 32c59dc14b72803 Alex Deucher 2016-08-31 711 return r;
> 771fe6b912fca54 Jerome Glisse 2009-06-05 712 }
> 771fe6b912fca54 Jerome Glisse 2009-06-05 713
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fhyperkitty%2Flist%2Fkbuild-all%40lists.01.org&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241681142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PxpqV%2Fgh3oZuixBzm0aZxnoy3NAJzdxW7R9fOkqD8pQ%3D&reserved=0>
>
> On Wed, Dec 1, 2021 at 3:20 PM Christian König
> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>
> Am 01.12.21 um 04:22 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")
> > Reported-by: kernel test robot <lkp@intel.com
> <mailto:lkp@intel.com>>
> > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu
> <mailto:zhou1615@umn.edu>>
> > ---
> > Changes in v2:
> > - Initialize the variables to silence warning
>
> What warning do you get? Double checking the code that shouldn't be
> necessary and is usually rather frowned upon.
>
> Thanks,
> Christian.
>
> >
> > 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 | 37
> ++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
> b/drivers/gpu/drm/radeon/radeon_kms.c
> > index 482fb0ae6cb5..9d0f840286a1 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -648,7 +648,9 @@ 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;
> > - int r;
> > + struct radeon_fpriv *fpriv = NULL;
> > + struct radeon_vm *vm = NULL;
> > + int r = 0;
> >
> > 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,38 @@ 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
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241681142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=H7WLEFylDXgOQLTAa5rPfEfbU8LtsqRxnGG8hWjI5IQ%3D&reserved=0>,
> 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
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241691135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=SSR7%2FvFI3gwMRiolWqM79J47Mje8Yz6B%2Ba6jmeX%2FA0E%3D&reserved=0>);
> > + 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;
> > }
> >
> > +out_vm_fini:
> > + if (r)
> > + radeon_vm_fini(rdev, vm);
> > +out_fpriv:
> > + if (r)
> > + kfree(fpriv);
> > out_suspend:
> > pm_runtime_mark_last_busy(dev->dev);
> > pm_runtime_put_autosuspend(dev->dev);
>
[-- Attachment #2: Type: text/html, Size: 40154 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com>
To: Qingyang Zhou <zhou1615@umn.edu>
Cc: kernel test robot <lkp@intel.com>,
David Airlie <airlied@linux.ie>,
"Pan, Xinhui" <Xinhui.Pan@amd.com>, Kangjie Lu <kjlu@umn.edu>,
linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
dri-devel@lists.freedesktop.org, Daniel Vetter <daniel@ffwll.ch>,
Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
Date: Wed, 1 Dec 2021 14:08:06 +0100 [thread overview]
Message-ID: <e2685075-fbc5-6f36-907f-76b6f76a59ce@amd.com> (raw)
In-Reply-To: <CA+Cm_xRjYWun_u7rod9X=c0-WExb798TLGJ6fMJMBqOMxAeuHw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 22394 bytes --]
Hi Zhou,
mhm I see. Problem is that zero initializing things just to silence the
warning is considered really bad practice.
Have you tried to use "goto out_suspend" instead of the "if(r)" at the
end of the good case?
That might silence the compiler warning, but might look a bit better
than initializing all the local variables.
Christian.
Am 01.12.21 um 13:48 schrieb Qingyang Zhou:
> Hi Christian:
>
> The warning is from the kernel test robot, I quote it here. The key
> point is that vm may be used in radeon_vm_fini(rdev, vm) without
> initialization. Although it is not possible in practice, I still add
> initializations to silence the warning of llvm.
>
> ---------- Forwarded message ---------
> From: *kernel test robot* <lkp@intel.com <mailto:lkp@intel.com>>
> Date: Wed, Dec 1, 2021 at 4:32 AM
> Subject: drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable
> 'vm' is used uninitialized whenever 'if' condition is false
> To: Zhou Qingyang <zhou1615@umn.edu <mailto:zhou1615@umn.edu>>
> Cc: <llvm@lists.linux.dev <mailto:llvm@lists.linux.dev>>,
> <kbuild-all@lists.01.org <mailto:kbuild-all@lists.01.org>>,
> <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>,
> 0day robot <lkp@intel.com <mailto:lkp@intel.com>>
>
>
> tree:
> https://github.com/0day-ci/linux/commits/UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FUPDATE-20211130-233655%2FZhou-Qingyang%2Fdrm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms%2F20211130-231509&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241621159%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=xidj6HQeF%2BN4Iaf%2BlsTNyLtUi5O6RfPQXdIP%2FSiDTaA%3D&reserved=0>
> head: 123fb3d217e89c4388fdb08b63991bac7c324219
> commit: 123fb3d217e89c4388fdb08b63991bac7c324219
> drm/radeon/radeon_kms: Fix a NULL pointer dereference in
> radeon_driver_open_kms()
> date: 5 hours ago
> config: mips-randconfig-r014-20211128
> (https://download.01.org/0day-ci/archive/20211201/202112010420.xkXHciHS-lkp@intel.com/config
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdownload.01.org%2F0day-ci%2Farchive%2F20211201%2F202112010420.xkXHciHS-lkp%40intel.com%2Fconfig&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241631158%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bFtNPX12YoMNV5vmbsU0Yqctl9bM4%2BHwr844eDiNJ9Y%3D&reserved=0>)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241641152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=02UXY8X18bDlrLzP2ShvfHZLzNMhveWG3ax6jaYjQ8g%3D&reserved=0> 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.githubusercontent.com%2Fintel%2Flkp-tests%2Fmaster%2Fsbin%2Fmake.cross&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241641152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=2Ynt8wRS%2FegIFRy7OXHVvkto8skoNpI6n6nB8XPyBFY%3D&reserved=0> -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install mips cross compiling tool for clang build
> # apt-get install binutils-mips-linux-gnu
> #
> https://github.com/0day-ci/linux/commit/123fb3d217e89c4388fdb08b63991bac7c324219
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommit%2F123fb3d217e89c4388fdb08b63991bac7c324219&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241651152%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=WiHolpngKWiQ3N2ztZnBMJpHk8K8dVGyG69UVboG1fc%3D&reserved=0>
> git remote add linux-review https://github.com/0day-ci/linux
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241661151%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NneL5vqGdpoQ%2Fmy%2Fr36qZjBr4lmrRszvC1S1hL2xsMM%3D&reserved=0>
> git fetch --no-tags linux-review
> UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
> git checkout 123fb3d217e89c4388fdb08b63991bac7c324219
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/drm/radeon/
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com <mailto:lkp@intel.com>>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable 'vm'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> if (rdev->accel_working) {
> ^~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use
> occurs here
> radeon_vm_fini(rdev, vm);
> ^~
> drivers/gpu/drm/radeon/radeon_kms.c:672:3: note: remove the 'if' if
> its condition is always true
> if (rdev->accel_working) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:664:6: warning: variable 'vm'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> if (rdev->family >= CHIP_CAYMAN) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use
> occurs here
> radeon_vm_fini(rdev, vm);
> ^~
> drivers/gpu/drm/radeon/radeon_kms.c:664:2: note: remove the 'if' if
> its condition is always true
> if (rdev->family >= CHIP_CAYMAN) {
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/radeon/radeon_kms.c:653:22: note: initialize the
> variable 'vm' to silence this warning
> struct radeon_vm *vm;
> ^
> = NULL
> 2 warnings generated.
>
>
> vim +672 drivers/gpu/drm/radeon/radeon_kms.c
>
> 771fe6b912fca54 Jerome Glisse 2009-06-05 638
> f482a1419545ded Alex Deucher 2012-07-17 639 /**
> f482a1419545ded Alex Deucher 2012-07-17 640 *
> radeon_driver_open_kms - drm callback for open
> f482a1419545ded Alex Deucher 2012-07-17 641 *
> f482a1419545ded Alex Deucher 2012-07-17 642 * @dev: drm dev
> pointer
> f482a1419545ded Alex Deucher 2012-07-17 643 * @file_priv: drm
> file
> f482a1419545ded Alex Deucher 2012-07-17 644 *
> f482a1419545ded Alex Deucher 2012-07-17 645 * On device open,
> init vm on cayman+ (all asics).
> f482a1419545ded Alex Deucher 2012-07-17 646 * Returns 0 on
> success, error on failure.
> f482a1419545ded Alex Deucher 2012-07-17 647 */
> 771fe6b912fca54 Jerome Glisse 2009-06-05 648 int
> radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> 771fe6b912fca54 Jerome Glisse 2009-06-05 649 {
> 721604a15b934f0 Jerome Glisse 2012-01-05 650 struct
> radeon_device *rdev = dev->dev_private;
> 10ebc0bc09344ab Dave Airlie 2012-09-17 651 int r;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 652 struct radeon_fpriv
> *fpriv;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 653 struct radeon_vm *vm;
> 721604a15b934f0 Jerome Glisse 2012-01-05 654
> 721604a15b934f0 Jerome Glisse 2012-01-05 655
> file_priv->driver_priv = NULL;
> 721604a15b934f0 Jerome Glisse 2012-01-05 656
> 10ebc0bc09344ab Dave Airlie 2012-09-17 657 r =
> pm_runtime_get_sync(dev->dev);
> 9fb10671011143d Aditya Pakki 2020-06-13 658 if (r < 0) {
> 9fb10671011143d Aditya Pakki 2020-06-13 659
> pm_runtime_put_autosuspend(dev->dev);
> 10ebc0bc09344ab Dave Airlie 2012-09-17 660 return r;
> 9fb10671011143d Aditya Pakki 2020-06-13 661 }
> 10ebc0bc09344ab Dave Airlie 2012-09-17 662
> 721604a15b934f0 Jerome Glisse 2012-01-05 663 /* new gpu
> have virtual address space support */
> 721604a15b934f0 Jerome Glisse 2012-01-05 664 if
> (rdev->family >= CHIP_CAYMAN) {
> 721604a15b934f0 Jerome Glisse 2012-01-05 665
> 721604a15b934f0 Jerome Glisse 2012-01-05 666 fpriv =
> kzalloc(sizeof(*fpriv), GFP_KERNEL);
> 721604a15b934f0 Jerome Glisse 2012-01-05 667 if
> (unlikely(!fpriv)) {
> 32c59dc14b72803 Alex Deucher 2016-08-31 668 r =
> -ENOMEM;
> 32c59dc14b72803 Alex Deucher 2016-08-31 669 goto
> out_suspend;
> 721604a15b934f0 Jerome Glisse 2012-01-05 670 }
> 721604a15b934f0 Jerome Glisse 2012-01-05 671
> 544143f9e01a60a Alex Deucher 2015-01-28 @672 if
> (rdev->accel_working) {
> cc9e67e3d7000c1 Christian König 2014-07-18 673 vm =
> &fpriv->vm;
> cc9e67e3d7000c1 Christian König 2014-07-18 674 r =
> radeon_vm_init(rdev, vm);
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 675 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 676
> goto out_fpriv;
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 677 }
> d72d43cfc5847c1 Christian König 2012-10-09 678
> f1e3dc708aaadb9 Christian König 2014-02-20 679 r =
> radeon_bo_reserve(rdev->ring_tmp_bo.bo
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241661151%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=u%2FJICBkdCMwgZH0zsmtS%2F2kQTtkwatlaQ51hnB8C1xo%3D&reserved=0>,
> false);
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 680 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 681
> goto out_vm_fini;
> 74073c9dd299056 Quentin Casasnovas 2014-03-18 682 }
> f1e3dc708aaadb9 Christian König 2014-02-20 683
> d72d43cfc5847c1 Christian König 2012-10-09 684 /* map
> the ib pool buffer read only into
> d72d43cfc5847c1 Christian König 2012-10-09 685 *
> virtual address space */
> cc9e67e3d7000c1 Christian König 2014-07-18 686
> vm->ib_bo_va = radeon_vm_bo_add(rdev, vm,
> d72d43cfc5847c1 Christian König 2012-10-09 687
> rdev->ring_tmp_bo.bo
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241671146%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Xci0wMHbXbhIcvQuZXPCdhhNXCOls%2BjoEhOVUPUQLhE%3D&reserved=0>);
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 688 if
> (!vm->ib_bo_va) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 689
> r = -ENOMEM;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 690
> goto out_vm_fini;
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 691 }
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 692
> cc9e67e3d7000c1 Christian König 2014-07-18 693 r =
> radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
> cc9e67e3d7000c1 Christian König 2014-07-18 694
> RADEON_VA_IB_OFFSET,
> d72d43cfc5847c1 Christian König 2012-10-09 695
> RADEON_VM_PAGE_READABLE |
> d72d43cfc5847c1 Christian König 2012-10-09 696
> RADEON_VM_PAGE_SNOOPED);
> 721604a15b934f0 Jerome Glisse 2012-01-05 697 if (r) {
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 698
> goto out_vm_fini;
> 721604a15b934f0 Jerome Glisse 2012-01-05 699 }
> 24f47acc78b0ab5 Jérôme Glisse 2014-05-07 700 }
> 721604a15b934f0 Jerome Glisse 2012-01-05 701
> file_priv->driver_priv = fpriv;
> 721604a15b934f0 Jerome Glisse 2012-01-05 702 }
> 10ebc0bc09344ab Dave Airlie 2012-09-17 703
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 704 out_vm_fini:
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 705
> radeon_vm_fini(rdev, vm);
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 706 out_fpriv:
> 123fb3d217e89c4 Zhou Qingyang 2021-11-30 707 kfree(fpriv);
> 32c59dc14b72803 Alex Deucher 2016-08-31 708 out_suspend:
> 10ebc0bc09344ab Dave Airlie 2012-09-17 709
> pm_runtime_mark_last_busy(dev->dev);
> 10ebc0bc09344ab Dave Airlie 2012-09-17 710
> pm_runtime_put_autosuspend(dev->dev);
> 32c59dc14b72803 Alex Deucher 2016-08-31 711 return r;
> 771fe6b912fca54 Jerome Glisse 2009-06-05 712 }
> 771fe6b912fca54 Jerome Glisse 2009-06-05 713
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fhyperkitty%2Flist%2Fkbuild-all%40lists.01.org&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241681142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PxpqV%2Fgh3oZuixBzm0aZxnoy3NAJzdxW7R9fOkqD8pQ%3D&reserved=0>
>
> On Wed, Dec 1, 2021 at 3:20 PM Christian König
> <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> wrote:
>
> Am 01.12.21 um 04:22 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")
> > Reported-by: kernel test robot <lkp@intel.com
> <mailto:lkp@intel.com>>
> > Signed-off-by: Zhou Qingyang <zhou1615@umn.edu
> <mailto:zhou1615@umn.edu>>
> > ---
> > Changes in v2:
> > - Initialize the variables to silence warning
>
> What warning do you get? Double checking the code that shouldn't be
> necessary and is usually rather frowned upon.
>
> Thanks,
> Christian.
>
> >
> > 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 | 37
> ++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
> b/drivers/gpu/drm/radeon/radeon_kms.c
> > index 482fb0ae6cb5..9d0f840286a1 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -648,7 +648,9 @@ 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;
> > - int r;
> > + struct radeon_fpriv *fpriv = NULL;
> > + struct radeon_vm *vm = NULL;
> > + int r = 0;
> >
> > 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,38 @@ 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
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241681142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=H7WLEFylDXgOQLTAa5rPfEfbU8LtsqRxnGG8hWjI5IQ%3D&reserved=0>,
> 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
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fring_tmp_bo.bo%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Cd8f40721cdb04ff26c4e08d9b4c8f26a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637739602241691135%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=SSR7%2FvFI3gwMRiolWqM79J47Mje8Yz6B%2Ba6jmeX%2FA0E%3D&reserved=0>);
> > + 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;
> > }
> >
> > +out_vm_fini:
> > + if (r)
> > + radeon_vm_fini(rdev, vm);
> > +out_fpriv:
> > + if (r)
> > + kfree(fpriv);
> > out_suspend:
> > pm_runtime_mark_last_busy(dev->dev);
> > pm_runtime_put_autosuspend(dev->dev);
>
[-- Attachment #2: Type: text/html, Size: 40154 bytes --]
next prev parent reply other threads:[~2021-12-01 13:08 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-30 15:04 [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms() Zhou Qingyang
2021-11-30 15:04 ` Zhou Qingyang
2021-11-30 15:04 ` Zhou Qingyang
2021-11-30 15:11 ` Christian König
2021-11-30 15:11 ` Christian König
2021-11-30 15:11 ` Christian König
2021-11-30 15:33 ` [PATCH v2] " Zhou Qingyang
2021-11-30 15:33 ` Zhou Qingyang
2021-11-30 15:33 ` Zhou Qingyang
2021-11-30 15:37 ` Christian König
2021-11-30 15:37 ` Christian König
2021-11-30 15:37 ` Christian König
2021-11-30 15:57 ` [PATCH v3] " Zhou Qingyang
2021-11-30 15:57 ` Zhou Qingyang
2021-11-30 15:57 ` Zhou Qingyang
2021-12-01 3:22 ` [PATCH v4] " Zhou Qingyang
2021-12-01 3:22 ` Zhou Qingyang
2021-12-01 3:22 ` Zhou Qingyang
2021-12-01 7:20 ` Christian König
2021-12-01 7:20 ` Christian König
2021-12-01 7:20 ` Christian König
2021-12-01 12:48 ` Qingyang Zhou
2021-12-01 12:48 ` Qingyang Zhou
2021-12-01 13:08 ` Christian König [this message]
2021-12-01 13:08 ` Christian König
2021-12-01 15:13 ` [PATCH v5] " Zhou Qingyang
2021-12-01 15:13 ` Zhou Qingyang
2021-12-01 15:13 ` Zhou Qingyang
2021-12-01 15:15 ` Christian König
2021-12-01 15:15 ` Christian König
2021-12-01 15:15 ` Christian König
2021-12-02 17:13 ` Alex Deucher
2021-12-02 17:13 ` Alex Deucher
2021-12-01 6:57 ` [PATCH v3] " Christian König
2021-12-01 6:57 ` Christian König
2021-12-01 6:57 ` Christian König
2021-12-01 3:23 [PATCH v4] " Zhou Qingyang
2021-12-01 3:23 ` Zhou Qingyang
2021-12-01 3:23 ` Zhou Qingyang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e2685075-fbc5-6f36-907f-76b6f76a59ce@amd.com \
--to=christian.koenig@amd.com \
--cc=Xinhui.Pan@amd.com \
--cc=airlied@linux.ie \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=kjlu@umn.edu \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
--cc=zhou1615@umn.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.