[PATCH] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[PATCH] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[shechenglong]

When drm_sysfs_minor_alloc() fails (e.g., due to -ENOMEM), the
following Oops occurs because an ERR_PTR is passed to put_device():

  BUG: kernel NULL pointer dereference, address: 0000000000000030
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  RIP: 0010:kobject_put+0xd/0x60
  Call Trace:
   drm_minor_alloc_release+0x1c/0x50 [drm]
   drm_managed_release+0x96/0x160 [drm]
   drm_dev_init+0x269/0x330 [drm]
   drm_dev_alloc+0x3f/0x80 [drm]
   virtio_gpu_probe+0x40/0x180 [virtio_gpu]
   virtio_dev_probe+0x1fd/0x360
   ...
   do_syscall_64+0xaf/0x500
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

The call path that leads to this crash is:

  virtio_gpu_probe()
    drm_dev_alloc()
      drm_dev_init()
        drm_minor_alloc()              // allocates minor
          drm_sysfs_minor_alloc()      // returns ERR_PTR(-ENOMEM)
          minor->kdev = ERR_PTR(-ENOMEM)  // stored directly
          return -ENOMEM
        // drm_dev_init() fails, triggers cleanup:
        drm_managed_release()
          drm_minor_alloc_release(dev, minor)
            put_device(minor->kdev)     // ERR_PTR(-ENOMEM) dereferenced
              kobject_put()             // crashes at address 0x30

drm_minor_alloc() assigns the return value of drm_sysfs_minor_alloc()
directly to minor->kdev, even when it is an error pointer.  The
subsequent cleanup unconditionally calls put_device(minor->kdev),
which dereferences the error pointer and causes a NULL-pointer
dereference (offset 0x30 into a non-page-mapped area).

Fix this by using a temporary variable to hold the result of
drm_sysfs_minor_alloc().  If the allocation fails, we return the error
immediately, leaving minor->kdev as NULL (the whole minor structure is
zero-allocated).  put_device(NULL) is explicitly allowed and safe.

Fixes: f96306f9892b ("drm: manage drm_minor cleanup with drmm_")

