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

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

[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;
+			}
+
 			r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
 						  RADEON_VA_IB_OFFSET,
 						  RADEON_VM_PAGE_READABLE |
-- 
2.25.1

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

[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;
+			}
+
 			r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
 						  RADEON_VA_IB_OFFSET,
 						  RADEON_VM_PAGE_READABLE |
-- 
2.25.1

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

[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;
+			}
+
 			r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
 						  RADEON_VA_IB_OFFSET,
 						  RADEON_VM_PAGE_READABLE |
-- 
2.25.1

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

[Christian König]

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 |

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

[Christian König]

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 |

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

[Christian König]

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 |

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

[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;
 
 	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

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

[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;
 
 	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

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

[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;
 
 	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

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

[Christian König]

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

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

[Christian König]

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

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

[Christian König]

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

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

[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 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

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

[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 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

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

[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 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

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

[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

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

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

[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

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

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

[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

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

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

[Christian König]

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

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

[Christian König]

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

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

[Christian König]

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

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

[Qingyang Zhou]

[-- 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 --]

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

[Qingyang Zhou]

[-- 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 --]

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

[Christian König]

[-- 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 --]

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

[Christian König]

[-- 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 --]

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

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

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

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

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

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

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

[Christian König]

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

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

[Christian König]

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

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

[Christian König]

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

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

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

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

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

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

[Christian König]

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

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

[Christian König]

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

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

[Christian König]

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

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

[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 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

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

[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 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

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

[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 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