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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Feb 03, 2021 at 03:06:30PM +0100, Lino Sanfilippo wrote:
> 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@kunbus.com>
> >>
> >> 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.

If you encounter a bug, you should generally just refer the exact thing
where you encounter it. This will help a lot, e.g. to reproduce it.

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

Right, I used tools/testing/selftests/tpm2/test_space.sh.

I think I agree you with the regression.

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

Thank you for your effort. It's of course highly appreciated!

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

/Jarkko
>