Re: [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip

[Lino Sanfilippo]

Hi,


On 03.02.21 02:09, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 11:09:01PM +0100, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
>>
>> The following sequence of operations
>>
>> 1. open device /dev/tpmrm
>> 2. remove the registered tpm chip driver
> 
> What is "tpm chip driver"? Please just refer to the exact thing
> (e.g. tpm_tis_spi is the one you should refer to in your case).
> 

I did not explicitly refer to tpm_tis_spi because the refcount issue is in the TPM
chip driver core code and thus any TPM chip driver that creates the tpmrm device is
concerned. 

But if it helps to make the problem clearer I will only mention the tpm_tis_spi 
case in the next patch version.

>>
>> ------------[ 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 ]---
> 
> I guess this is happening with tpm_tis_spi. Unfortunately I don't have
> anything available that would use it.
> 
> I did testing with tpm_tis but so far no success reproducing.

With tpm_tis_spi this can be reproduced constantly. I haven't tried it with tpm_tis but 
I would expect it here to happen, too. However to trigger this bug you need something 
(in my case its an iotedge daemon) that opens /dev/tpmrm and keeps it open and writes 
occasionally commands to the TPM device even after you have unloaded the tpm_tis_spi module.

In your case you could try:

1. open /dev/tpmrm with a small test program and sleep for a few seconds
2. do 'rmmod tpm_tis' from another console while the test program sleeps
3. write to the opened /dev/tpmrm in your test program after the sleep. 
The data to write should be chosen in a way that it passes the sanity checks in 
tpm_common_write (alternatively just comment these checks out and write 
some random data). As soon as tpm_try_get_ops() in tpm_common_write() is called
the bug should trigger.

> 
> Hope this feedback helps to improve. You really need to rewrite the whole
> commit message. I wonder who could try to reproduce this. With a quick
> skim I get the issue.

Thanks for the thorough review. I will try to address all of the points you
mentioned in the next patch version. 

> Also you don't have James Bottomley in the CC list of the patch who is
> the author of the original commit. Please sort that out too...

Ok, will do so.

> /Jarkko
> 

Regards,
Lino