On 2/4/21 6:50 PM, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> 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 <[email protected]>
Tested-by: Stefan Berger <[email protected]>
Steps:
modprobe tpm_vtpm_proxy
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm1
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
exec 100>&- # fails before, works after --> great job! :-)
On 2/4/21 7:46 PM, Stefan Berger wrote:
> On 2/4/21 6:50 PM, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <[email protected]>
>>
>> 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 <[email protected]>
>
> Tested-by: Stefan Berger <[email protected]>
>
> Steps:
>
> modprobe tpm_vtpm_proxy
>
> swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
>
> exec 100<>/dev/tpmrm1
>
> kill -9 <swtpm pid>
>
> rmmod tpm_vtpm_proxy
>
> exec 100>&- # fails before, works after --> great job! :-)
>
>
To clarify: When I tested this I had *both* patches applied. Without the
patches I got the null pointer exception in tpm2_del_space(). The 2nd
patch alone solves that issue when using the steps above.
[ 525.647443] [c000000005d3bba0] [c000000000e81d78]
mutex_lock+0x28/0x90 (unreliable)
[ 525.647539] [c000000005d3bbd0] [c0080000001f5da0]
tpm2_del_space+0x48/0x130 [tpm]
[ 525.647635] [c000000005d3bc20] [c0080000001f56b8]
tpmrm_release+0x40/0x70 [tpm]
[ 525.647746] [c000000005d3bc50] [c0000000004bf718] __fput+0xb8/0x340
[ 525.647842] [c000000005d3bca0] [c00000000017def4]
task_work_run+0xe4/0x150
[ 525.647930] [c000000005d3bcf0] [c00000000001feb4]
do_notify_resume+0x484/0x4f0
[ 525.648023] [c000000005d3bdb0] [c000000000033a64]
syscall_exit_prepare+0x1d4/0x330
[ 525.648115] [c000000005d3be20] [c00000000000d96c]
system_call_common+0xfc/0x27c
On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
> To clarify: When I tested this I had *both* patches applied. Without
> the patches I got the null pointer exception in tpm2_del_space(). The
> 2nd patch alone solves that issue when using the steps above.
Yes, I can't confirm the bug either. I only have lpc tis devices, so
it could be something to do with spi, but when I do
python3 in one shell
>>> fd = open("/dev/tpmrm0", "wb")
do rmmod tpm_tis in another shell
>>> buf = bytearray(20)
>>> fd.write(buf)
20
so I don't see the oops you see on write. However
>>> fd.close()
And it oopses here in tpm2_del_space
James
Hi 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 <[email protected]>
>
> Tested-by: Stefan Berger <[email protected]>
>
> 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
Hi,
On 05.02.21 03:01, James Bottomley wrote:
> On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
>> To clarify: When I tested this I had *both* patches applied. Without
>> the patches I got the null pointer exception in tpm2_del_space(). The
>> 2nd patch alone solves that issue when using the steps above.
>
>
> Yes, I can't confirm the bug either. I only have lpc tis devices, so
> it could be something to do with spi, but when I do
>
> python3 in one shell
>
>>>> fd = open("/dev/tpmrm0", "wb")
>
> do rmmod tpm_tis in another shell
>
>>>> buf = bytearray(20)
>>>> fd.write(buf)
> 20
The issue is in the TPM chip driver code, so AFAIU it should not matter whether its
SPI or something else. Maybe check again, that the file is still open when
tpm_tis is removed and the write actually comes after the rmmod?
Also note that there are some sanity checks in tpm_common_write() that the written
data has to pass to get to the point where tpm_try_get_ops() is called, which
is the call that eventually triggers the bug.
>
> so I don't see the oops you see on write. However
>
>>>> fd.close()
>
> And it oopses here in tpm2_del_space
>
> James
>
>
Regards,
Lino
On 2/4/21 9:01 PM, James Bottomley wrote:
> On Thu, 2021-02-04 at 20:44 -0500, Stefan Berger wrote:
>> To clarify: When I tested this I had *both* patches applied. Without
>> the patches I got the null pointer exception in tpm2_del_space(). The
>> 2nd patch alone solves that issue when using the steps above.
>
> Yes, I can't confirm the bug either. I only have lpc tis devices, so
> it could be something to do with spi, but when I do
I can confirm this bug:
insmod /usr/lib/modules/5.10.0+/extra/tpm.ko ; insmod
/usr/lib/modules/5.10.0+/extra/tpm_vtpm_proxy.ko
swtpm chardev --vtpm-proxy --tpm2 --tpmstate dir=./ &
exec 100<>/dev/tpmrm0
kill -9 <swtpm pid>
rmmod tpm_vtpm_proxy
echo -en '\x80\x01\x00\x00\x00\x0c\x00\x00\x01\x44\x00\x00' >&100
[ 167.289390] [c000000015d6fb60] [c0000000007d3ac0]
refcount_warn_saturate+0x210/0x230 (unreliable)
[ 167.290392] [c000000015d6fbc0] [c000000000831328] kobject_put+0x1b8/0x2e0
[ 167.291398] [c000000015d6fc50] [c000000000955548] put_device+0x28/0x40
[ 167.292409] [c000000015d6fc70] [c0080000008609a8]
tpm_try_get_ops+0xb0/0x100 [tpm]
[ 167.293417] [c000000015d6fcb0] [c008000000861864]
tpm_common_write+0x15c/0x250 [tpm]
[ 167.294429] [c000000015d6fd20] [c0000000004be190] vfs_write+0xf0/0x380
[ 167.295437] [c000000015d6fd70] [c0000000004be6c8] ksys_write+0x78/0x130
[ 167.296450] [c000000015d6fdc0] [c00000000003377c]
system_call_exception+0x15c/0x270
[ 167.297461] [c000000015d6fe20] [c00000000000d960]
system_call_common+0xf0/0x27c
With this patch applied this error here is gone. Just have make sure to
replace tpm.ko and tpm_vtpm_proxy.ko, not just the latter.
So my Tested-By is good for both patches.
Stefan