[PATCH 0/2] drm/vkms: Fix some memory related bug

[PATCH 0/2] drm/vkms: Fix some memory related bug

[Yuan Can]

This series contains two memory related bugfixes about vkms driver.

Yuan Can (2):
  drm/vkms: Fix memory leak in vkms_init()
  drm/vkms: Fix null-ptr-deref in vkms_release()

 drivers/gpu/drm/vkms/vkms_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] drm/vkms: Fix memory leak in vkms_init()

[Yuan Can]

A memory leak was reported after the vkms module install failed.

unreferenced object 0xffff88810bc28520 (size 16):
  comm "modprobe", pid 9662, jiffies 4298009455 (age 42.590s)
  hex dump (first 16 bytes):
    01 01 00 64 81 88 ff ff 00 00 dc 0a 81 88 ff ff  ...d............
  backtrace:
    [<00000000e7561ff8>] kmalloc_trace+0x27/0x60
    [<000000000b1954a0>] 0xffffffffc45200a9
    [<00000000abbf1da0>] do_one_initcall+0xd0/0x4f0
    [<000000001505ee87>] do_init_module+0x1a4/0x680
    [<00000000958079ad>] load_module+0x6249/0x7110
    [<00000000117e4696>] __do_sys_finit_module+0x140/0x200
    [<00000000f74b12d2>] do_syscall_64+0x35/0x80
    [<000000008fc6fcde>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

The reason is that the vkms_init() returns without checking the return
value of vkms_create(), and if the vkms_create() failed, the config
allocated at the beginning of vkms_init() is leaked.

 vkms_init()
   config = kmalloc(...) # config allocated
   ...
   return vkms_create() # vkms_create failed and config is leaked

Fix this problem by checking return value of vkms_create() and free the
config if error happened.

Fixes: 2df7af93fdad ("drm/vkms: Add vkms_config type")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 0ffe5f0e33f7..dfe983eaa07f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -218,6 +218,7 @@ static int vkms_create(struct vkms_config *config)
 
 static int __init vkms_init(void)
 {
+	int ret;
 	struct vkms_config *config;
 
 	config = kmalloc(sizeof(*config), GFP_KERNEL);
@@ -230,7 +231,11 @@ static int __init vkms_init(void)
 	config->writeback = enable_writeback;
 	config->overlay = enable_overlay;
 
-	return vkms_create(config);
+	ret = vkms_create(config);
+	if (ret)
+		kfree(config);
+
+	return ret;
 }
 
 static void vkms_destroy(struct vkms_config *config)
-- 
2.17.1

[PATCH 2/2] drm/vkms: Fix null-ptr-deref in vkms_release()

[Yuan Can]

A null-ptr-deref is triggered when it tries to destroy the workqueue in
vkms->output.composer_workq in vkms_release().

 KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
 CPU: 5 PID: 17193 Comm: modprobe Not tainted 6.0.0-11331-gd465bff130bf #24
 RIP: 0010:destroy_workqueue+0x2f/0x710
 ...
 Call Trace:
  <TASK>
  ? vkms_config_debugfs_init+0x50/0x50 [vkms]
  __devm_drm_dev_alloc+0x15a/0x1c0 [drm]
  vkms_init+0x245/0x1000 [vkms]
  do_one_initcall+0xd0/0x4f0
  do_init_module+0x1a4/0x680
  load_module+0x6249/0x7110
  __do_sys_finit_module+0x140/0x200
  do_syscall_64+0x35/0x80
  entry_SYSCALL_64_after_hwframe+0x46/0xb0

The reason is that an OOM happened which triggers the destroy of the
workqueue, however, the workqueue is alloced in the later process,
thus a null-ptr-deref happened. A simple call graph is shown as below:

 vkms_init()
  vkms_create()
    devm_drm_dev_alloc()
      __devm_drm_dev_alloc()
        devm_drm_dev_init()
          devm_add_action_or_reset()
            devm_add_action() # an error happened
            devm_drm_dev_init_release()
              drm_dev_put()
                kref_put()
                  drm_dev_release()
                    vkms_release()
                      destroy_workqueue() # null-ptr-deref happened
    vkms_modeset_init()
      vkms_output_init()
        vkms_crtc_init() # where the workqueue get allocated

Fix this by checking if composer_workq is NULL before passing it to
the destroy_workqueue() in vkms_release().

Fixes: 6c234fe37c57 ("drm/vkms: Implement CRC debugfs API")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index dfe983eaa07f..f716c5796f5f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
 {
 	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
 
-	destroy_workqueue(vkms->output.composer_workq);
+	if (vkms->output.composer_workq)
+		destroy_workqueue(vkms->output.composer_workq);
 }
 
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
-- 
2.17.1

Re: [PATCH 1/2] drm/vkms: Fix memory leak in vkms_init()

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On 11/01, Yuan Can wrote:
> A memory leak was reported after the vkms module install failed.
> 
> unreferenced object 0xffff88810bc28520 (size 16):
>   comm "modprobe", pid 9662, jiffies 4298009455 (age 42.590s)
>   hex dump (first 16 bytes):
>     01 01 00 64 81 88 ff ff 00 00 dc 0a 81 88 ff ff  ...d............
>   backtrace:
>     [<00000000e7561ff8>] kmalloc_trace+0x27/0x60
>     [<000000000b1954a0>] 0xffffffffc45200a9
>     [<00000000abbf1da0>] do_one_initcall+0xd0/0x4f0
>     [<000000001505ee87>] do_init_module+0x1a4/0x680
>     [<00000000958079ad>] load_module+0x6249/0x7110
>     [<00000000117e4696>] __do_sys_finit_module+0x140/0x200
>     [<00000000f74b12d2>] do_syscall_64+0x35/0x80
>     [<000000008fc6fcde>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> The reason is that the vkms_init() returns without checking the return
> value of vkms_create(), and if the vkms_create() failed, the config
> allocated at the beginning of vkms_init() is leaked.
> 
>  vkms_init()
>    config = kmalloc(...) # config allocated
>    ...
>    return vkms_create() # vkms_create failed and config is leaked
> 
> Fix this problem by checking return value of vkms_create() and free the
> config if error happened.
> 
> Fixes: 2df7af93fdad ("drm/vkms: Add vkms_config type")
> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 0ffe5f0e33f7..dfe983eaa07f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -218,6 +218,7 @@ static int vkms_create(struct vkms_config *config)
>  
>  static int __init vkms_init(void)
>  {
> +	int ret;
>  	struct vkms_config *config;
>  
>  	config = kmalloc(sizeof(*config), GFP_KERNEL);
> @@ -230,7 +231,11 @@ static int __init vkms_init(void)
>  	config->writeback = enable_writeback;
>  	config->overlay = enable_overlay;
>  
> -	return vkms_create(config);
> +	ret = vkms_create(config);
> +	if (ret)
> +		kfree(config);
> +
> +	return ret;

Good catch. Thanks,

Reviewed-by: Melissa Wen <mwen@igalia.com>

>  }
>  
>  static void vkms_destroy(struct vkms_config *config)
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm/vkms: Fix null-ptr-deref in vkms_release()

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On 11/01, Yuan Can wrote:
> A null-ptr-deref is triggered when it tries to destroy the workqueue in
> vkms->output.composer_workq in vkms_release().
> 
>  KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>  CPU: 5 PID: 17193 Comm: modprobe Not tainted 6.0.0-11331-gd465bff130bf #24
>  RIP: 0010:destroy_workqueue+0x2f/0x710
>  ...
>  Call Trace:
>   <TASK>
>   ? vkms_config_debugfs_init+0x50/0x50 [vkms]
>   __devm_drm_dev_alloc+0x15a/0x1c0 [drm]
>   vkms_init+0x245/0x1000 [vkms]
>   do_one_initcall+0xd0/0x4f0
>   do_init_module+0x1a4/0x680
>   load_module+0x6249/0x7110
>   __do_sys_finit_module+0x140/0x200
>   do_syscall_64+0x35/0x80
>   entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> The reason is that an OOM happened which triggers the destroy of the
> workqueue, however, the workqueue is alloced in the later process,
> thus a null-ptr-deref happened. A simple call graph is shown as below:
> 
>  vkms_init()
>   vkms_create()
>     devm_drm_dev_alloc()
>       __devm_drm_dev_alloc()
>         devm_drm_dev_init()
>           devm_add_action_or_reset()
>             devm_add_action() # an error happened
>             devm_drm_dev_init_release()
>               drm_dev_put()
>                 kref_put()
>                   drm_dev_release()
>                     vkms_release()
>                       destroy_workqueue() # null-ptr-deref happened
>     vkms_modeset_init()
>       vkms_output_init()
>         vkms_crtc_init() # where the workqueue get allocated
> 
> Fix this by checking if composer_workq is NULL before passing it to
> the destroy_workqueue() in vkms_release().
> 
> Fixes: 6c234fe37c57 ("drm/vkms: Implement CRC debugfs API")
> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index dfe983eaa07f..f716c5796f5f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
>  {
>  	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
>  
> -	destroy_workqueue(vkms->output.composer_workq);
> +	if (vkms->output.composer_workq)
> +		destroy_workqueue(vkms->output.composer_workq);

Reviewed-by: Melissa Wen <mwen@igalia.com>
>  }
>  
>  static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]