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 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 Cc: llvm@lists.linux.dev, kbuild-all@lists.01.org, < linux-kernel@vger.kernel.org>, 0day robot lkp@intel.com
tree: https://github.com/0day-ci/linux/commits/UPDATE-20211130-233655/Zhou-Qingyan... 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@i... ) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -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/123fb3d217e89c4388fdb08b63991bac7c32... git remote add linux-review https://github.com/0day-ci/linux 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
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, 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); 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
On Wed, Dec 1, 2021 at 3:20 PM Christian König 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 Signed-off-by: Zhou Qingyang 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,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; }+out_vm_fini:
if (r)radeon_vm_fini(rdev, vm);+out_fpriv:
if (r)kfree(fpriv);