Changes in v3 - drop the patch that introduces the new function tpm_chip_free() - rework the commit messages for the patches (style, typos, etc.) - add fixes tag to patch 2 - add James Bottomley to cc list - add stable mailing list to cc list
Changes in v2: - drop the patch that erroneously cleaned up after failed installation of an action handler in tpmm_chip_alloc() (pointed out by Jarkko Sakkinen) - make the commit message for patch 1 more detailed - add fixes tags and kernel logs
Lino Sanfilippo (2): tpm: fix reference counting for struct tpm_chip tpm: in tpm2_del_space check if ops pointer is still valid
drivers/char/tpm/tpm-chip.c | 18 +++++++++++++++--- drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- drivers/char/tpm/tpm_ftpm_tee.c | 2 ++ drivers/char/tpm/tpm_vtpm_proxy.c | 1 + 4 files changed, 28 insertions(+), 8 deletions(-)
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
1. Open device /dev/tpmrm 2. Remove module tpm_tis_spi 3. Write a TPM command to the file descriptor opened at step 1.
------------[ cut here ]------------ WARNING: CPU: 3 PID: 1161 at lib/refcount.c:25 kobject_get+0xa0/0xa4 refcount_t: addition on 0; use-after-free. Modules linked in: tpm_tis_spi tpm_tis_core tpm mdio_bcm_unimac brcmfmac sha256_generic libsha256 sha256_arm hci_uart btbcm bluetooth cfg80211 vc4 brcmutil ecdh_generic ecc snd_soc_core crc32_arm_ce libaes raspberrypi_hwmon ac97_bus snd_pcm_dmaengine bcm2711_thermal snd_pcm snd_timer genet snd phy_generic soundcore [last unloaded: spi_bcm2835] CPU: 3 PID: 1161 Comm: hold_open Not tainted 5.10.0ls-main-dirty #2 Hardware name: BCM2711 [<c0410c3c>] (unwind_backtrace) from [<c040b580>] (show_stack+0x10/0x14) [<c040b580>] (show_stack) from [<c1092174>] (dump_stack+0xc4/0xd8) [<c1092174>] (dump_stack) from [<c0445a30>] (__warn+0x104/0x108) [<c0445a30>] (__warn) from [<c0445aa8>] (warn_slowpath_fmt+0x74/0xb8) [<c0445aa8>] (warn_slowpath_fmt) from [<c08435d0>] (kobject_get+0xa0/0xa4) [<c08435d0>] (kobject_get) from [<bf0a715c>] (tpm_try_get_ops+0x14/0x54 [tpm]) [<bf0a715c>] (tpm_try_get_ops [tpm]) from [<bf0a7d6c>] (tpm_common_write+0x38/0x60 [tpm]) [<bf0a7d6c>] (tpm_common_write [tpm]) from [<c05a7ac0>] (vfs_write+0xc4/0x3c0) [<c05a7ac0>] (vfs_write) from [<c05a7ee4>] (ksys_write+0x58/0xcc) [<c05a7ee4>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xc226bfa8 to 0xc226bff0) bfa0: 00000000 000105b4 00000003 beafe664 00000014 00000000 bfc0: 00000000 000105b4 000103f8 00000004 00000000 00000000 b6f9c000 beafe684 bfe0: 0000006c beafe648 0001056c b6eb6944 ---[ end trace d4b8409def9b8b1f ]---
The reason for this warning is the attempt to get the chip->dev reference in tpm_common_write() although the reference counter is already zero.
Since commit 8979b02aaf1d ("tpm: Fix reference count to main device") the extra reference used to prevent a premature zero counter is never taken, because the required TPM_CHIP_FLAG_TPM2 flag is never set.
Fix this by removing the flag condition.
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Fix this also by installing an action handler that puts chip->devs as soon as the chip is unregistered.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com --- drivers/char/tpm/tpm-chip.c | 18 +++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 2 ++ drivers/char/tpm/tpm_vtpm_proxy.c | 1 + 3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb..3ace199 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -360,8 +360,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, * while cdevs is in use. The corresponding put * is in the tpm_devs_release (TPM2 only) */ - 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); @@ -422,8 +421,21 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev, rc = devm_add_action_or_reset(pdev, (void (*)(void *)) put_device, &chip->dev); - if (rc) + if (rc) { + put_device(&chip->devs); return ERR_PTR(rc); + } + + rc = devm_add_action_or_reset(pdev, + (void (*)(void *)) put_device, + &chip->devs); + if (rc) { + devm_remove_action(pdev, + (void (*)(void *)) put_device, + &chip->dev); + put_device(&chip->dev); + return ERR_PTR(rc); + }
dev_set_drvdata(pdev, chip);
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c index 2ccdf8a..82858c2 100644 --- a/drivers/char/tpm/tpm_ftpm_tee.c +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -286,6 +286,7 @@ static int ftpm_tee_probe(struct device *dev)
out_chip: put_device(&pvt_data->chip->dev); + put_device(&pvt_data->chip->devs); out_chip_alloc: tee_shm_free(pvt_data->shm); out_shm_alloc: @@ -318,6 +319,7 @@ static int ftpm_tee_remove(struct device *dev) tpm_chip_unregister(pvt_data->chip);
/* frees chip */ + put_device(&pvt_data->chip->devs); put_device(&pvt_data->chip->dev);
/* Free the shared memory pool */ diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c index 91c772e3..97b60f8 100644 --- a/drivers/char/tpm/tpm_vtpm_proxy.c +++ b/drivers/char/tpm/tpm_vtpm_proxy.c @@ -520,6 +520,7 @@ static struct proxy_dev *vtpm_proxy_create_proxy_dev(void) */ static inline void vtpm_proxy_delete_proxy_dev(struct proxy_dev *proxy_dev) { + put_device(&proxy_dev->chip->devs); put_device(&proxy_dev->chip->dev); /* frees chip */ kfree(proxy_dev); }
On 2/4/21 6:50 PM, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- Write a TPM command to the file descriptor opened at step 1.
------------[ cut here ]------------ WARNING: CPU: 3 PID: 1161 at lib/refcount.c:25 kobject_get+0xa0/0xa4 refcount_t: addition on 0; use-after-free. Modules linked in: tpm_tis_spi tpm_tis_core tpm mdio_bcm_unimac brcmfmac sha256_generic libsha256 sha256_arm hci_uart btbcm bluetooth cfg80211 vc4 brcmutil ecdh_generic ecc snd_soc_core crc32_arm_ce libaes raspberrypi_hwmon ac97_bus snd_pcm_dmaengine bcm2711_thermal snd_pcm snd_timer genet snd phy_generic soundcore [last unloaded: spi_bcm2835] CPU: 3 PID: 1161 Comm: hold_open Not tainted 5.10.0ls-main-dirty #2 Hardware name: BCM2711 [<c0410c3c>] (unwind_backtrace) from [<c040b580>] (show_stack+0x10/0x14) [<c040b580>] (show_stack) from [<c1092174>] (dump_stack+0xc4/0xd8) [<c1092174>] (dump_stack) from [<c0445a30>] (__warn+0x104/0x108) [<c0445a30>] (__warn) from [<c0445aa8>] (warn_slowpath_fmt+0x74/0xb8) [<c0445aa8>] (warn_slowpath_fmt) from [<c08435d0>] (kobject_get+0xa0/0xa4) [<c08435d0>] (kobject_get) from [<bf0a715c>] (tpm_try_get_ops+0x14/0x54 [tpm]) [<bf0a715c>] (tpm_try_get_ops [tpm]) from [<bf0a7d6c>] (tpm_common_write+0x38/0x60 [tpm]) [<bf0a7d6c>] (tpm_common_write [tpm]) from [<c05a7ac0>] (vfs_write+0xc4/0x3c0) [<c05a7ac0>] (vfs_write) from [<c05a7ee4>] (ksys_write+0x58/0xcc) [<c05a7ee4>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xc226bfa8 to 0xc226bff0) bfa0: 00000000 000105b4 00000003 beafe664 00000014 00000000 bfc0: 00000000 000105b4 000103f8 00000004 00000000 00000000 b6f9c000 beafe684 bfe0: 0000006c beafe648 0001056c b6eb6944 ---[ end trace d4b8409def9b8b1f ]---
The reason for this warning is the attempt to get the chip->dev reference in tpm_common_write() although the reference counter is already zero.
Since commit 8979b02aaf1d ("tpm: Fix reference count to main device") the extra reference used to prevent a premature zero counter is never taken, because the required TPM_CHIP_FLAG_TPM2 flag is never set.
Fix this by removing the flag condition.
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Fix this also by installing an action handler that puts chip->devs as soon as the chip is unregistered.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
Tested-by: Stefan Berger stefanb@linux.ibm.com
Steps:
modprobe tpm_vtpm_proxy
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm1
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
exec 100>&- # fails before, works after --> great job! :-)
On 2/4/21 7:46 PM, Stefan Berger wrote:
On 2/4/21 6:50 PM, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- Write a TPM command to the file descriptor opened at step 1.
------------[ cut here ]------------ WARNING: CPU: 3 PID: 1161 at lib/refcount.c:25 kobject_get+0xa0/0xa4 refcount_t: addition on 0; use-after-free. Modules linked in: tpm_tis_spi tpm_tis_core tpm mdio_bcm_unimac brcmfmac sha256_generic libsha256 sha256_arm hci_uart btbcm bluetooth cfg80211 vc4 brcmutil ecdh_generic ecc snd_soc_core crc32_arm_ce libaes raspberrypi_hwmon ac97_bus snd_pcm_dmaengine bcm2711_thermal snd_pcm snd_timer genet snd phy_generic soundcore [last unloaded: spi_bcm2835] CPU: 3 PID: 1161 Comm: hold_open Not tainted 5.10.0ls-main-dirty #2 Hardware name: BCM2711 [<c0410c3c>] (unwind_backtrace) from [<c040b580>] (show_stack+0x10/0x14) [<c040b580>] (show_stack) from [<c1092174>] (dump_stack+0xc4/0xd8) [<c1092174>] (dump_stack) from [<c0445a30>] (__warn+0x104/0x108) [<c0445a30>] (__warn) from [<c0445aa8>] (warn_slowpath_fmt+0x74/0xb8) [<c0445aa8>] (warn_slowpath_fmt) from [<c08435d0>] (kobject_get+0xa0/0xa4) [<c08435d0>] (kobject_get) from [<bf0a715c>] (tpm_try_get_ops+0x14/0x54 [tpm]) [<bf0a715c>] (tpm_try_get_ops [tpm]) from [<bf0a7d6c>] (tpm_common_write+0x38/0x60 [tpm]) [<bf0a7d6c>] (tpm_common_write [tpm]) from [<c05a7ac0>] (vfs_write+0xc4/0x3c0) [<c05a7ac0>] (vfs_write) from [<c05a7ee4>] (ksys_write+0x58/0xcc) [<c05a7ee4>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xc226bfa8 to 0xc226bff0) bfa0: 00000000 000105b4 00000003 beafe664 00000014 00000000 bfc0: 00000000 000105b4 000103f8 00000004 00000000 00000000 b6f9c000 beafe684 bfe0: 0000006c beafe648 0001056c b6eb6944 ---[ end trace d4b8409def9b8b1f ]---
The reason for this warning is the attempt to get the chip->dev reference in tpm_common_write() although the reference counter is already zero.
Since commit 8979b02aaf1d ("tpm: Fix reference count to main device") the extra reference used to prevent a premature zero counter is never taken, because the required TPM_CHIP_FLAG_TPM2 flag is never set.
Fix this by removing the flag condition.
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Fix this also by installing an action handler that puts chip->devs as soon as the chip is unregistered.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
Tested-by: Stefan Berger stefanb@linux.ibm.com
Steps:
modprobe tpm_vtpm_proxy
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm1
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
exec 100>&- # fails before, works after --> great job! :-)
To clarify: When I tested this I had *both* patches applied. Without the patches I got the null pointer exception in tpm2_del_space(). The 2nd patch alone solves that issue when using the steps above.
[ 525.647443] [c000000005d3bba0] [c000000000e81d78] mutex_lock+0x28/0x90 (unreliable) [ 525.647539] [c000000005d3bbd0] [c0080000001f5da0] tpm2_del_space+0x48/0x130 [tpm] [ 525.647635] [c000000005d3bc20] [c0080000001f56b8] tpmrm_release+0x40/0x70 [tpm] [ 525.647746] [c000000005d3bc50] [c0000000004bf718] __fput+0xb8/0x340 [ 525.647842] [c000000005d3bca0] [c00000000017def4] task_work_run+0xe4/0x150 [ 525.647930] [c000000005d3bcf0] [c00000000001feb4] do_notify_resume+0x484/0x4f0 [ 525.648023] [c000000005d3bdb0] [c000000000033a64] syscall_exit_prepare+0x1d4/0x330 [ 525.648115] [c000000005d3be20] [c00000000000d96c] system_call_common+0xfc/0x27c
On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
To clarify: When I tested this I had *both* patches applied. Without the patches I got the null pointer exception in tpm2_del_space(). The 2nd patch alone solves that issue when using the steps above.
Yes, I can't confirm the bug either. I only have lpc tis devices, so it could be something to do with spi, but when I do
python3 in one shell
fd = open("/dev/tpmrm0", "wb")
do rmmod tpm_tis in another shell
buf = bytearray(20) fd.write(buf)
20
so I don't see the oops you see on write. However
fd.close()
And it oopses here in tpm2_del_space
James
Hi,
On 05.02.21 03:01, James Bottomley wrote:
On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
To clarify: When I tested this I had *both* patches applied. Without the patches I got the null pointer exception in tpm2_del_space(). The 2nd patch alone solves that issue when using the steps above.
Yes, I can't confirm the bug either. I only have lpc tis devices, so it could be something to do with spi, but when I do
python3 in one shell
fd = open("/dev/tpmrm0", "wb")
do rmmod tpm_tis in another shell
buf = bytearray(20) fd.write(buf)
20
The issue is in the TPM chip driver code, so AFAIU it should not matter whether its SPI or something else. Maybe check again, that the file is still open when tpm_tis is removed and the write actually comes after the rmmod? Also note that there are some sanity checks in tpm_common_write() that the written data has to pass to get to the point where tpm_try_get_ops() is called, which is the call that eventually triggers the bug.
so I don't see the oops you see on write. However
fd.close()
And it oopses here in tpm2_del_space
James
Regards, Lino
On 2/4/21 9:01 PM, James Bottomley wrote:
On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
To clarify: When I tested this I had *both* patches applied. Without the patches I got the null pointer exception in tpm2_del_space(). The 2nd patch alone solves that issue when using the steps above.
Yes, I can't confirm the bug either. I only have lpc tis devices, so it could be something to do with spi, but when I do
I can confirm this bug:
insmod /usr/lib/modules/5.10.0+/extra/tpm.ko ; insmod /usr/lib/modules/5.10.0+/extra/tpm_vtpm_proxy.ko
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm0
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
echo -en '\x80\x01\x00\x00\x00\x0c\x00\x00\x01\x44\x00\x00' >&100
[ 167.289390] [c000000015d6fb60] [c0000000007d3ac0] refcount_warn_saturate+0x210/0x230 (unreliable) [ 167.290392] [c000000015d6fbc0] [c000000000831328] kobject_put+0x1b8/0x2e0 [ 167.291398] [c000000015d6fc50] [c000000000955548] put_device+0x28/0x40 [ 167.292409] [c000000015d6fc70] [c0080000008609a8] tpm_try_get_ops+0xb0/0x100 [tpm] [ 167.293417] [c000000015d6fcb0] [c008000000861864] tpm_common_write+0x15c/0x250 [tpm] [ 167.294429] [c000000015d6fd20] [c0000000004be190] vfs_write+0xf0/0x380 [ 167.295437] [c000000015d6fd70] [c0000000004be6c8] ksys_write+0x78/0x130 [ 167.296450] [c000000015d6fdc0] [c00000000003377c] system_call_exception+0x15c/0x270 [ 167.297461] [c000000015d6fe20] [c00000000000d960] system_call_common+0xf0/0x27c
With this patch applied this error here is gone. Just have make sure to replace tpm.ko and tpm_vtpm_proxy.ko, not just the latter.
So my Tested-By is good for both patches.
Stefan
Hi Stefan,
On 05.02.21 01:46, Stefan Berger wrote:
On 2/4/21 6:50 PM, Lino Sanfilippo wrote:
Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
Tested-by: Stefan Berger stefanb@linux.ibm.com
Steps:
modprobe tpm_vtpm_proxy
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm1
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
exec 100>&- # fails before, works after --> great job! :-)
Great, thank you very much for testing!
Best Regards, Lino
On Fri, Feb 05, 2021 at 12:50:42AM +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- Write a TPM command to the file descriptor opened at step 1.
------------[ cut here ]------------ WARNING: CPU: 3 PID: 1161 at lib/refcount.c:25 kobject_get+0xa0/0xa4 refcount_t: addition on 0; use-after-free. Modules linked in: tpm_tis_spi tpm_tis_core tpm mdio_bcm_unimac brcmfmac sha256_generic libsha256 sha256_arm hci_uart btbcm bluetooth cfg80211 vc4 brcmutil ecdh_generic ecc snd_soc_core crc32_arm_ce libaes raspberrypi_hwmon ac97_bus snd_pcm_dmaengine bcm2711_thermal snd_pcm snd_timer genet snd phy_generic soundcore [last unloaded: spi_bcm2835] CPU: 3 PID: 1161 Comm: hold_open Not tainted 5.10.0ls-main-dirty #2 Hardware name: BCM2711 [<c0410c3c>] (unwind_backtrace) from [<c040b580>] (show_stack+0x10/0x14) [<c040b580>] (show_stack) from [<c1092174>] (dump_stack+0xc4/0xd8) [<c1092174>] (dump_stack) from [<c0445a30>] (__warn+0x104/0x108) [<c0445a30>] (__warn) from [<c0445aa8>] (warn_slowpath_fmt+0x74/0xb8) [<c0445aa8>] (warn_slowpath_fmt) from [<c08435d0>] (kobject_get+0xa0/0xa4) [<c08435d0>] (kobject_get) from [<bf0a715c>] (tpm_try_get_ops+0x14/0x54 [tpm]) [<bf0a715c>] (tpm_try_get_ops [tpm]) from [<bf0a7d6c>] (tpm_common_write+0x38/0x60 [tpm]) [<bf0a7d6c>] (tpm_common_write [tpm]) from [<c05a7ac0>] (vfs_write+0xc4/0x3c0) [<c05a7ac0>] (vfs_write) from [<c05a7ee4>] (ksys_write+0x58/0xcc) [<c05a7ee4>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xc226bfa8 to 0xc226bff0) bfa0: 00000000 000105b4 00000003 beafe664 00000014 00000000 bfc0: 00000000 000105b4 000103f8 00000004 00000000 00000000 b6f9c000 beafe684 bfe0: 0000006c beafe648 0001056c b6eb6944 ---[ end trace d4b8409def9b8b1f ]---
The reason for this warning is the attempt to get the chip->dev reference in tpm_common_write() although the reference counter is already zero.
Since commit 8979b02aaf1d ("tpm: Fix reference count to main device") the extra reference used to prevent a premature zero counter is never taken, because the required TPM_CHIP_FLAG_TPM2 flag is never set.
Fix this by removing the flag condition.
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Fix this also by installing an action handler that puts chip->devs as soon as the chip is unregistered.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
drivers/char/tpm/tpm-chip.c | 18 +++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 2 ++ drivers/char/tpm/tpm_vtpm_proxy.c | 1 + 3 files changed, 18 insertions(+), 3 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
On Fri, Feb 05, 2021 at 12:50:42AM +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- Write a TPM command to the file descriptor opened at step 1.
WARNING: CPU: 3 PID: 1161 at lib/refcount.c:25 kobject_get+0xa0/0xa4 refcount_t: addition on 0; use-after-free. Modules linked in: tpm_tis_spi tpm_tis_core tpm mdio_bcm_unimac brcmfmac sha256_generic libsha256 sha256_arm hci_uart btbcm bluetooth cfg80211 vc4 brcmutil ecdh_generic ecc snd_soc_core crc32_arm_ce libaes raspberrypi_hwmon ac97_bus snd_pcm_dmaengine bcm2711_thermal snd_pcm snd_timer genet snd phy_generic soundcore [last unloaded: spi_bcm2835] CPU: 3 PID: 1161 Comm: hold_open Not tainted 5.10.0ls-main-dirty #2 Hardware name: BCM2711 [<c0410c3c>] (unwind_backtrace) from [<c040b580>] (show_stack+0x10/0x14) [<c040b580>] (show_stack) from [<c1092174>] (dump_stack+0xc4/0xd8) [<c1092174>] (dump_stack) from [<c0445a30>] (__warn+0x104/0x108) [<c0445a30>] (__warn) from [<c0445aa8>] (warn_slowpath_fmt+0x74/0xb8) [<c0445aa8>] (warn_slowpath_fmt) from [<c08435d0>] (kobject_get+0xa0/0xa4) [<c08435d0>] (kobject_get) from [<bf0a715c>] (tpm_try_get_ops+0x14/0x54 [tpm]) [<bf0a715c>] (tpm_try_get_ops [tpm]) from [<bf0a7d6c>] (tpm_common_write+0x38/0x60 [tpm]) [<bf0a7d6c>] (tpm_common_write [tpm]) from [<c05a7ac0>] (vfs_write+0xc4/0x3c0) [<c05a7ac0>] (vfs_write) from [<c05a7ee4>] (ksys_write+0x58/0xcc) [<c05a7ee4>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) Exception stack(0xc226bfa8 to 0xc226bff0) bfa0: 00000000 000105b4 00000003 beafe664 00000014 00000000 bfc0: 00000000 000105b4 000103f8 00000004 00000000 00000000 b6f9c000 beafe684 bfe0: 0000006c beafe648 0001056c b6eb6944
The reason for this warning is the attempt to get the chip->dev reference in tpm_common_write() although the reference counter is already zero.
Since commit 8979b02aaf1d ("tpm: Fix reference count to main device") the extra reference used to prevent a premature zero counter is never taken, because the required TPM_CHIP_FLAG_TPM2 flag is never set.
Fix this by removing the flag condition.
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Seems wonky, the devs is just supposed to be a side thing, nothing should be using it as a primary reference count for a tpm.
The bug here is only that tpm_common_open() did not get a kref on the chip before putting it in priv and linking it to the fd. See the comment before tpm_try_get_ops() indicating the caller must already have taken care to ensure the chip is valid.
This should be all you need to fix the oops:
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c index 1784530b8387bb..1b738dca7fffb5 100644 --- a/drivers/char/tpm/tpm-dev-common.c +++ b/drivers/char/tpm/tpm-dev-common.c @@ -105,6 +105,7 @@ static void tpm_timeout_work(struct work_struct *work) void tpm_common_open(struct file *file, struct tpm_chip *chip, struct file_priv *priv, struct tpm_space *space) { + get_device(&priv->chip.dev); priv->chip = chip; priv->space = space; priv->response_read = true; @@ -261,6 +262,7 @@ void tpm_common_release(struct file *file, struct file_priv *priv) flush_work(&priv->timeout_work); file->private_data = NULL; priv->response_length = 0; + put_device(&chip->dev); }
int __init tpm_dev_common_init(void)
Fix this also by installing an action handler that puts chip->devs as soon as the chip is unregistered.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com drivers/char/tpm/tpm-chip.c | 18 +++++++++++++++--- drivers/char/tpm/tpm_ftpm_tee.c | 2 ++ drivers/char/tpm/tpm_vtpm_proxy.c | 1 + 3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb..3ace199 100644 +++ b/drivers/char/tpm/tpm-chip.c @@ -360,8 +360,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, * while cdevs is in use. The corresponding put * is in the tpm_devs_release (TPM2 only) */
- 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); @@ -422,8 +421,21 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev, rc = devm_add_action_or_reset(pdev, (void (*)(void *)) put_device, &chip->dev);
- if (rc)
- if (rc) {
put_device(&chip->devs);
This isn't right read what 'or_reset' does
Jason
Hi,
On 05.02.21 14:05, Jason Gunthorpe wrote:
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Seems wonky, the devs is just supposed to be a side thing, nothing should be using it as a primary reference count for a tpm.
The bug here is only that tpm_common_open() did not get a kref on the chip before putting it in priv and linking it to the fd. See the comment before tpm_try_get_ops() indicating the caller must already have taken care to ensure the chip is valid.
This should be all you need to fix the oops:
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c index 1784530b8387bb..1b738dca7fffb5 100644 --- a/drivers/char/tpm/tpm-dev-common.c +++ b/drivers/char/tpm/tpm-dev-common.c @@ -105,6 +105,7 @@ static void tpm_timeout_work(struct work_struct *work) void tpm_common_open(struct file *file, struct tpm_chip *chip, struct file_priv *priv, struct tpm_space *space) {
get_device(&priv->chip.dev); priv->chip = chip; priv->space = space; priv->response_read = true;
This is racy, isnt it? The time between we open the file and we want to grab the reference in common_open() the chip can already be unregistered and freed.
As a matter of fact this solution was the first thing that came into my mind, too, until I noticed the possible race condition. I can only guess that this was what James had in mind when he chose to take the extra reference to chip->dev in tpm_chip_alloc() instead of common_open().
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb..3ace199 100644 +++ b/drivers/char/tpm/tpm-chip.c @@ -360,8 +360,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, * while cdevs is in use. The corresponding put * is in the tpm_devs_release (TPM2 only) */
- 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); @@ -422,8 +421,21 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev, rc = devm_add_action_or_reset(pdev, (void (*)(void *)) put_device, &chip->dev);
- if (rc)
- if (rc) {
put_device(&chip->devs);This isn't right read what 'or_reset' does
In case of failure installing the action handler devm_add_action_or_reset() puts chip->dev for us. But we also have put chip->devs since we have retrieved a reference to both chip->dev and chip->devs. Or do I miss something here?
Jason
Regards, Lino
On Fri, Feb 05, 2021 at 03:55:09PM +0100, Lino Sanfilippo wrote:
Hi,
On 05.02.21 14:05, Jason Gunthorpe wrote:
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") already introduced function tpm_devs_release() to release the extra reference but did not implement the required put on chip->devs that results in the call of this function.
Seems wonky, the devs is just supposed to be a side thing, nothing should be using it as a primary reference count for a tpm.
The bug here is only that tpm_common_open() did not get a kref on the chip before putting it in priv and linking it to the fd. See the comment before tpm_try_get_ops() indicating the caller must already have taken care to ensure the chip is valid.
This should be all you need to fix the oops:
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c index 1784530b8387bb..1b738dca7fffb5 100644 +++ b/drivers/char/tpm/tpm-dev-common.c @@ -105,6 +105,7 @@ static void tpm_timeout_work(struct work_struct *work) void tpm_common_open(struct file *file, struct tpm_chip *chip, struct file_priv *priv, struct tpm_space *space) {
get_device(&priv->chip.dev); priv->chip = chip; priv->space = space; priv->response_read = true;This is racy, isnt it? The time between we open the file and we want to grab the reference in common_open() the chip can already be unregistered and freed.
No, the cdev layer holds the refcount on the device while open is being called.
Jason
On 05.02.21 16:15, Jason Gunthorpe wrote:
No, the cdev layer holds the refcount on the device while open is being called.
Jason
Yes, but the reference that is responsible for the chip deallocation is chip->dev which is linked to chip->cdev and represents /dev/tpm, not /dev/tpmrm. You are right, we dont have the issue with /dev/tpm for the reason you mentioned. But /dev/tpmrm is represented by chip->cdevs and keeping this ref held by the cdev layer wont protect us from the chip being freed (which is the reason why we need the chip->dev reference in the first place).
And yes, the naming dev/devs/cdev/cdevs is quite confusing :(
Regards, Lino
On Fri, Feb 05, 2021 at 04:50:13PM +0100, Lino Sanfilippo wrote:
On 05.02.21 16:15, Jason Gunthorpe wrote:
No, the cdev layer holds the refcount on the device while open is being called.
Yes, but the reference that is responsible for the chip deallocation is chip->dev which is linked to chip->cdev and represents /dev/tpm, not /dev/tpmrm. You are right, we dont have the issue with /dev/tpm for the reason you mentioned. But /dev/tpmrm is represented by chip->cdevs and keeping this ref held by the cdev layer wont protect us from the chip being freed (which is the reason why we need the chip->dev reference in the first place).
No, they are all chained together because they are all in the same struct:
struct tpm_chip { struct device dev; struct device devs; struct cdev cdev; struct cdev cdevs;
dev holds the refcount on memory, when it goes 0 the whole thing is kfreed.
The rule is dev's refcount can't go to zero while any other refcount is != 0.
For instance devs holds a get on dev that is put back only when devs goes to 0:
static void tpm_devs_release(struct device *dev) { struct tpm_chip *chip = container_of(dev, struct tpm_chip, devs);
/* release the master device reference */ put_device(&chip->dev); }
Both cdev elements do something similar inside the cdev layer.
The net result is during any open() the tpm_chip is guarenteed to have a positive refcount.
Jason
On 05.02.21 at 16:58, Jason Gunthorpe wrote: eference in the first place).
No, they are all chained together because they are all in the same struct:
struct tpm_chip { struct device dev; struct device devs; struct cdev cdev; struct cdev cdevs;
dev holds the refcount on memory, when it goes 0 the whole thing is kfreed.
The rule is dev's refcount can't go to zero while any other refcount is != 0.
For instance devs holds a get on dev that is put back only when devs goes to 0:
static void tpm_devs_release(struct device *dev) { struct tpm_chip *chip = container_of(dev, struct tpm_chip, devs);
/* release the master device reference */ put_device(&chip->dev); }
Both cdev elements do something similar inside the cdev layer.
Well this chaining is exactly what does not work nowadays and what the patch is supposed to fix: currently we dont ever take the extra ref (not even in TPM 2 case, note that TPM_CHIP_FLAG_TMP2 is never set), so
- if (chip->flags & TPM_CHIP_FLAG_TPM2) - get_device(&chip->dev); + get_device(&chip->dev);
and tpm_devs_release() is never called, since there is nothing that ever puts devs, so
+ rc = devm_add_action_or_reset(pdev, + (void (*)(void *)) put_device, + &chip->devs);
The race with only get_device()/putdevice() in tpm_common_open()/tpm_common_release() is:
1. tpm chip is allocated with dev refcount = 1, devs refcount = 1 2. /dev/tpmrm is opened but before we get the ref to dev in tpm_common() another thread rmmmods the chip driver: 3. the chip is unregistered, dev is put with refcount = 0 and the whole chip struct is freed 3. Now open() proceeds, tries to grab the extra ref chip->dev from a chip that has already been deallocated and the system crashes.
As I already wrote, that approach was my first thought, too, but since the result crashed due to the race condition, I chose the approach in patch 1.
Regards, Lino
The net result is during any open() the tpm_chip is guarenteed to have a positive refcount.
On Fri, Feb 05, 2021 at 10:50:02PM +0100, Lino Sanfilippo wrote:
On 05.02.21 at 16:58, Jason Gunthorpe wrote: eference in the first place).
No, they are all chained together because they are all in the same struct:
struct tpm_chip { struct device dev; struct device devs; struct cdev cdev; struct cdev cdevs;
dev holds the refcount on memory, when it goes 0 the whole thing is kfreed.
The rule is dev's refcount can't go to zero while any other refcount is != 0.
For instance devs holds a get on dev that is put back only when devs goes to 0:
static void tpm_devs_release(struct device *dev) { struct tpm_chip *chip = container_of(dev, struct tpm_chip, devs);
/* release the master device reference */ put_device(&chip->dev); }
Both cdev elements do something similar inside the cdev layer.
Well this chaining is exactly what does not work nowadays and what the patch is supposed to fix: currently we dont ever take the extra ref (not even in TPM 2 case, note that TPM_CHIP_FLAG_TMP2 is never set), so
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
get_device(&chip->dev);
- get_device(&chip->dev);
Oh, hah, yes that is busted up. The patch sketch I sent to James is the right way to handle it, feel free to take it up
and tpm_devs_release() is never called, since there is nothing that ever puts devs, so
Yes, that is a pre-existing memory leak
The race with only get_device()/putdevice() in tpm_common_open()/tpm_common_release() is:
The refcount handling is busted up and not working the way it is designed, when that is fixed there is no race.
Jason
From: Lino Sanfilippo l.sanfilippo@kunbus.com
In tpm2_del_space() chip->ops is used for flushing the sessions. However this function may be called after tpm_chip_unregister() which sets the chip->ops pointer to NULL. Avoid a possible NULL pointer dereference by checking if chip->ops is still valid before accessing it.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com --- drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c index 784b8b3..9a29a40 100644 --- a/drivers/char/tpm/tpm2-space.c +++ b/drivers/char/tpm/tpm2-space.c @@ -58,12 +58,17 @@ 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)) { - tpm2_flush_sessions(chip, space); - tpm_chip_stop(chip); + down_read(&chip->ops_sem); + if (chip->ops) { + mutex_lock(&chip->tpm_mutex); + if (!tpm_chip_start(chip)) { + tpm2_flush_sessions(chip, space); + tpm_chip_stop(chip); + } + mutex_unlock(&chip->tpm_mutex); } - mutex_unlock(&chip->tpm_mutex); + up_read(&chip->ops_sem); + kfree(space->context_buf); kfree(space->session_buf); }
On Fri, 2021-02-05 at 00:50 +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
In tpm2_del_space() chip->ops is used for flushing the sessions. However this function may be called after tpm_chip_unregister() which sets the chip->ops pointer to NULL. Avoid a possible NULL pointer dereference by checking if chip->ops is still valid before accessing it.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2- space.c index 784b8b3..9a29a40 100644 --- a/drivers/char/tpm/tpm2-space.c +++ b/drivers/char/tpm/tpm2-space.c @@ -58,12 +58,17 @@ 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)) {
tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);
- down_read(&chip->ops_sem);
- if (chip->ops) {
mutex_lock(&chip->tpm_mutex);if (!tpm_chip_start(chip)) {tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);}mutex_unlock(&chip->tpm_mutex);
- mutex_unlock(&chip->tpm_mutex);
- up_read(&chip->ops_sem);
- kfree(space->context_buf); kfree(space->session_buf);
}
Actually, this still isn't right. As I said to the last person who reported this, we should be doing a get/put on the ops, not rolling our own here:
https://lore.kernel.org/linux-integrity/e7566e1e48f5be9dca034b4bfb67683b5d3c...
The reporter went silent before we could get this tested, but could you try, please, because your patch is still hand rolling the ops get/put, just slightly better than it had been done previously.
James
On Thu, Feb 04, 2021 at 04:34:11PM -0800, James Bottomley wrote:
On Fri, 2021-02-05 at 00:50 +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
In tpm2_del_space() chip->ops is used for flushing the sessions. However this function may be called after tpm_chip_unregister() which sets the chip->ops pointer to NULL. Avoid a possible NULL pointer dereference by checking if chip->ops is still valid before accessing it.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2- space.c index 784b8b3..9a29a40 100644 --- a/drivers/char/tpm/tpm2-space.c +++ b/drivers/char/tpm/tpm2-space.c @@ -58,12 +58,17 @@ 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)) {
tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);
- down_read(&chip->ops_sem);
- if (chip->ops) {
mutex_lock(&chip->tpm_mutex);if (!tpm_chip_start(chip)) {tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);}mutex_unlock(&chip->tpm_mutex);
- mutex_unlock(&chip->tpm_mutex);
- up_read(&chip->ops_sem);
- kfree(space->context_buf); kfree(space->session_buf);
}
Actually, this still isn't right. As I said to the last person who reported this, we should be doing a get/put on the ops, not rolling our own here:
https://lore.kernel.org/linux-integrity/e7566e1e48f5be9dca034b4bfb67683b5d3c...
The reporter went silent before we could get this tested, but could you try, please, because your patch is still hand rolling the ops get/put, just slightly better than it had been done previously.
James
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Honestly, I have to admit that this thread leaked from me. It happened exactly at the time when I was on vacation. Something must have gone wrong when I pulled emails after that.
/Jarkko
On Fri, 2021-02-05 at 04:18 +0200, Jarkko Sakkinen wrote:
On Thu, Feb 04, 2021 at 04:34:11PM -0800, James Bottomley wrote:
On Fri, 2021-02-05 at 00:50 +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
In tpm2_del_space() chip->ops is used for flushing the sessions. However this function may be called after tpm_chip_unregister() which sets the chip->ops pointer to NULL. Avoid a possible NULL pointer dereference by checking if chip-
ops is
still valid before accessing it.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2- space.c index 784b8b3..9a29a40 100644 --- a/drivers/char/tpm/tpm2-space.c +++ b/drivers/char/tpm/tpm2-space.c @@ -58,12 +58,17 @@ 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)) {
tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);
- down_read(&chip->ops_sem);
- if (chip->ops) {
mutex_lock(&chip->tpm_mutex);if (!tpm_chip_start(chip)) {tpm2_flush_sessions(chip, space);tpm_chip_stop(chip);}mutex_unlock(&chip->tpm_mutex);
- mutex_unlock(&chip->tpm_mutex);
- up_read(&chip->ops_sem);
- kfree(space->context_buf); kfree(space->session_buf);
}
Actually, this still isn't right. As I said to the last person who reported this, we should be doing a get/put on the ops, not rolling our own here:
https://lore.kernel.org/linux-integrity/e7566e1e48f5be9dca034b4bfb67683b5d3c...
The reporter went silent before we could get this tested, but could you try, please, because your patch is still hand rolling the ops get/put, just slightly better than it had been done previously.
James
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need.
To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
tpm 2 is special because we have two character device nodes: /dev/tpm0 and /dev/tpmrm0. The way we make this work is that tpm0 is the master and tpmrm0 the slave, so the slave holds an extra reference on the master which is put when the slave final put happens. This means that our resources aren't freed until the final puts of both devices, which is the model we're using.
The practical consequence of this model is that if you allocate a chip structure with tpm_chip_alloc() you have to release it again by doing a put of *both* devices.
However, patch fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") contains two bugs: firstly it didn't add a devm action release for devs and secondly it didn't update the only non-devm user ibm vtpm to do the double put.
Stefan noticed the latter, so we got the bogus patch 8979b02aaf1d ("tpm: Fix reference count to main device") applied which simply breaks the master/slave model by not taking a reference on the master for the slave. I'm not sure why I didn't notice the problem with this fix at the time, but attention must have been elsewhere.
Subsequently we got ftpm added which copied the ibm vtpm put bug.
So I think 1/2 is the correct fix for all three bugs. I just need to find a way to verify it.
James
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need.
To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
The refcount on the struct device only keeps the memory alive, it doesn't say anything about the ops. We still need to lock and check the ops each and every time they are used.
The fact cdev goes all the way till fput means we don't need the extra get/put I suggested to Lino at all.
The practical consequence of this model is that if you allocate a chip structure with tpm_chip_alloc() you have to release it again by doing a put of *both* devices.
The final put of the devs should be directly after the cdev_device_del(), not in a devm. This became all confused because the devs was created during alloc, not register. Having a device that is initialized but will never be added is weird.
See sketch below.
Stefan noticed the latter, so we got the bogus patch 8979b02aaf1d ("tpm: Fix reference count to main device") applied which simply breaks the master/slave model by not taking a reference on the master for the slave. I'm not sure why I didn't notice the problem with this fix at the time, but attention must have been elsewhere.
Well, this is sort of OK because we never use the devs in TPM1, so we end up freeing the chip with a positive refcount on the devs, which is weird but not a functional bug.
Jason
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e10910..e07193a0dd4438 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -344,7 +344,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev_num = rc;
device_initialize(&chip->dev); - device_initialize(&chip->devs);
chip->dev.class = tpm_class; chip->dev.class->shutdown_pre = tpm_class_shutdown; @@ -352,29 +351,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev.parent = pdev; chip->dev.groups = chip->groups;
- chip->devs.parent = pdev; - chip->devs.class = tpmrm_class; - chip->devs.release = tpm_devs_release; - /* 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) - */ - if (chip->flags & TPM_CHIP_FLAG_TPM2) - get_device(&chip->dev); - if (chip->dev_num == 0) chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR); else chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
- chip->devs.devt = - MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES); - rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num); - if (rc) - goto out; - rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num); if (rc) goto out;
@@ -382,9 +364,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
cdev_init(&chip->cdev, &tpm_fops); - cdev_init(&chip->cdevs, &tpmrm_fops); chip->cdev.owner = THIS_MODULE; - chip->cdevs.owner = THIS_MODULE;
rc = tpm2_init_space(&chip->work_space, TPM2_SPACE_BUFFER_SIZE); if (rc) { @@ -396,7 +376,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, return chip;
out: - put_device(&chip->devs); put_device(&chip->dev); return ERR_PTR(rc); } @@ -445,13 +424,33 @@ static int tpm_add_char_device(struct tpm_chip *chip) }
if (chip->flags & TPM_CHIP_FLAG_TPM2) { + device_initialize(&chip->devs); + chip->devs.parent = pdev; + chip->devs.class = tpmrm_class; + rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num); + if (rc) + goto out_put_devs; + + /* + * 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. + */ + get_device(&chip->dev); + chip->devs.release = tpm_devs_release; + + chip->devs.devt = + MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES); + cdev_init(&chip->cdevs, &tpmrm_fops); + chip->cdevs.owner = THIS_MODULE; + rc = cdev_device_add(&chip->cdevs, &chip->devs); if (rc) { dev_err(&chip->devs, "unable to cdev_device_add() %s, major %d, minor %d, err=%d\n", dev_name(&chip->devs), MAJOR(chip->devs.devt), MINOR(chip->devs.devt), rc); - return rc; + goto out_put_devs; } }
@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock);
+out_put_devs: + put_device(&chip->devs); +out_del_dev: + cdev_device_del(&chip->cdev); return rc; }
@@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip); - if (chip->flags & TPM_CHIP_FLAG_TPM2) + if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs); + put_device(&chip->devs); + } tpm_del_char_device(chip); } EXPORT_SYMBOL_GPL(tpm_chip_unregister);
On Fri, 2021-02-05 at 13:25 -0400, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need. To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
The refcount on the struct device only keeps the memory alive, it doesn't say anything about the ops. We still need to lock and check the ops each and every time they are used.
I think this is the crux of our disagreement: I think the ops doesn't matter because to call try_get_ops you have to have a chip structure and the only way you get a chip structure is if you hold a device containing it, in which case the device hold guarantees the chip can't be freed. Or if you pass in TPM_ANY_NUM to an operation which calls tpm_chip_find_get() which iterates the idr to find a chip under the idr lock. If you find a chip device at the idr, you're guaranteed it exists, because elimination of it is the first thing the release does and if you find a dying dev (i.e. the release routine blocks on the idr mutex trying to kill the chip attachment), try_get_ops() fails because the ops are already NULL.
In either case, I think you get returned a device to which you hold a reference. Is there any other case where you can get a chip without also getting a device reference?
I'll answer the other point in a separate email, but I think the principle sounds OK: we could do the final put right after we del the char devices because that's called in the module release routine and thus not have to rely on the devm actions which, as you say, are an annoying complication.
James
On Fri, Feb 05, 2021 at 09:54:29AM -0800, James Bottomley wrote:
On Fri, 2021-02-05 at 13:25 -0400, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need. To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
The refcount on the struct device only keeps the memory alive, it doesn't say anything about the ops. We still need to lock and check the ops each and every time they are used.
I think this is the crux of our disagreement: I think the ops doesn't matter because to call try_get_ops you have to have a chip structure and the only way you get a chip structure is if you hold a device containing it, in which case the device hold guarantees the chip can't be freed.
The get_device() only guarentees the chip memory hasn't been kfree'd.
It doesn't mean tpm_chip_unregister() hasn't already run, completed and set ops == NULL.
In the file path we have the get_device implicitly by the file's i_cdev pointing to that chain of refcounts that ends on the chip's main struct device. So we know the chip memory cannot be kfreed while the struct file exists.
However, there is nothing preventing the struct file from living past tpm_chip_unregister(). cdev_device_del() does not wait for all files's to be closed, it only removes the ability to open new files. Open files do prevent removal of the module, but it does not prevent hot-unplug of the underling device, eg with sysfs unbind.
In fact, nothing about tpm_chip_unregister() excludes open files.
So it is perfectly legal for tpm_chip_unregister() to return, the devm put_device to be called, and the refcount of the chip to still be positive - held by open files.
In this situation ops will be NULL when file operations are called and eg doing a tpm_chip_start will crash on:
if (chip->ops->clk_enable)
To use the TPM driver, the rules are you must hold a get_device() on a chip, and then upgrade it to a 'tpm_try_get_ops' before calling any driver functions.
Only the critical region formed by tpm_try_get_ops() will prevent tpm_chip_unregister() from completing. It is the thing that ensures the driver is actually present.
In either case, I think you get returned a device to which you hold a reference. Is there any other case where you can get a chip without also getting a device reference?
There should be no case where there is a struct chip pointer without something owning a reference for that pointer.
I'll answer the other point in a separate email, but I think the principle sounds OK: we could do the final put right after we del the char devices because that's called in the module release routine and thus not have to rely on the devm actions which, as you say, are an annoying complication.
I think tpm_alloc() should have an error unwind that is only put_device(chip->dev), anything else breaks the basic programming pattern of alloc/register.
Jason
On Fri, 2021-02-05 at 13:25 -0400, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
[...]
The practical consequence of this model is that if you allocate a chip structure with tpm_chip_alloc() you have to release it again by doing a put of *both* devices.
The final put of the devs should be directly after the cdev_device_del(), not in a devm. This became all confused because the devs was created during alloc, not register. Having a device that is initialized but will never be added is weird.
See sketch below.
Stefan noticed the latter, so we got the bogus patch 8979b02aaf1d ("tpm: Fix reference count to main device") applied which simply breaks the master/slave model by not taking a reference on the master for the slave. I'm not sure why I didn't notice the problem with this fix at the time, but attention must have been elsewhere.
Well, this is sort of OK because we never use the devs in TPM1, so we end up freeing the chip with a positive refcount on the devs, which is weird but not a functional bug.
Jason
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm- chip.c index ddaeceb7e10910..e07193a0dd4438 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -344,7 +344,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev_num = rc; device_initialize(&chip->dev);
- device_initialize(&chip->devs);
chip->dev.class = tpm_class; chip->dev.class->shutdown_pre = tpm_class_shutdown; @@ -352,29 +351,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev.parent = pdev; chip->dev.groups = chip->groups;
- chip->devs.parent = pdev;
- chip->devs.class = tpmrm_class;
- chip->devs.release = tpm_devs_release;
- /* 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)*/- if (chip->flags & TPM_CHIP_FLAG_TPM2)
get_device(&chip->dev);- if (chip->dev_num == 0) chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR); else chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
- chip->devs.devt =
MKDEV(MAJOR(tpm_devt), chip->dev_num +TPM_NUM_DEVICES);
- rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
- if (rc)
goto out;- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num); if (rc) goto out;
@@ -382,9 +364,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->flags |= TPM_CHIP_FLAG_VIRTUAL; cdev_init(&chip->cdev, &tpm_fops);
- cdev_init(&chip->cdevs, &tpmrm_fops); chip->cdev.owner = THIS_MODULE;
- chip->cdevs.owner = THIS_MODULE;
rc = tpm2_init_space(&chip->work_space, TPM2_SPACE_BUFFER_SIZE); if (rc) { @@ -396,7 +376,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, return chip; out:
- put_device(&chip->devs); put_device(&chip->dev); return ERR_PTR(rc);
} @@ -445,13 +424,33 @@ static int tpm_add_char_device(struct tpm_chip *chip) } if (chip->flags & TPM_CHIP_FLAG_TPM2) {
device_initialize(&chip->devs);chip->devs.parent = pdev;chip->devs.class = tpmrm_class;rc = dev_set_name(&chip->devs, "tpmrm%d", chip-dev_num);
if (rc)goto out_put_devs;/** get extra reference on main device to hold onbehalf of devs.
* This holds the chip structure while cdevs is inuse. The
* corresponding put is in the tpm_devs_release.*/get_device(&chip->dev);chip->devs.release = tpm_devs_release;chip->devs.devt =MKDEV(MAJOR(tpm_devt), chip->dev_num +TPM_NUM_DEVICES);
cdev_init(&chip->cdevs, &tpmrm_fops);chip->cdevs.owner = THIS_MODULE;
Effectively all of this shuffles the tpmrm device allocation from chip_alloc to chip_add ... I'm not averse to this but it does mean we can suffer allocation failures now in the add routine and it makes error handling a bit more complex. On the other hand we can now check the TPM2 flag correctly, so it's swings and roundabouts.
rc = cdev_device_add(&chip->cdevs, &chip->devs); if (rc) { dev_err(&chip->devs, "unable to cdev_device_add() %s, major%d, minor %d, err=%d\n", dev_name(&chip->devs), MAJOR(chip-
devs.devt),
MINOR(chip->devs.devt), rc);
return rc;
goto out_put_devs;@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock); +out_put_devs:
- put_device(&chip->devs);
I think there should be a if (chip->flags & TPM_CHIP_FLAG_TPM2) here.
I realise you got everything semantically correct and you only ever go to this label from somewhere that already has the check, but guess what will happen when the bot rewriters get hold of this ...
+out_del_dev:
- cdev_device_del(&chip->cdev); return rc;
} @@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
Actually, I think you want to go further here. If there's a
put_device(&chips->dev)
as the last statement (or moved into tpm_del_char_device) we should now have no active reference on the devices from the kernel and we can eliminate the
rc = devm_add_action_or_reset(pdev, (void (*)(void *)) put_device, &chip->dev);
In tpmm_chip_alloc(). That way both /dev/tpm and /dev/tpmrm have identical lifetime properties.
James
On Fri, Feb 05, 2021 at 05:08:20PM -0800, James Bottomley wrote:
Effectively all of this shuffles the tpmrm device allocation from chip_alloc to chip_add ... I'm not averse to this but it does mean we can suffer allocation failures now in the add routine and it makes error handling a bit more complex.
We already have to handle failures here, so this doesn't seem any worse (and the existing error handling looked wrong, I fixed it)
rc = cdev_device_add(&chip->cdevs, &chip->devs); if (rc) { dev_err(&chip->devs, "unable to cdev_device_add() %s, major%d, minor %d, err=%d\n", dev_name(&chip->devs), MAJOR(chip-
devs.devt),
MINOR(chip->devs.devt), rc);
return rc;
goto out_put_devs;@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock); +out_put_devs:
- put_device(&chip->devs);
I think there should be a if (chip->flags & TPM_CHIP_FLAG_TPM2) here.
I realise you got everything semantically correct and you only ever go to this label from somewhere that already has the check, but guess what will happen when the bot rewriters get hold of this ...
Makes sense
+out_del_dev:
- cdev_device_del(&chip->cdev); return rc;
} @@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
Actually, I think you want to go further here. If there's a
put_device(&chips->dev)
as the last statement (or moved into tpm_del_char_device) we should now
The proper TPM driver remove sequence is:
remove() { /* Upon return the core guarentees no driver callback is running or * will ever run again */ tpm_chip_unregister()
// Safe to do this because nothing will now use the HW resources free_irq(chip->XXX) unmap_memory(chip->YYY)
// Now we are done with the memory put_device(&chip-dev); }
ie the general driver design should expect the chip memory to continue to exist after unregister because it will need to refer to it to destroy any driver resources.
have no active reference on the devices from the kernel and we can eliminate the
rc = devm_add_action_or_reset(pdev, (void (*)(void *)) put_device, &chip->dev);
This devm exists because adding the put_device to the error unwinds of every driver probe function was too daunting. It can be removed only if someone goes and updates every driver to correctly error-unwind tpm_chip_alloc() with put_device() in the driver probe function.
Jason
Hi Jason,
On 05.02.21 18:25, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need.
To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
The refcount on the struct device only keeps the memory alive, it doesn't say anything about the ops. We still need to lock and check the ops each and every time they are used.
The fact cdev goes all the way till fput means we don't need the extra get/put I suggested to Lino at all.
The practical consequence of this model is that if you allocate a chip structure with tpm_chip_alloc() you have to release it again by doing a put of *both* devices.
The final put of the devs should be directly after the cdev_device_del(), not in a devm. This became all confused because the devs was created during alloc, not register. Having a device that is initialized but will never be added is weird.
See sketch below.
Stefan noticed the latter, so we got the bogus patch 8979b02aaf1d ("tpm: Fix reference count to main device") applied which simply breaks the master/slave model by not taking a reference on the master for the slave. I'm not sure why I didn't notice the problem with this fix at the time, but attention must have been elsewhere.
Well, this is sort of OK because we never use the devs in TPM1, so we end up freeing the chip with a positive refcount on the devs, which is weird but not a functional bug.
Jason
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e10910..e07193a0dd4438 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -344,7 +344,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev_num = rc; device_initialize(&chip->dev);
- device_initialize(&chip->devs);
chip->dev.class = tpm_class; chip->dev.class->shutdown_pre = tpm_class_shutdown; @@ -352,29 +351,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev.parent = pdev; chip->dev.groups = chip->groups;
- chip->devs.parent = pdev;
- chip->devs.class = tpmrm_class;
- chip->devs.release = tpm_devs_release;
- /* 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)*/- if (chip->flags & TPM_CHIP_FLAG_TPM2)
get_device(&chip->dev);- if (chip->dev_num == 0) chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR); else chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
- chip->devs.devt =
MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);- rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
- if (rc)
goto out;- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num); if (rc) goto out;
@@ -382,9 +364,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->flags |= TPM_CHIP_FLAG_VIRTUAL; cdev_init(&chip->cdev, &tpm_fops);
- cdev_init(&chip->cdevs, &tpmrm_fops); chip->cdev.owner = THIS_MODULE;
- chip->cdevs.owner = THIS_MODULE;
rc = tpm2_init_space(&chip->work_space, TPM2_SPACE_BUFFER_SIZE); if (rc) { @@ -396,7 +376,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, return chip; out:
- put_device(&chip->devs); put_device(&chip->dev); return ERR_PTR(rc);
} @@ -445,13 +424,33 @@ static int tpm_add_char_device(struct tpm_chip *chip) } if (chip->flags & TPM_CHIP_FLAG_TPM2) {
device_initialize(&chip->devs);chip->devs.parent = pdev;chip->devs.class = tpmrm_class;rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num);if (rc)goto out_put_devs;/** 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.*/get_device(&chip->dev);chip->devs.release = tpm_devs_release;chip->devs.devt =MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);cdev_init(&chip->cdevs, &tpmrm_fops);chip->cdevs.owner = THIS_MODULE;- rc = cdev_device_add(&chip->cdevs, &chip->devs); if (rc) { dev_err(&chip->devs, "unable to cdev_device_add() %s, major %d, minor %d, err=%d\n", dev_name(&chip->devs), MAJOR(chip->devs.devt), MINOR(chip->devs.devt), rc);
return rc;
goto out_put_devs;@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock); +out_put_devs:
- put_device(&chip->devs);
+out_del_dev:
- cdev_device_del(&chip->cdev); return rc;
} @@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
} EXPORT_SYMBOL_GPL(tpm_chip_unregister);
I tested the solution you scetched and it fixes the issue for me. Will you send a (real) patch for this?
Best regards, Lino
On Tue, Feb 09, 2021 at 12:52:17PM +0100, Lino Sanfilippo wrote:
@@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
} EXPORT_SYMBOL_GPL(tpm_chip_unregister);
I tested the solution you scetched and it fixes the issue for me. Will you send a (real) patch for this?
No, feel free to bundle this up with any fixes needed and send it with a Signed-off-by from both of us
I did it pretty fast so it will need a careful read that there isn't a typo
Thanks, Jason
On 09.02.21 14:36, Jason Gunthorpe wrote:
EXPORT_SYMBOL_GPL(tpm_chip_unregister);
I tested the solution you scetched and it fixes the issue for me. Will you send a (real) patch for this?
No, feel free to bundle this up with any fixes needed and send it with a Signed-off-by from both of us
I did it pretty fast so it will need a careful read that there isn't a typo
Ok, will do so.
Regards, Lino
On Tue, Feb 09, 2021 at 09:36:53AM -0400, Jason Gunthorpe wrote:
On Tue, Feb 09, 2021 at 12:52:17PM +0100, Lino Sanfilippo wrote:
@@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
} EXPORT_SYMBOL_GPL(tpm_chip_unregister);
I tested the solution you scetched and it fixes the issue for me. Will you send a (real) patch for this?
No, feel free to bundle this up with any fixes needed and send it with a Signed-off-by from both of us
I did it pretty fast so it will need a careful read that there isn't a typo
Thanks, Jason
Let's use CDB too as it exist and Sean kindly provided a better documentation for it in some recent kernel release. It's great exactly for this type of situation.
/Jarkko
On Tue, Feb 09, 2021 at 12:52:17PM +0100, Lino Sanfilippo wrote:
Hi Jason,
On 05.02.21 18:25, Jason Gunthorpe wrote:
On Fri, Feb 05, 2021 at 08:48:11AM -0800, James Bottomley wrote:
Thanks for pointing this out. I'd strongly support Jason's proposal:
https://lore.kernel.org/linux-integrity/20201215175624.GG5487@ziepe.ca/
It's the best long-term way to fix this.
Really, no it's not. It introduces extra mechanism we don't need.
To recap the issue: character devices already have an automatic mechanism which holds a reference to the struct device while the character node is open so the default is to release resources on final put of the struct device.
The refcount on the struct device only keeps the memory alive, it doesn't say anything about the ops. We still need to lock and check the ops each and every time they are used.
The fact cdev goes all the way till fput means we don't need the extra get/put I suggested to Lino at all.
The practical consequence of this model is that if you allocate a chip structure with tpm_chip_alloc() you have to release it again by doing a put of *both* devices.
The final put of the devs should be directly after the cdev_device_del(), not in a devm. This became all confused because the devs was created during alloc, not register. Having a device that is initialized but will never be added is weird.
See sketch below.
Stefan noticed the latter, so we got the bogus patch 8979b02aaf1d ("tpm: Fix reference count to main device") applied which simply breaks the master/slave model by not taking a reference on the master for the slave. I'm not sure why I didn't notice the problem with this fix at the time, but attention must have been elsewhere.
Well, this is sort of OK because we never use the devs in TPM1, so we end up freeing the chip with a positive refcount on the devs, which is weird but not a functional bug.
Jason
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e10910..e07193a0dd4438 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -344,7 +344,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev_num = rc; device_initialize(&chip->dev);
- device_initialize(&chip->devs);
chip->dev.class = tpm_class; chip->dev.class->shutdown_pre = tpm_class_shutdown; @@ -352,29 +351,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->dev.parent = pdev; chip->dev.groups = chip->groups;
- chip->devs.parent = pdev;
- chip->devs.class = tpmrm_class;
- chip->devs.release = tpm_devs_release;
- /* 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)*/- if (chip->flags & TPM_CHIP_FLAG_TPM2)
get_device(&chip->dev);- if (chip->dev_num == 0) chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR); else chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
- chip->devs.devt =
MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);- rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
- if (rc)
goto out;- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num); if (rc) goto out;
@@ -382,9 +364,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, chip->flags |= TPM_CHIP_FLAG_VIRTUAL; cdev_init(&chip->cdev, &tpm_fops);
- cdev_init(&chip->cdevs, &tpmrm_fops); chip->cdev.owner = THIS_MODULE;
- chip->cdevs.owner = THIS_MODULE;
rc = tpm2_init_space(&chip->work_space, TPM2_SPACE_BUFFER_SIZE); if (rc) { @@ -396,7 +376,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, return chip; out:
- put_device(&chip->devs); put_device(&chip->dev); return ERR_PTR(rc);
} @@ -445,13 +424,33 @@ static int tpm_add_char_device(struct tpm_chip *chip) } if (chip->flags & TPM_CHIP_FLAG_TPM2) {
device_initialize(&chip->devs);chip->devs.parent = pdev;chip->devs.class = tpmrm_class;rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num);if (rc)goto out_put_devs;/** 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.*/get_device(&chip->dev);chip->devs.release = tpm_devs_release;chip->devs.devt =MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);cdev_init(&chip->cdevs, &tpmrm_fops);chip->cdevs.owner = THIS_MODULE;- rc = cdev_device_add(&chip->cdevs, &chip->devs); if (rc) { dev_err(&chip->devs, "unable to cdev_device_add() %s, major %d, minor %d, err=%d\n", dev_name(&chip->devs), MAJOR(chip->devs.devt), MINOR(chip->devs.devt), rc);
return rc;
goto out_put_devs;@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock); +out_put_devs:
- put_device(&chip->devs);
+out_del_dev:
- cdev_device_del(&chip->cdev); return rc;
} @@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (IS_ENABLED(CONFIG_HW_RANDOM_TPM)) hwrng_unregister(&chip->hwrng); tpm_bios_log_teardown(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- if (chip->flags & TPM_CHIP_FLAG_TPM2) { cdev_device_del(&chip->cdevs, &chip->devs);
put_device(&chip->devs);- } tpm_del_char_device(chip);
} EXPORT_SYMBOL_GPL(tpm_chip_unregister);
I tested the solution you scetched and it fixes the issue for me. Will you send a (real) patch for this?
One *option*:
1. You take the Jason's patch. 2. https://www.kernel.org/doc/html/v5.10/process/submitting-patches.html#when-t...
Just mentioning this, and spreading the knowledge about co-developed-by.
Best regards, Lino
/Jarkko
Hi,
On 12.02.21 at 11:59, Jarkko Sakkinen wrote:
One *option*:
- You take the Jason's patch.
- https://www.kernel.org/doc/html/v5.10/process/submitting-patches.html#when-t...
Just mentioning this, and spreading the knowledge about co-developed-by.
This seems to me like a very good fit, thanks for pointing at this. I will prepare a new patch series and use that tag.
Best regards, Lino
Hi James,
On 05.02.21 01:34, James Bottomley wrote:
Actually, this still isn't right. As I said to the last person who reported this, we should be doing a get/put on the ops, not rolling our own here:
https://lore.kernel.org/linux-integrity/e7566e1e48f5be9dca034b4bfb67683b5d3c...
Thanks for pointing this out, I was not aware of this discussion.
The reporter went silent before we could get this tested, but could you try, please, because your patch is still hand rolling the ops get/put, just slightly better than it had been done previously.
I tested your patch and it fixes the issue. Your solution seems indeed much cleaner.
FWIW:
Tested-by: Lino Sanfilippo l.sanfilippo@kunbus.com
Best Regards, Lino
Hi James,
On 05.02.21 01:34, James Bottomley wrote:
The reporter went silent before we could get this tested, but could you try, please, because your patch is still hand rolling the ops get/put, just slightly better than it had been done previously.
I tested your patch and it fixes the issue. Your solution seems indeed much cleaner.
FWIW:
Tested-by: Lino Sanfilippo l.sanfilippo@kunbus.com
Are you going to send a patch for this? As stated above I verified that your solution fixes the issue.
Best regards, Lino
On Fri, Feb 05, 2021 at 12:50:43AM +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
In tpm2_del_space() chip->ops is used for flushing the sessions. However this function may be called after tpm_chip_unregister() which sets the chip->ops pointer to NULL. Avoid a possible NULL pointer dereference by checking if chip->ops is still valid before accessing it.
Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com
drivers/char/tpm/tpm2-space.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
linux-stable-mirror@lists.linaro.org
-
Greg KH -
James Bottomley -
Jarkko Sakkinen -
Jason Gunthorpe -
Lino Sanfilippo -
Lino Sanfilippo -
Stefan Berger