2023-05-15 22:51:29

by Jarkko Sakkinen

Subject: [PATCH v2] tpm: tpm_vtpm_proxy: fix a race condition in /dev/vtpmx creation

From: Jarkko Sakkinen <[email protected]>

/dev/vtpmx is made visible before 'workqueue' is initialized, which can
lead to a memory corruption in the worst case scenario.

Address this by initializing 'workqueue' as the very first step of the
driver initialization.

Cc: [email protected]
Fixes: 6f99612e2500 ("tpm: Proxy driver for supporting multiple emulated TPMs")
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v2:
- Replace vtpmx_cleanup() with destroy_workqueue():
https://lore.kernel.org/linux-integrity/CSLCEYDKKWWE.36POIXVT65SLE@suppilovahvero/
- Fix typo:
https://lore.kernel.org/linux-integrity/[email protected]/
---
drivers/char/tpm/tpm_vtpm_proxy.c | 30 +++++++-----------------------
1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 5c865987ba5c..30e953988cab 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -683,37 +683,21 @@ static struct miscdevice vtpmx_miscdev = {
.fops = &vtpmx_fops,
};

-static int vtpmx_init(void)
-{
- return misc_register(&vtpmx_miscdev);
-}
-
-static void vtpmx_cleanup(void)
-{
- misc_deregister(&vtpmx_miscdev);
-}
-
static int __init vtpm_module_init(void)
{
int rc;

- rc = vtpmx_init();
- if (rc) {
- pr_err("couldn't create vtpmx device\n");
- return rc;
- }
-
workqueue = create_workqueue("tpm-vtpm");
if (!workqueue) {
pr_err("couldn't create workqueue\n");
- rc = -ENOMEM;
- goto err_vtpmx_cleanup;
+ return -ENOMEM;
}

- return 0;
-
-err_vtpmx_cleanup:
- vtpmx_cleanup();
+ rc = misc_register(&vtpmx_miscdev);
+ if (rc) {
+ pr_err("couldn't create vtpmx device\n");
+ destroy_workqueue(workqueue);
+ }

return rc;
}
@@ -721,7 +705,7 @@ static int __init vtpm_module_init(void)
static void __exit vtpm_module_exit(void)
{
destroy_workqueue(workqueue);
- vtpmx_cleanup();
+ misc_deregister(&vtpmx_miscdev);
}

module_init(vtpm_module_init);
--
2.39.2

2023-05-19 22:28:33

by Stefan Berger

Subject: Re: [PATCH v2] tpm: tpm_vtpm_proxy: fix a race condition in /dev/vtpmx creation

On 5/15/23 18:25, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <[email protected]>
>
> /dev/vtpmx is made visible before 'workqueue' is initialized, which can
> lead to a memory corruption in the worst case scenario.
>
> Address this by initializing 'workqueue' as the very first step of the
> driver initialization.
>
> Cc: [email protected]
> Fixes: 6f99612e2500 ("tpm: Proxy driver for supporting multiple emulated TPMs")
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> v2:
> - Replace vtpmx_cleanup() with destroy_workqueue():
> https://lore.kernel.org/linux-integrity/CSLCEYDKKWWE.36POIXVT65SLE@suppilovahvero/
> - Fix typo:
> https://lore.kernel.org/linux-integrity/[email protected]/
> ---
> drivers/char/tpm/tpm_vtpm_proxy.c | 30 +++++++-----------------------
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 5c865987ba5c..30e953988cab 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -683,37 +683,21 @@ static struct miscdevice vtpmx_miscdev = {
> .fops = &vtpmx_fops,
> };
>
> -static int vtpmx_init(void)
> -{
> - return misc_register(&vtpmx_miscdev);
> -}
> -
> -static void vtpmx_cleanup(void)
> -{
> - misc_deregister(&vtpmx_miscdev);
> -}
> -
> static int __init vtpm_module_init(void)
> {
> int rc;
>
> - rc = vtpmx_init();
> - if (rc) {
> - pr_err("couldn't create vtpmx device\n");
> - return rc;
> - }
> -
> workqueue = create_workqueue("tpm-vtpm");
> if (!workqueue) {
> pr_err("couldn't create workqueue\n");
> - rc = -ENOMEM;
> - goto err_vtpmx_cleanup;
> + return -ENOMEM;
> }
>
> - return 0;
> -
> -err_vtpmx_cleanup:
> - vtpmx_cleanup();
> + rc = misc_register(&vtpmx_miscdev);
> + if (rc) {
> + pr_err("couldn't create vtpmx device\n");
> + destroy_workqueue(workqueue);
> + }
>
> return rc;
> }
> @@ -721,7 +705,7 @@ static int __init vtpm_module_init(void)
> static void __exit vtpm_module_exit(void)
> {
> destroy_workqueue(workqueue);
> - vtpmx_cleanup();
> + misc_deregister(&vtpmx_miscdev);
> }
>
> module_init(vtpm_module_init);

Reviewed-by: Stefan Berger <[email protected]>