* [PATCH v2 0/3] TPM fixes
@ 2021-02-02 22:09 Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip Lino Sanfilippo
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-02 22:09 UTC (permalink / raw)
To: peterhuewe, jarkko
Cc: jgg, stefanb, linux-integrity, linux-kernel, LinoSanfilippo
This series fixes a reference count issue and a possible NULL pointer
access in the TPM core code.
It also introduces a new function tpm_chip_free() which is used as the
counterpart to tpm_chip_alloc(). The function is supposed to hide the
internals of a proper tpm_chip deallocation.
Changes in v2:
- drop the patch that erroneously cleaned up after failed installation of
an action handler ni 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 (3):
tpm: fix reference counting for struct tpm_chip
tpm: Provide a function tpm_chip_free() to free tpm chips
tpm: in tpm2_del_space check if ops pointer is still valid
drivers/char/tpm/tpm-chip.c | 34 +++++++++++++++++++++++++++++++---
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm2-space.c | 15 ++++++++++-----
drivers/char/tpm/tpm_ftpm_tee.c | 4 ++--
drivers/char/tpm/tpm_vtpm_proxy.c | 2 +-
5 files changed, 45 insertions(+), 11 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip
2021-02-02 22:09 [PATCH v2 0/3] TPM fixes Lino Sanfilippo
@ 2021-02-02 22:09 ` Lino Sanfilippo
2021-02-03 1:09 ` Jarkko Sakkinen
2021-02-02 22:09 ` [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid Lino Sanfilippo
2 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-02 22:09 UTC (permalink / raw)
To: peterhuewe, jarkko
Cc: jgg, stefanb, linux-integrity, linux-kernel, LinoSanfilippo,
Lino Sanfilippo
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
The following sequence of operations
1. open device /dev/tpmrm
2. remove the registered tpm chip driver
3. perform a write() to /dev/tpmrm
results in a refcount warning:
------------[ 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 is an attempt to get the chip->dev reference (see
tpm_try_get_ops()) although the last reference to this device has already
been released with the unregistration of the chip driver.
As far as I understand, the history of this bug is as follows:
Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>")
introduced the support for /dev/tpmrm. Beside the new device chip->devs
this code introduced the taking of an extra reference to chip->dev to
ensure that this device is not released as long as /dev/tpmrm is still in
use:
+ 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
+ */
The extra reference to chip->dev was supposed to be released as soon as the
reference to chip->devs was freed:
+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);
+}
However the code did not work as expected, because the reference to
chip->devs was never released. Thus tpm_devs_release() was never called
which in turn also prevented the extra reference to chip->dev from being
released.
This led to behaviour which commit 8979b02aaf1d ("tpm: Fix reference count
to main device") tried to fix as an excerpt of the commit message shows:
"The main device is currently not properly released due to one additional
reference to the 'devs' device which is only released in case of a TPM 2.
So, also get the additional reference only in case of a TPM2."
Instead of adding the missing put for chip->devs, this patch chose another
approach by only taking the reference in case of TPM2:
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ get_device(&chip->dev);
This fixed the non-TPM2 case but left the TPM2 case without the required
extra reference, since there is no code path that ever sets the flag
TPM_CHIP_FLAG_TPM2, which is the situation we have today.
To fix this
1. revert commit 8979b02aaf1d ("tpm: Fix reference count to main device")
to make sure the extra reference is taken unconditionally as proposed by
commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>")
2. fix commit fdc915f7f719 ("tpm: expose spaces via a device link
/dev/tpmrm<n>") by adding an action handler to do the missing put to
chip->devs. This eventually results in the call of function
tpm_devs_release() which in turn performs the final put to the extra
reference to chip->dev.
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips
2021-02-02 22:09 [PATCH v2 0/3] TPM fixes Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip Lino Sanfilippo
@ 2021-02-02 22:09 ` Lino Sanfilippo
2021-02-03 1:28 ` Jarkko Sakkinen
2021-02-02 22:09 ` [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid Lino Sanfilippo
2 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-02 22:09 UTC (permalink / raw)
To: peterhuewe, jarkko
Cc: jgg, stefanb, linux-integrity, linux-kernel, LinoSanfilippo,
Lino Sanfilippo
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Provide a function tpm_chip_free() as a counterpart to tpm_chip_alloc().
The function hides the internals of freeing a struct tpm_chip instance
by putting the device references which are part of this structure.
Use the new function at the appropriate places.
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
drivers/char/tpm/tpm-chip.c | 16 ++++++++++++++++
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm_ftpm_tee.c | 6 ++----
drivers/char/tpm/tpm_vtpm_proxy.c | 3 +--
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 3ace199..777baae 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -402,6 +402,22 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
EXPORT_SYMBOL_GPL(tpm_chip_alloc);
/**
+ * tpm_chip_free() - free an instance of struct tpm_chip that has been
+ * allocated with tpm_chip_alloc() before.
+ * @chip: chip to free
+ *
+ * Frees an instance of struct tpm_chip by releasing internal device references.
+ * This function is used to hide the internals needed to free a struct tpm_chip
+ * instance thas has been allocated with tpm_chip_alloc() before.
+ */
+void tpm_chip_free(struct tpm_chip *chip)
+{
+ put_device(&chip->devs);
+ put_device(&chip->dev);
+}
+EXPORT_SYMBOL_GPL(tpm_chip_free);
+
+/**
* tpmm_chip_alloc() - allocate a new struct tpm_chip instance
* @pdev: parent device to which the chip is associated
* @ops: struct tpm_class_ops instance
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 947d1db..e6bb6ae 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -199,6 +199,7 @@ void tpm_put_ops(struct tpm_chip *chip);
struct tpm_chip *tpm_chip_alloc(struct device *dev,
const struct tpm_class_ops *ops);
+void tpm_chip_free(struct tpm_chip *chip);
struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
const struct tpm_class_ops *ops);
int tpm_chip_register(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
index 82858c2..47ffaae 100644
--- a/drivers/char/tpm/tpm_ftpm_tee.c
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -285,8 +285,7 @@ static int ftpm_tee_probe(struct device *dev)
return 0;
out_chip:
- put_device(&pvt_data->chip->dev);
- put_device(&pvt_data->chip->devs);
+ tpm_chip_free(chip);
out_chip_alloc:
tee_shm_free(pvt_data->shm);
out_shm_alloc:
@@ -319,8 +318,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);
+ tpm_chip_free(pvt_data->chip);
/* Free the shared memory pool */
tee_shm_free(pvt_data->shm);
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index 97b60f8..f887bb3 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -520,8 +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 */
+ tpm_chip_free(proxy_dev->chip);
kfree(proxy_dev);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid
2021-02-02 22:09 [PATCH v2 0/3] TPM fixes Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips Lino Sanfilippo
@ 2021-02-02 22:09 ` Lino Sanfilippo
2021-02-03 1:17 ` Jarkko Sakkinen
2 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-02 22:09 UTC (permalink / raw)
To: peterhuewe, jarkko
Cc: jgg, stefanb, linux-integrity, linux-kernel, LinoSanfilippo,
Lino Sanfilippo
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
In tpm2_del_space() the sessions are flushed by means of the tpm_chip
operations. However the concerning operations pointer my already be NULL at
this time in case that the chip has been unregistered (see
tpm_chip_unregister() which calls tpm_del_char_device() which sets
chip->ops to NULL).
Before accessing chip->ops check if it is still valid. Skip flushing
the sessions in this case.
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip
2021-02-02 22:09 ` [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip Lino Sanfilippo
@ 2021-02-03 1:09 ` Jarkko Sakkinen
2021-02-03 14:06 ` Lino Sanfilippo
0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2021-02-03 1:09 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel,
Lino Sanfilippo, James.Bottomley
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).
> 3. perform a write() to /dev/tpmrm
Just a nit: Use capital letter as the first letter in sentences.
If I do "rmmod tpm_tis" the device is gone.
> results in a refcount warning:
>
> ------------[ 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.
>
> The reason is an attempt to get the chip->dev reference (see
> tpm_try_get_ops()) although the last reference to this device has already
> been released with the unregistration of the chip driver.
>
> As far as I understand, the history of this bug is as follows:
Generally you don't write in person to a commit message. This not an
email in that sense. You should write in imperative form.
> Commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>")
> introduced the support for /dev/tpmrm. Beside the new device chip->devs
> this code introduced the taking of an extra reference to chip->dev to
> ensure that this device is not released as long as /dev/tpmrm is still in
> use:
>
> + 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
> + */
Copy pasting this is probably unnecessary.
> The extra reference to chip->dev was supposed to be released as soon as the
> reference to chip->devs was freed:
>
> +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);
> +}
OK, what you really should explain in the commit message is:
1. Why this function *is not* called.
2. What your fix does to make things better.
> However the code did not work as expected, because the reference to
> chip->devs was never released. Thus tpm_devs_release() was never called
> which in turn also prevented the extra reference to chip->dev from being
> released.
If it did not work, it works now :-) So any fix would not be required.
Just making a point here.
> This led to behaviour which commit 8979b02aaf1d ("tpm: Fix reference count
> to main device") tried to fix as an excerpt of the commit message shows:
>
> "The main device is currently not properly released due to one additional
> reference to the 'devs' device which is only released in case of a TPM 2.
> So, also get the additional reference only in case of a TPM2."
>
> Instead of adding the missing put for chip->devs, this patch chose another
> approach by only taking the reference in case of TPM2:
>
> + if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + get_device(&chip->dev);
>
> This fixed the non-TPM2 case but left the TPM2 case without the required
> extra reference, since there is no code path that ever sets the flag
> TPM_CHIP_FLAG_TPM2, which is the situation we have todoay.
You should not need this explanation to explain refcount mismatch. Also
past tense should not be used.
I appreciate the time taken to write all this but you should just explain
what you do and why. Let's just say this explanation is very obfuscted and
hard to follow.
> To fix this
>
> 1. revert commit 8979b02aaf1d ("tpm: Fix reference count to main device")
> to make sure the extra reference is taken unconditionally as proposed by
> commit fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>")
> 2. fix commit fdc915f7f719 ("tpm: expose spaces via a device link
>
> /dev/tpmrm<n>") by adding an action handler to do the missing put to
> chip->devs. This eventually results in the call of function
> tpm_devs_release() which in turn performs the final put to the extra
> reference to chip->dev.
>
> Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>")
> Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device")
If the fix makes sense a Cc stable is also required.
> 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
>
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.
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...
/Jarkko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid
2021-02-02 22:09 ` [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid Lino Sanfilippo
@ 2021-02-03 1:17 ` Jarkko Sakkinen
2021-02-03 14:22 ` Lino Sanfilippo
0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2021-02-03 1:17 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel, Lino Sanfilippo
On Tue, Feb 02, 2021 at 11:09:03PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> In tpm2_del_space() the sessions are flushed by means of the tpm_chip
> operations. However the concerning operations pointer my already be NULL at
~~
may
What is "concerniog operations"? Unfamiliar term. Maybe just consistently
se chip->ops? Now you have also "tpm_chip operations" and chip->ops, in
addition to "concerning operations" in one paragraph commit message.
> this time in case that the chip has been unregistered (see
> tpm_chip_unregister() which calls tpm_del_char_device() which sets
> chip->ops to NULL).
> Before accessing chip->ops check if it is still valid. Skip flushing
> the sessions in this case.
>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
Instead of cross referencing please describe the scenario (i.e.
the sequence of operations) of failure.
Fixes tag is also missing.
> ---
> 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
>
/Jarkko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips
2021-02-02 22:09 ` [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips Lino Sanfilippo
@ 2021-02-03 1:28 ` Jarkko Sakkinen
2021-02-03 14:09 ` Lino Sanfilippo
0 siblings, 1 reply; 11+ messages in thread
From: Jarkko Sakkinen @ 2021-02-03 1:28 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel, Lino Sanfilippo
On Tue, Feb 02, 2021 at 11:09:02PM +0100, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> Provide a function tpm_chip_free() as a counterpart to tpm_chip_alloc().
> The function hides the internals of freeing a struct tpm_chip instance
> by putting the device references which are part of this structure.
>
> Use the new function at the appropriate places.
>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
I prefer open coding here.
/Jarkko
> ---
> drivers/char/tpm/tpm-chip.c | 16 ++++++++++++++++
> drivers/char/tpm/tpm.h | 1 +
> drivers/char/tpm/tpm_ftpm_tee.c | 6 ++----
> drivers/char/tpm/tpm_vtpm_proxy.c | 3 +--
> 4 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 3ace199..777baae 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -402,6 +402,22 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> EXPORT_SYMBOL_GPL(tpm_chip_alloc);
>
> /**
> + * tpm_chip_free() - free an instance of struct tpm_chip that has been
> + * allocated with tpm_chip_alloc() before.
> + * @chip: chip to free
> + *
> + * Frees an instance of struct tpm_chip by releasing internal device references.
> + * This function is used to hide the internals needed to free a struct tpm_chip
> + * instance thas has been allocated with tpm_chip_alloc() before.
> + */
> +void tpm_chip_free(struct tpm_chip *chip)
> +{
> + put_device(&chip->devs);
> + put_device(&chip->dev);
> +}
> +EXPORT_SYMBOL_GPL(tpm_chip_free);
> +
> +/**
> * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
> * @pdev: parent device to which the chip is associated
> * @ops: struct tpm_class_ops instance
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 947d1db..e6bb6ae 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -199,6 +199,7 @@ void tpm_put_ops(struct tpm_chip *chip);
>
> struct tpm_chip *tpm_chip_alloc(struct device *dev,
> const struct tpm_class_ops *ops);
> +void tpm_chip_free(struct tpm_chip *chip);
> struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
> const struct tpm_class_ops *ops);
> int tpm_chip_register(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 82858c2..47ffaae 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -285,8 +285,7 @@ static int ftpm_tee_probe(struct device *dev)
> return 0;
>
> out_chip:
> - put_device(&pvt_data->chip->dev);
> - put_device(&pvt_data->chip->devs);
> + tpm_chip_free(chip);
> out_chip_alloc:
> tee_shm_free(pvt_data->shm);
> out_shm_alloc:
> @@ -319,8 +318,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);
> + tpm_chip_free(pvt_data->chip);
>
> /* Free the shared memory pool */
> tee_shm_free(pvt_data->shm);
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 97b60f8..f887bb3 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -520,8 +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 */
> + tpm_chip_free(proxy_dev->chip);
> kfree(proxy_dev);
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip
2021-02-03 1:09 ` Jarkko Sakkinen
@ 2021-02-03 14:06 ` Lino Sanfilippo
2021-02-03 21:43 ` Jarkko Sakkinen
0 siblings, 1 reply; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-03 14:06 UTC (permalink / raw)
To: Jarkko Sakkinen, Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel, James.Bottomley
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.
>>
>> ------------[ 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips
2021-02-03 1:28 ` Jarkko Sakkinen
@ 2021-02-03 14:09 ` Lino Sanfilippo
0 siblings, 0 replies; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-03 14:09 UTC (permalink / raw)
To: Jarkko Sakkinen, Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel
Hi
On 03.02.21 02:28, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 11:09:02PM +0100, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Provide a function tpm_chip_free() as a counterpart to tpm_chip_alloc().
>> The function hides the internals of freeing a struct tpm_chip instance
>> by putting the device references which are part of this structure.
>>
>> Use the new function at the appropriate places.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> I prefer open coding here.
>
> /Jarkko
>
Ok, then I will drop this patch in the next series version.
Regards,
Lino
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid
2021-02-03 1:17 ` Jarkko Sakkinen
@ 2021-02-03 14:22 ` Lino Sanfilippo
0 siblings, 0 replies; 11+ messages in thread
From: Lino Sanfilippo @ 2021-02-03 14:22 UTC (permalink / raw)
To: Jarkko Sakkinen, Lino Sanfilippo
Cc: peterhuewe, jgg, stefanb, linux-integrity, linux-kernel
Hi,
On 03.02.21 02:17, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 11:09:03PM +0100, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> In tpm2_del_space() the sessions are flushed by means of the tpm_chip
>> operations. However the concerning operations pointer my already be NULL at
> ~~
> may
>
> What is "concerniog operations"? Unfamiliar term. Maybe just consistently
> se chip->ops? Now you have also "tpm_chip operations" and chip->ops, in
> addition to "concerning operations" in one paragraph commit message.
'concerning' referred to 'operations pointer'. But yes, using multiple times
a different term for the same thing is quite confusing. I will fix this.
>> this time in case that the chip has been unregistered (see
>> tpm_chip_unregister() which calls tpm_del_char_device() which sets
>> chip->ops to NULL).
>> Before accessing chip->ops check if it is still valid. Skip flushing
>> the sessions in this case.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> Instead of cross referencing please describe the scenario (i.e.
> the sequence of operations) of failure.
>
> Fixes tag is also missing.
>
Right, will add it.
Thanks,
Lino
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip
2021-02-03 14:06 ` Lino Sanfilippo
@ 2021-02-03 21:43 ` Jarkko Sakkinen
0 siblings, 0 replies; 11+ messages in thread
From: Jarkko Sakkinen @ 2021-02-03 21:43 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: Lino Sanfilippo, peterhuewe, jgg, stefanb, linux-integrity,
linux-kernel, James.Bottomley
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-03 21:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 22:09 [PATCH v2 0/3] TPM fixes Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 1/3] tpm: fix reference counting for struct tpm_chip Lino Sanfilippo
2021-02-03 1:09 ` Jarkko Sakkinen
2021-02-03 14:06 ` Lino Sanfilippo
2021-02-03 21:43 ` Jarkko Sakkinen
2021-02-02 22:09 ` [PATCH v2 2/3] tpm: Provide a function tpm_chip_free() to free tpm chips Lino Sanfilippo
2021-02-03 1:28 ` Jarkko Sakkinen
2021-02-03 14:09 ` Lino Sanfilippo
2021-02-02 22:09 ` [PATCH v2 3/3] tpm: in tpm2_del_space check if ops pointer is still valid Lino Sanfilippo
2021-02-03 1:17 ` Jarkko Sakkinen
2021-02-03 14:22 ` Lino Sanfilippo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).