[PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

List overview All Threads
Download

newer

older

[PATCH 1/8] drm/i915/migrate:...

[PATCH] fix a NULL pointer...

Zhou Qingyang

30 Nov 2021 30 Nov '21

3:04 p.m.

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@umn.edu --- drivers/gpu/drm/radeon/radeon_kms.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..ead015c055fb 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -688,6 +688,13 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) * 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; + radeon_vm_fini(rdev, vm); + kfree(fpriv); + goto out_suspend; + } + r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va, RADEON_VA_IB_OFFSET, RADEON_VM_PAGE_READABLE |

-- 2.25.1

Show replies by date

Christian König

30 Nov 30 Nov

3:11 p.m.

Am 30.11.21 um 16:04 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@umn.edu

drivers/gpu/drm/radeon/radeon_kms.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..ead015c055fb 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -688,6 +688,13 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) * 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;
		radeon_vm_fini(rdev, vm);
		kfree(fpriv);
		goto out_suspend;
	}

Impressive catch for an automated checker.

Please improve the error handling into goto style since we now add the fourth instance of the same error handling code. Apart from that looks good to me.

Thanks, Christian.

...

	r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
				  RADEON_VA_IB_OFFSET,
				  RADEON_VM_PAGE_READABLE |

Zhou Qingyang

3:33 p.m.

New subject: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Fix this bug by adding a check of vm->ib_bo_va.

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@umn.edu --- Changes in v2: - Improve the error handling into goto style

drivers/gpu/drm/radeon/radeon_kms.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..e49a9d160e52 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -649,6 +649,8 @@ 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; + struct radeon_vm *vm;

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)) { @@ -673,34 +673,38 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) vm = &fpriv->vm; r = radeon_vm_init(rdev, vm); if (r) { - kfree(fpriv); - goto out_suspend; + 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; + 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; + goto out_vm_fini; } } file_priv->driver_priv = fpriv; }

+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);

-- 2.25.1

Christian König

3:37 p.m.

New subject: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Am 30.11.21 um 16:33 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@umn.edu

Changes in v2:

Improve the error handling into goto style

drivers/gpu/drm/radeon/radeon_kms.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..e49a9d160e52 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -649,6 +649,8 @@ 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;

struct radeon_vm *vm;

Please keep variables like "i" or "r" declared last.

...

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)) {
@@ -673,34 +673,38 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) vm = &fpriv->vm; r = radeon_vm_init(rdev, vm); if (r) {
		kfree(fpriv);
		goto out_suspend;
		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;
		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;
		goto out_vm_fini;
}
} file_priv->driver_priv = fpriv; }

That here won't work.

...

+out_vm_fini:

radeon_vm_fini(rdev, vm);

+out_fpriv:

kfree(fpriv);

You are finishing the VM and freeing up the memory in the good case now as well.

Christian.

...

out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

Zhou Qingyang

3:57 p.m.

New subject: [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Fix this bug by adding a check of vm->ib_bo_va.

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@umn.edu --- 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 | 35 ++++++++++++++++------------- 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..439f4d1fdd65 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,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); out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

-- 2.25.1

Zhou Qingyang

1 Dec 1 Dec

3:22 a.m.

New subject: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Fix this bug by adding a check of vm->ib_bo_va.

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

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;

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); out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

-- 2.25.1

Christian König

7:20 a.m.

New subject: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

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);
out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

Qingyang Zhou

12:48 p.m.

New subject: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

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);
out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

Christian König

1:08 p.m.

New subject: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

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-Qingyan... 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@i... 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/123fb3d217e89c4388fdb08b63991bac7c32... 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);

Zhou Qingyang

3:13 p.m.

New subject: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Fix this bug by adding a check of vm->ib_bo_va.

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@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/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) + 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);

-- 2.25.1

Christian König

3:15 p.m.

New subject: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

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@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/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@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);

Alex Deucher

2 Dec 2 Dec

5:13 p.m.

New subject: [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@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=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@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/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@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);

Christian König

1 Dec 1 Dec

6:57 a.m.

New subject: [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()

Am 30.11.21 um 16:57 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@umn.edu

Reviewed-by: Christian König christian.koenig@amd.com

...

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 | 35 ++++++++++++++++------------- 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 482fb0ae6cb5..439f4d1fdd65 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,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);
out_suspend: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);

1244

Age (days ago)

1246

Last active (days ago)

dri-devel@lists.freedesktop.org

12 comments

4 participants

tags (0)

participants (4)

Alex Deucher
Christian König
Qingyang Zhou
Zhou Qingyang