[PATCH v3 0/2] TPM fixes

[PATCH v3 0/2] TPM fixes

[Lino Sanfilippo]

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(-)

-- 
2.7.4

[PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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);
 }
-- 
2.7.4

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Stefan Berger]

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:
>
> 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>

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! :-)

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Stefan Berger]

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:
>>
>> 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>
>
> 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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[James Bottomley]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Stefan Berger]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Greg KH]

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:
> 
> 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(-)

<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>

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Jason Gunthorpe]

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:
> 
> 1. Open device /dev/tpmrm
> 2. Remove module tpm_tis_spi
> 3. 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);
>  		return ERR_PTR(rc);

This isn't right read what 'or_reset' does

Jason

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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);
>>  		return ERR_PTR(rc);
> 
> 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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Jason Gunthorpe]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Jason Gunthorpe]

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

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

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.
>

Re: [PATCH v3 1/2] tpm: fix reference counting for struct tpm_chip

[Jason Gunthorpe]

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

[PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

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);
 }
-- 
2.7.4

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[James Bottomley]

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/e7566e1e48f5be9dca034b4bfb67683b5d3cb88f.camel@HansenPartnership.com/

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jarkko Sakkinen]

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/e7566e1e48f5be9dca034b4bfb67683b5d3cb88f.camel@HansenPartnership.com/
> 
> 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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[James Bottomley]

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/e7566e1e48f5be9dca034b4bfb67683b5d3cb88f.camel@HansenPartnership.com/
> > 
> > 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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jason Gunthorpe]

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);

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[James Bottomley]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jason Gunthorpe]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[James Bottomley]

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 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;
> +

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jason Gunthorpe]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jason Gunthorpe]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jarkko Sakkinen]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Jarkko Sakkinen]

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-to-use-acked-by-cc-and-co-developed-by

Just mentioning this, and spreading the knowledge about co-developed-by.

> Best regards,
> Lino

/Jarkko

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

Hi,

On 12.02.21 at 11:59, Jarkko Sakkinen wrote:

>
> One *option*:
>
> 1. You take the Jason's patch.
> 2. https://www.kernel.org/doc/html/v5.10/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> 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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

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/e7566e1e48f5be9dca034b4bfb67683b5d3cb88f.camel@HansenPartnership.com/
> 

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Lino Sanfilippo]

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

Re: [PATCH v3 2/2] tpm: in tpm2_del_space check if ops pointer is still valid

[Greg KH]

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>