Signed-off-by: shechenglong <shechenglong@xfusion.com>
---
 drivers/gpu/drm/drm_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 675675480..2348fbb21 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -143,6 +143,7 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
 static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 {
 	struct drm_minor *minor;
+	struct device *kdev;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -164,9 +165,11 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 	if (r)
 		return r;
 
-	minor->kdev = drm_sysfs_minor_alloc(minor);
-	if (IS_ERR(minor->kdev))
-		return PTR_ERR(minor->kdev);
+	kdev = drm_sysfs_minor_alloc(minor);
+	if (IS_ERR(kdev))
+		return PTR_ERR(kdev);
+
+	minor->kdev = kdev;
 
 	*drm_minor_get_slot(dev, type) = minor;
 	return 0;
-- 
2.43.0

[RESEND PATCH] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[shechenglong]

When drm_sysfs_minor_alloc() fails (e.g., due to -ENOMEM), the
following Oops occurs because an ERR_PTR is passed to put_device():

  BUG: kernel NULL pointer dereference, address: 0000000000000030
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  RIP: 0010:kobject_put+0xd/0x60
  Call Trace:
   drm_minor_alloc_release+0x1c/0x50 [drm]
   drm_managed_release+0x96/0x160 [drm]
   drm_dev_init+0x269/0x330 [drm]
   drm_dev_alloc+0x3f/0x80 [drm]
   virtio_gpu_probe+0x40/0x180 [virtio_gpu]
   virtio_dev_probe+0x1fd/0x360
   ...
   do_syscall_64+0xaf/0x500
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

The call path that leads to this crash is:

  virtio_gpu_probe()
    drm_dev_alloc()
      drm_dev_init()
        drm_minor_alloc()              // allocates minor
          drm_sysfs_minor_alloc()      // returns ERR_PTR(-ENOMEM)
          minor->kdev = ERR_PTR(-ENOMEM)  // stored directly
          return -ENOMEM
        // drm_dev_init() fails, triggers cleanup:
        drm_managed_release()
          drm_minor_alloc_release(dev, minor)
            put_device(minor->kdev)     // ERR_PTR(-ENOMEM) dereferenced
              kobject_put()             // crashes at address 0x30

drm_minor_alloc() assigns the return value of drm_sysfs_minor_alloc()
directly to minor->kdev, even when it is an error pointer.  The
subsequent cleanup unconditionally calls put_device(minor->kdev),
which dereferences the error pointer and causes a NULL-pointer
dereference (offset 0x30 into a non-page-mapped area).

Fix this by using a temporary variable to hold the result of
drm_sysfs_minor_alloc().  If the allocation fails, we return the error
immediately, leaving minor->kdev as NULL (the whole minor structure is
zero-allocated).  put_device(NULL) is explicitly allowed and safe.

Fixes: f96306f9892b ("drm: manage drm_minor cleanup with drmm_")

Signed-off-by: shechenglong <shechenglong@xfusion.com>
---
 drivers/gpu/drm/drm_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 675675480..2348fbb21 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -143,6 +143,7 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
 static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 {
 	struct drm_minor *minor;
+	struct device *kdev;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -164,9 +165,11 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 	if (r)
 		return r;
 
-	minor->kdev = drm_sysfs_minor_alloc(minor);
-	if (IS_ERR(minor->kdev))
-		return PTR_ERR(minor->kdev);
+	kdev = drm_sysfs_minor_alloc(minor);
+	if (IS_ERR(kdev))
+		return PTR_ERR(kdev);
+
+	minor->kdev = kdev;
 
 	*drm_minor_get_slot(dev, type) = minor;
 	return 0;
-- 
2.43.0

Re: [RESEND PATCH] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[Thomas Zimmermann]

Hi

Am 25.08.26 um 10:44 schrieb shechenglong:
> When drm_sysfs_minor_alloc() fails (e.g., due to -ENOMEM), the
> following Oops occurs because an ERR_PTR is passed to put_device():
>
>    BUG: kernel NULL pointer dereference, address: 0000000000000030
>    #PF: supervisor read access in kernel mode
>    #PF: error_code(0x0000) - not-present page
>    RIP: 0010:kobject_put+0xd/0x60
>    Call Trace:
>     drm_minor_alloc_release+0x1c/0x50 [drm]
>     drm_managed_release+0x96/0x160 [drm]
>     drm_dev_init+0x269/0x330 [drm]
>     drm_dev_alloc+0x3f/0x80 [drm]
>     virtio_gpu_probe+0x40/0x180 [virtio_gpu]
>     virtio_dev_probe+0x1fd/0x360
>     ...
>     do_syscall_64+0xaf/0x500
>     entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> The call path that leads to this crash is:
>
>    virtio_gpu_probe()
>      drm_dev_alloc()
>        drm_dev_init()
>          drm_minor_alloc()              // allocates minor
>            drm_sysfs_minor_alloc()      // returns ERR_PTR(-ENOMEM)
>            minor->kdev = ERR_PTR(-ENOMEM)  // stored directly
>            return -ENOMEM
>          // drm_dev_init() fails, triggers cleanup:
>          drm_managed_release()
>            drm_minor_alloc_release(dev, minor)
>              put_device(minor->kdev)     // ERR_PTR(-ENOMEM) dereferenced
>                kobject_put()             // crashes at address 0x30
>
> drm_minor_alloc() assigns the return value of drm_sysfs_minor_alloc()
> directly to minor->kdev, even when it is an error pointer.  The
> subsequent cleanup unconditionally calls put_device(minor->kdev),
> which dereferences the error pointer and causes a NULL-pointer
> dereference (offset 0x30 into a non-page-mapped area).
>
> Fix this by using a temporary variable to hold the result of
> drm_sysfs_minor_alloc().  If the allocation fails, we return the error
> immediately, leaving minor->kdev as NULL (the whole minor structure is
> zero-allocated).  put_device(NULL) is explicitly allowed and safe.
>
> Fixes: f96306f9892b ("drm: manage drm_minor cleanup with drmm_")
>
> Signed-off-by: shechenglong <shechenglong@xfusion.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for the fix.

Best regards
Thomas

> ---
>   drivers/gpu/drm/drm_drv.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 675675480..2348fbb21 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -143,6 +143,7 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
>   static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
>   {
>   	struct drm_minor *minor;
> +	struct device *kdev;
>   	int r;
>   
>   	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> @@ -164,9 +165,11 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
>   	if (r)
>   		return r;
>   
> -	minor->kdev = drm_sysfs_minor_alloc(minor);
> -	if (IS_ERR(minor->kdev))
> -		return PTR_ERR(minor->kdev);
> +	kdev = drm_sysfs_minor_alloc(minor);
> +	if (IS_ERR(kdev))
> +		return PTR_ERR(kdev);
> +
> +	minor->kdev = kdev;
>   
>   	*drm_minor_get_slot(dev, type) = minor;
>   	return 0;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)

Re: [RESEND PATCH] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[shechenglong]

Hi Thomas,

Thanks again for the review.

I noticed that this patch has not yet appeared in the drm-misc branches.
Is there anything else needed from my side for it to be merged?

Best regards,
chenglong She

[PATCH v2] drm: Fix NULL pointer dereference in drm_minor_alloc() on error path

[shechenglong]

When drm_sysfs_minor_alloc() fails (e.g., due to -ENOMEM), the
following Oops occurs because an ERR_PTR is passed to put_device():

  BUG: kernel NULL pointer dereference, address: 0000000000000030
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  RIP: 0010:kobject_put+0xd/0x60
  Call Trace:
   drm_minor_alloc_release+0x1c/0x50 [drm]
   drm_managed_release+0x96/0x160 [drm]
   drm_dev_init+0x269/0x330 [drm]
   drm_dev_alloc+0x3f/0x80 [drm]
   virtio_gpu_probe+0x40/0x180 [virtio_gpu]
   virtio_dev_probe+0x1fd/0x360
   ...
   do_syscall_64+0xaf/0x500
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

The call path that leads to this crash is:

  virtio_gpu_probe()
    drm_dev_alloc()
      drm_dev_init()
        drm_minor_alloc()              // allocates minor
          drm_sysfs_minor_alloc()      // returns ERR_PTR(-ENOMEM)
          minor->kdev = ERR_PTR(-ENOMEM)  // stored directly
          return -ENOMEM
        // drm_dev_init() fails, triggers cleanup:
        drm_managed_release()
          drm_minor_alloc_release(dev, minor)
            put_device(minor->kdev)     // ERR_PTR(-ENOMEM) dereferenced
              kobject_put()             // crashes at address 0x30

drm_minor_alloc() assigns the return value of drm_sysfs_minor_alloc()
directly to minor->kdev, even when it is an error pointer.  The
subsequent cleanup unconditionally calls put_device(minor->kdev),
which dereferences the error pointer and causes a NULL-pointer
dereference (offset 0x30 into a non-page-mapped area).

Fix this by using a temporary variable to hold the result of
drm_sysfs_minor_alloc().  If the allocation fails, we return the error
immediately, leaving minor->kdev as NULL (the whole minor structure is
zero-allocated).  put_device(NULL) is explicitly allowed and safe.

Fixes: f96306f9892b ("drm: manage drm_minor cleanup with drmm_")

Signed-off-by: chenjialong <chenjialong@xfusion.com>
Signed-off-by: shechenglong <shechenglong@xfusion.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index c808958a2..9c51312cd 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -143,6 +143,7 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
 static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 {
 	struct drm_minor *minor;
+	struct device *kdev;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -164,9 +165,11 @@ static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
 	if (r)
 		return r;
 
-	minor->kdev = drm_sysfs_minor_alloc(minor);
-	if (IS_ERR(minor->kdev))
-		return PTR_ERR(minor->kdev);
+	kdev = drm_sysfs_minor_alloc(minor);
+	if (IS_ERR(kdev))
+		return PTR_ERR(kdev);
+
+	minor->kdev = kdev;
 
 	*drm_minor_get_slot(dev, type) = minor;
 	return 0;
-- 
2.43.0