* [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:04 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:04 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher, Christian König
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:04 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:04 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, Alex Deucher, Christian König, Pan, Xinhui,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:04 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:04 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher, Christian König
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:04 ` Zhou Qingyang
(?)
@ 2021-11-30 15:11 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:11 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher
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 |
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:11 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:11 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher
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 |
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:11 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:11 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kjlu, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter,
amd-gfx, dri-devel, linux-kernel
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 |
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:11 ` Christian König
(?)
@ 2021-11-30 15:33 ` Zhou Qingyang
-1 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:33 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, Alex Deucher, Christian König, Pan, Xinhui,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel
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;
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:33 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:33 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher, Christian König
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;
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:33 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:33 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher, Christian König
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;
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:33 ` Zhou Qingyang
(?)
@ 2021-11-30 15:37 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:37 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kjlu, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter,
amd-gfx, dri-devel, linux-kernel
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:37 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:37 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:37 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-11-30 15:37 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:37 ` Christian König
(?)
@ 2021-11-30 15:57 ` Zhou Qingyang
-1 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:57 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, Alex Deucher, Christian König, Pan, Xinhui,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel
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 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:57 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:57 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher, Christian König
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 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-11-30 15:57 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-11-30 15:57 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher, Christian König
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 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:57 ` Zhou Qingyang
(?)
@ 2021-12-01 3:22 ` Zhou Qingyang
-1 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:22 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, kernel test robot, Alex Deucher, Christian König, Pan,
Xinhui, David Airlie, Daniel Vetter, amd-gfx, dri-devel,
linux-kernel
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
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 3:22 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:22 UTC (permalink / raw)
To: zhou1615
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Alex Deucher, Christian König
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
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 3:22 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:22 UTC (permalink / raw)
To: zhou1615
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Daniel Vetter, Alex Deucher,
Christian König
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
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-11-30 15:57 ` Zhou Qingyang
(?)
@ 2021-12-01 6:57 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 6:57 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 6:57 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 6:57 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kjlu, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter,
amd-gfx, dri-devel, linux-kernel
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 6:57 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 6:57 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 3:22 ` Zhou Qingyang
(?)
@ 2021-12-01 7:20 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 7:20 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kjlu, kernel test robot, Alex Deucher, Pan, Xinhui, David Airlie,
Daniel Vetter, amd-gfx, dri-devel, linux-kernel
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 7:20 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 7:20 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 7:20 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 7:20 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Daniel Vetter, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 7:20 ` Christian König
@ 2021-12-01 12:48 ` Qingyang Zhou
-1 siblings, 0 replies; 39+ messages in thread
From: Qingyang Zhou @ 2021-12-01 12:48 UTC (permalink / raw)
To: Christian König
Cc: kernel test robot, David Airlie, Pan, Xinhui, Kangjie Lu,
linux-kernel, amd-gfx, dri-devel, Alex Deucher
[-- Attachment #1: Type: text/plain, Size: 15294 bytes --]
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-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
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
)
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/123fb3d217e89c4388fdb08b63991bac7c324219
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);
>
>
[-- Attachment #2: Type: text/html, Size: 20421 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 12:48 ` Qingyang Zhou
0 siblings, 0 replies; 39+ messages in thread
From: Qingyang Zhou @ 2021-12-01 12:48 UTC (permalink / raw)
To: Christian König
Cc: kernel test robot, David Airlie, Pan, Xinhui, Kangjie Lu,
linux-kernel, amd-gfx, dri-devel, Daniel Vetter, Alex Deucher
[-- Attachment #1: Type: text/plain, Size: 15294 bytes --]
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-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
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
)
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/123fb3d217e89c4388fdb08b63991bac7c324219
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);
>
>
[-- Attachment #2: Type: text/html, Size: 20421 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 12:48 ` Qingyang Zhou
@ 2021-12-01 13:08 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 13:08 UTC (permalink / raw)
To: Qingyang Zhou
Cc: kernel test robot, David Airlie, Pan, Xinhui, Kangjie Lu,
linux-kernel, amd-gfx, dri-devel, Alex Deucher
[-- 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 --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 13:08 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 13:08 UTC (permalink / raw)
To: Qingyang Zhou
Cc: kernel test robot, David Airlie, Pan, Xinhui, Kangjie Lu,
linux-kernel, amd-gfx, dri-devel, Daniel Vetter, Alex Deucher
[-- 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 --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 13:08 ` Christian König
(?)
@ 2021-12-01 15:13 ` Zhou Qingyang
-1 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 15:13 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher, Christian König
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)
+ 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 15:13 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 15:13 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, Alex Deucher, Christian König, Pan, Xinhui,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel
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)
+ 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 15:13 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 15:13 UTC (permalink / raw)
To: zhou1615
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher, Christian König
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)
+ 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 15:13 ` Zhou Qingyang
(?)
@ 2021-12-01 15:15 ` Christian König
-1 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 15:15 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 15:15 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 15:15 UTC (permalink / raw)
To: Zhou Qingyang
Cc: David Airlie, Pan, Xinhui, kjlu, linux-kernel, amd-gfx,
dri-devel, Daniel Vetter, Alex Deucher
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 15:15 ` Christian König
0 siblings, 0 replies; 39+ messages in thread
From: Christian König @ 2021-12-01 15:15 UTC (permalink / raw)
To: Zhou Qingyang
Cc: kjlu, Alex Deucher, Pan, Xinhui, David Airlie, Daniel Vetter,
amd-gfx, dri-devel, linux-kernel
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);
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
2021-12-01 15:15 ` Christian König
@ 2021-12-02 17:13 ` Alex Deucher
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Deucher @ 2021-12-02 17:13 UTC (permalink / raw)
To: Christian König
Cc: Zhou Qingyang, David Airlie, Pan, Xinhui, Kangjie Lu, LKML,
amd-gfx list, Maling list - DRI developers, Alex Deucher
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);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v5] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-02 17:13 ` Alex Deucher
0 siblings, 0 replies; 39+ messages in thread
From: Alex Deucher @ 2021-12-02 17:13 UTC (permalink / raw)
To: Christian König
Cc: David Airlie, Pan, Xinhui, Kangjie Lu, LKML, amd-gfx list,
Maling list - DRI developers, Alex Deucher, Zhou Qingyang
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);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 3:23 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:23 UTC (permalink / raw)
To: zhou1615
Cc: kjlu, kernel test robot, Alex Deucher, Christian König, Pan,
Xinhui, David Airlie, Daniel Vetter, amd-gfx, dri-devel,
linux-kernel
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 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 | 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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 3:23 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:23 UTC (permalink / raw)
To: zhou1615
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Alex Deucher, Christian König
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 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 | 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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v4] drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
@ 2021-12-01 3:23 ` Zhou Qingyang
0 siblings, 0 replies; 39+ messages in thread
From: Zhou Qingyang @ 2021-12-01 3:23 UTC (permalink / raw)
To: zhou1615
Cc: kernel test robot, David Airlie, Pan, Xinhui, kjlu, linux-kernel,
amd-gfx, dri-devel, Daniel Vetter, Alex Deucher,
Christian König
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 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 | 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);
--
2.25.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
end of thread, other threads:[~2021-12-02 17:13 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.