* [PATCH 1/2] tpm: Fix tpmrm reference counting
@ 2021-06-15 9:14 Vincent Whitchurch
2021-06-15 9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
2021-06-16 18:53 ` [PATCH 1/2] tpm: Fix tpmrm reference counting Jason Gunthorpe
0 siblings, 2 replies; 7+ messages in thread
From: Vincent Whitchurch @ 2021-06-15 9:14 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Stefan Berger
Cc: kernel, Vincent Whitchurch, linux-integrity, linux-kernel
The code added by commit 8979b02aaf1d6de8 ("tpm: Fix reference count to
main device") tries to take an extra reference to the main device only
for TPM2 by looking at the flags, but the flags are actually not set
at the time when tpm_chip_alloc() is called, so no extra reference is
ever taken, leading to a use-after-free if the TPM modules are removed
when the tpmrm device is in use.
==================================================================
BUG: KASAN: use-after-free in __mutex_lock+0xe0/0xbd0
Read of size 8 at addr ffff888116c6acc0 by task sh/1210
CPU: 0 PID: 1210 Comm: sh Not tainted 5.13.0-rc5+ #200
Call Trace:
__mutex_lock+0xe0/0xbd0
tpm2_del_space+0x24/0xa0 [tpm]
tpmrm_release+0x3f/0x50 [tpm]
__fput+0x110/0x3c0
task_work_run+0x94/0xd0
do_exit+0x683/0x13e0
do_syscall_64+0x3c/0x80
Allocated by task 1153:
kasan_save_stack+0x19/0x40
__kasan_kmalloc+0x7f/0xa0
tpm_chip_alloc+0x3b/0x360 [tpm]
tpmm_chip_alloc+0x11/0x70 [tpm]
tpm_tis_core_init+0xce/0x570 [tpm_tis_core]
pnp_device_probe+0x9c/0x100
...
Freed by task 1243:
kfree+0x121/0x340
device_release+0x59/0xf0
kobject_put+0xa5/0x120
release_nodes+0x37f/0x3f0
driver_detach+0x7c/0xf0
bus_remove_driver+0x86/0x110
__x64_sys_delete_module+0x27b/0x320
...
==================================================================
The real fix to the problem which that commit tried to solve is to make
tpmm_chip_alloc() put the ->devs device in the devm release function,
since that is never done anywhere currently. This is safe since
device_initialize() is always called on ->devs. No conditional
reference taking is needed.
Fixes: 8979b02aaf1d6de8 ("tpm: Fix reference count to main device")
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
drivers/char/tpm/tpm-chip.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb7e109..029ee61c38de 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -358,10 +358,9 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
/* get extra reference on main device to hold on
* behalf of devs. This holds the chip structure
* while cdevs is in use. The corresponding put
- * is in the tpm_devs_release (TPM2 only)
+ * is in the tpm_devs_release
*/
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- get_device(&chip->dev);
+ get_device(&chip->dev);
if (chip->dev_num == 0)
chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
@@ -402,6 +401,14 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
}
EXPORT_SYMBOL_GPL(tpm_chip_alloc);
+static void tpmm_chip_release(void *data)
+{
+ struct tpm_chip *chip = data;
+
+ put_device(&chip->devs);
+ put_device(&chip->dev);
+}
+
/**
* tpmm_chip_alloc() - allocate a new struct tpm_chip instance
* @pdev: parent device to which the chip is associated
@@ -419,9 +426,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
if (IS_ERR(chip))
return chip;
- rc = devm_add_action_or_reset(pdev,
- (void (*)(void *)) put_device,
- &chip->dev);
+ rc = devm_add_action_or_reset(pdev, tpmm_chip_release, chip);
if (rc)
return ERR_PTR(rc);
--
2.28.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] tpm: Fix crash on tmprm release
2021-06-15 9:14 [PATCH 1/2] tpm: Fix tpmrm reference counting Vincent Whitchurch
@ 2021-06-15 9:14 ` Vincent Whitchurch
2021-06-15 13:18 ` Jarkko Sakkinen
2022-03-03 2:03 ` Stefan Berger
2021-06-16 18:53 ` [PATCH 1/2] tpm: Fix tpmrm reference counting Jason Gunthorpe
1 sibling, 2 replies; 7+ messages in thread
From: Vincent Whitchurch @ 2021-06-15 9:14 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Cc: kernel, Vincent Whitchurch, linux-integrity, linux-kernel
If the tpm_tis module is removed (or a system shutdown is triggered)
while the tpmrm device is use, the kernel crashes due to chip->ops being
NULL:
# exec 3<>/dev/tpmrm0
# rmmod tpm_tis
# exit
==================================================================
BUG: KASAN: null-ptr-deref in tpm_chip_start+0x2d/0x120 [tpm]
Read of size 8 at addr 0000000000000060 by task sh/994
Call Trace:
kasan_report.cold.13+0x10f/0x111
tpm_chip_start+0x2d/0x120 [tpm]
tpm2_del_space+0x2c/0xa0 [tpm]
tpmrm_release+0x3f/0x50 [tpm]
__fput+0x110/0x3c0
task_work_run+0x94/0xd0
do_exit+0x683/0x13e0
do_group_exit+0x8b/0x140
do_syscall_64+0x3c/0x80
==================================================================
Fix this by making tpm2_del_space() use tpm_try_get_ops(). The latter
already includes the calls to tpm_chip_start() and tpm_chip_stop().
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
drivers/char/tpm/tpm2-space.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 784b8b3cb903..e1111261021f 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -58,12 +58,10 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
{
- mutex_lock(&chip->tpm_mutex);
- if (!tpm_chip_start(chip)) {
+ if (!tpm_try_get_ops(chip)) {
tpm2_flush_sessions(chip, space);
- tpm_chip_stop(chip);
+ tpm_put_ops(chip);
}
- mutex_unlock(&chip->tpm_mutex);
kfree(space->context_buf);
kfree(space->session_buf);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] tpm: Fix crash on tmprm release
2021-06-15 9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
@ 2021-06-15 13:18 ` Jarkko Sakkinen
2021-06-17 5:44 ` Vincent Whitchurch
2022-03-03 2:03 ` Stefan Berger
1 sibling, 1 reply; 7+ messages in thread
From: Jarkko Sakkinen @ 2021-06-15 13:18 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: Peter Huewe, Jason Gunthorpe, kernel, linux-integrity, linux-kernel
On Tue, Jun 15, 2021 at 11:14:09AM +0200, Vincent Whitchurch wrote:
> If the tpm_tis module is removed (or a system shutdown is triggered)
> while the tpmrm device is use, the kernel crashes due to chip->ops being
> NULL:
>
> # exec 3<>/dev/tpmrm0
> # rmmod tpm_tis
> # exit
> ==================================================================
> BUG: KASAN: null-ptr-deref in tpm_chip_start+0x2d/0x120 [tpm]
> Read of size 8 at addr 0000000000000060 by task sh/994
>
> Call Trace:
> kasan_report.cold.13+0x10f/0x111
> tpm_chip_start+0x2d/0x120 [tpm]
> tpm2_del_space+0x2c/0xa0 [tpm]
> tpmrm_release+0x3f/0x50 [tpm]
> __fput+0x110/0x3c0
> task_work_run+0x94/0xd0
> do_exit+0x683/0x13e0
> do_group_exit+0x8b/0x140
> do_syscall_64+0x3c/0x80
> ==================================================================
>
> Fix this by making tpm2_del_space() use tpm_try_get_ops(). The latter
> already includes the calls to tpm_chip_start() and tpm_chip_stop().
This lacks explanation why migrating to tpm_try_get_ops() fixes the
issue. Saying that doing something fixes something is not good enough
explanation. So, can you extend this paragraph just a bit in the next
version?
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> drivers/char/tpm/tpm2-space.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 784b8b3cb903..e1111261021f 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,12 +58,10 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>
> void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
> {
> - mutex_lock(&chip->tpm_mutex);
> - if (!tpm_chip_start(chip)) {
> + if (!tpm_try_get_ops(chip)) {
> tpm2_flush_sessions(chip, space);
> - tpm_chip_stop(chip);
> + tpm_put_ops(chip);
> }
> - mutex_unlock(&chip->tpm_mutex);
> kfree(space->context_buf);
> kfree(space->session_buf);
> }
> --
> 2.28.0
>
>
/Jarkko
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/2] tpm: Fix crash on tmprm release
2021-06-15 13:18 ` Jarkko Sakkinen
@ 2021-06-17 5:44 ` Vincent Whitchurch
0 siblings, 0 replies; 7+ messages in thread
From: Vincent Whitchurch @ 2021-06-17 5:44 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Peter Huewe, Jason Gunthorpe, kernel, linux-integrity,
linux-kernel, James.Bottomley, l.sanfilippo
On Tue, Jun 15, 2021 at 03:18:48PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jun 15, 2021 at 11:14:09AM +0200, Vincent Whitchurch wrote:
> > If the tpm_tis module is removed (or a system shutdown is triggered)
> > while the tpmrm device is use, the kernel crashes due to chip->ops being
> > NULL:
> >
> > # exec 3<>/dev/tpmrm0
> > # rmmod tpm_tis
> > # exit
> > ==================================================================
> > BUG: KASAN: null-ptr-deref in tpm_chip_start+0x2d/0x120 [tpm]
> > Read of size 8 at addr 0000000000000060 by task sh/994
> >
> > Call Trace:
> > kasan_report.cold.13+0x10f/0x111
> > tpm_chip_start+0x2d/0x120 [tpm]
> > tpm2_del_space+0x2c/0xa0 [tpm]
> > tpmrm_release+0x3f/0x50 [tpm]
> > __fput+0x110/0x3c0
> > task_work_run+0x94/0xd0
> > do_exit+0x683/0x13e0
> > do_group_exit+0x8b/0x140
> > do_syscall_64+0x3c/0x80
> > ==================================================================
> >
> > Fix this by making tpm2_del_space() use tpm_try_get_ops(). The latter
> > already includes the calls to tpm_chip_start() and tpm_chip_stop().
>
> This lacks explanation why migrating to tpm_try_get_ops() fixes the
> issue. Saying that doing something fixes something is not good enough
> explanation. So, can you extend this paragraph just a bit in the next
> version?
Thank you for the review, but I see now that James already proposed a
more-or-less identical fix earlier, so I'll let that patch run its due
course:
https://lore.kernel.org/linux-integrity/e7566e1e48f5be9dca034b4bfb67683b5d3cb88f.camel@HansenPartnership.com/
https://lore.kernel.org/linux-integrity/327f4c87-e652-6cbe-c624-16a6edf0c31d@kunbus.com/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] tpm: Fix crash on tmprm release
2021-06-15 9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
2021-06-15 13:18 ` Jarkko Sakkinen
@ 2022-03-03 2:03 ` Stefan Berger
1 sibling, 0 replies; 7+ messages in thread
From: Stefan Berger @ 2022-03-03 2:03 UTC (permalink / raw)
To: Vincent Whitchurch, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe
Cc: kernel, linux-integrity, linux-kernel
On 6/15/21 05:14, Vincent Whitchurch wrote:
> If the tpm_tis module is removed (or a system shutdown is triggered)
> while the tpmrm device is use, the kernel crashes due to chip->ops being
> NULL:
>
> # exec 3<>/dev/tpmrm0
> # rmmod tpm_tis
> # exit
> ==================================================================
> BUG: KASAN: null-ptr-deref in tpm_chip_start+0x2d/0x120 [tpm]
> Read of size 8 at addr 0000000000000060 by task sh/994
>
> Call Trace:
> kasan_report.cold.13+0x10f/0x111
> tpm_chip_start+0x2d/0x120 [tpm]
> tpm2_del_space+0x2c/0xa0 [tpm]
> tpmrm_release+0x3f/0x50 [tpm]
> __fput+0x110/0x3c0
> task_work_run+0x94/0xd0
> do_exit+0x683/0x13e0
> do_group_exit+0x8b/0x140
> do_syscall_64+0x3c/0x80
> ==================================================================
>
> Fix this by making tpm2_del_space() use tpm_try_get_ops(). The latter
> already includes the calls to tpm_chip_start() and tpm_chip_stop().
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
As a follow-up to this message here: https://lkml.org/lkml/2022/3/1/552
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> drivers/char/tpm/tpm2-space.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 784b8b3cb903..e1111261021f 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,12 +58,10 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>
> void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
> {
> - mutex_lock(&chip->tpm_mutex);
> - if (!tpm_chip_start(chip)) {
> + if (!tpm_try_get_ops(chip)) {
> tpm2_flush_sessions(chip, space);
> - tpm_chip_stop(chip);
> + tpm_put_ops(chip);
> }
> - mutex_unlock(&chip->tpm_mutex);
> kfree(space->context_buf);
> kfree(space->session_buf);
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] tpm: Fix tpmrm reference counting
2021-06-15 9:14 [PATCH 1/2] tpm: Fix tpmrm reference counting Vincent Whitchurch
2021-06-15 9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
@ 2021-06-16 18:53 ` Jason Gunthorpe
2021-06-17 5:38 ` Vincent Whitchurch
1 sibling, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2021-06-16 18:53 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: Peter Huewe, Jarkko Sakkinen, Stefan Berger, kernel,
linux-integrity, linux-kernel
On Tue, Jun 15, 2021 at 11:14:08AM +0200, Vincent Whitchurch wrote:
> The code added by commit 8979b02aaf1d6de8 ("tpm: Fix reference count to
> main device") tries to take an extra reference to the main device only
> for TPM2 by looking at the flags, but the flags are actually not set
> at the time when tpm_chip_alloc() is called, so no extra reference is
> ever taken, leading to a use-after-free if the TPM modules are removed
> when the tpmrm device is in use.
Please read this
https://lore.kernel.org/linux-integrity/20210205172528.GP4718@ziepe.ca/
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] tpm: Fix tpmrm reference counting
2021-06-16 18:53 ` [PATCH 1/2] tpm: Fix tpmrm reference counting Jason Gunthorpe
@ 2021-06-17 5:38 ` Vincent Whitchurch
0 siblings, 0 replies; 7+ messages in thread
From: Vincent Whitchurch @ 2021-06-17 5:38 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Peter Huewe, Jarkko Sakkinen, Stefan Berger, kernel,
linux-integrity, linux-kernel, LinoSanfilippo
On Wed, Jun 16, 2021 at 08:53:01PM +0200, Jason Gunthorpe wrote:
> On Tue, Jun 15, 2021 at 11:14:08AM +0200, Vincent Whitchurch wrote:
> > The code added by commit 8979b02aaf1d6de8 ("tpm: Fix reference count to
> > main device") tries to take an extra reference to the main device only
> > for TPM2 by looking at the flags, but the flags are actually not set
> > at the time when tpm_chip_alloc() is called, so no extra reference is
> > ever taken, leading to a use-after-free if the TPM modules are removed
> > when the tpmrm device is in use.
>
> Please read this
>
> https://lore.kernel.org/linux-integrity/20210205172528.GP4718@ziepe.ca/
Thank you for the pointer. I see that Lino already posted your proposal
as a real patch as you requested so I will drop this.
https://lore.kernel.org/linux-integrity/1613949567-1181-2-git-send-email-LinoSanfilippo@gmx.de/
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-03 2:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 9:14 [PATCH 1/2] tpm: Fix tpmrm reference counting Vincent Whitchurch
2021-06-15 9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
2021-06-15 13:18 ` Jarkko Sakkinen
2021-06-17 5:44 ` Vincent Whitchurch
2022-03-03 2:03 ` Stefan Berger
2021-06-16 18:53 ` [PATCH 1/2] tpm: Fix tpmrm reference counting Jason Gunthorpe
2021-06-17 5:38 ` Vincent Whitchurch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.