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 moving the TPM 2 character device handling from tpm_chip_alloc() to tpm_add_char_device() which is called at a later point in time when the flag has been set in case of TPM2.
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 by putting chip->devs in tpm_chip_unregister().
Finally move the new implemenation for the TPM 2 handling into a new function to avoid multiple checks for the TPM_CHIP_FLAG_TPM2 flag in the good case and error cases.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Co-developed-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com Cc: stable@vger.kernel.org --- drivers/char/tpm/tpm-chip.c | 80 ++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb..44cac3a 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,39 +351,20 @@ 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;
if (!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); } @@ -431,6 +410,46 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev, } EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip) +{ + int rc; + + device_initialize(&chip->devs); + chip->devs.parent = chip->dev.parent; + 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); + goto out_put_devs; + } + + return 0; + +out_put_devs: + put_device(&chip->devs); + + return rc; +} + static int tpm_add_char_device(struct tpm_chip *chip) { int rc; @@ -445,14 +464,9 @@ static int tpm_add_char_device(struct tpm_chip *chip) }
if (chip->flags & TPM_CHIP_FLAG_TPM2) { - 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; - } + rc = tpm_add_tpm2_char_device(chip); + if (rc) + goto del_cdev; }
/* Make the chip available. */ @@ -460,6 +474,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock);
+ return 0; + +del_cdev: + cdev_device_del(&chip->cdev, &chip->dev); return rc; }
@@ -640,8 +658,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);
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
Add '.' to end.
- Remove module tpm_tis_spi
Add '.' to end.
- 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 moving the TPM 2 character device handling from tpm_chip_alloc() to tpm_add_char_device() which is called at a later point in time when the flag has been set in case of TPM2.
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 by putting chip->devs in tpm_chip_unregister().
Finally move the new implemenation for the TPM 2 handling into a new function to avoid multiple checks for the TPM_CHIP_FLAG_TPM2 flag in the good case and error cases.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Co-developed-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com Cc: stable@vger.kernel.org
Put Cc first.
drivers/char/tpm/tpm-chip.c | 80 ++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb..44cac3a 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,39 +351,20 @@ 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;if (!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);
} @@ -431,6 +410,46 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev, } EXPORT_SYMBOL_GPL(tpmm_chip_alloc); +static int tpm_add_tpm2_char_device(struct tpm_chip *chip) +{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
- return rc;
+}
static int tpm_add_char_device(struct tpm_chip *chip) { int rc; @@ -445,14 +464,9 @@ static int tpm_add_char_device(struct tpm_chip *chip) } if (chip->flags & TPM_CHIP_FLAG_TPM2) {
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;}
rc = tpm_add_tpm2_char_device(chip);if (rc)goto del_cdev;/* Make the chip available. */ @@ -460,6 +474,10 @@ static int tpm_add_char_device(struct tpm_chip *chip) idr_replace(&dev_nums_idr, chip, chip->dev_num); mutex_unlock(&idr_lock);
- return 0;
+del_cdev:
- cdev_device_del(&chip->cdev, &chip->dev); return rc;
} @@ -640,8 +658,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); -- 2.7.4
Other than that, this looks good to me.
If this passes testing, I can fix those nit's.
I'll test this post 5.12 PR.
/Jarkko
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip) +{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
I'd rather you organize this so chip->devs.release and the get_device is always sent instead of having the possiblity for a put_device that doesn't call release
Jason
On Tue, Feb 16, 2021 at 08:53:42AM -0400, Jason Gunthorpe wrote:
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip)
BTW, this naming is crap.
- 2x tpm - char is useless
-> tpm2_add_device
+{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- chip->devs.class = tpmrm_class;
- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num);
- if (rc)
goto out_put_devs;
Right, and empty line missing here.
- /*
* 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);
Isn't this less than 100 chars?
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
I'd rather you organize this so chip->devs.release and the get_device is always sent instead of having the possiblity for a put_device that doesn't call release
/Jarkko
On Tue, Feb 16, 2021 at 06:04:42PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 08:53:42AM -0400, Jason Gunthorpe wrote:
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip)
BTW, this naming is crap.
- 2x tpm
- char is useless
-> tpm2_add_device
Actually, tpm2s_add_device() add put it to tpm2-space.c.
+{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- chip->devs.class = tpmrm_class;
- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num);
- if (rc)
goto out_put_devs;Right, and empty line missing here.
- /*
* 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);Isn't this less than 100 chars?
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
I'd rather you organize this so chip->devs.release and the get_device is always sent instead of having the possiblity for a put_device that doesn't call release
/Jarkko
On Tue, Feb 16, 2021 at 06:09:50PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 06:04:42PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 08:53:42AM -0400, Jason Gunthorpe wrote:
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip)
BTW, this naming is crap.
- 2x tpm
- char is useless
-> tpm2_add_device
Actually, tpm2s_add_device() add put it to tpm2-space.c.
No, tpms_add_device() :-)
(sorry)
/Jarkko
+{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- chip->devs.class = tpmrm_class;
- rc = dev_set_name(&chip->devs, "tpmrm%d", chip->dev_num);
- if (rc)
goto out_put_devs;Right, and empty line missing here.
- /*
* 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);Isn't this less than 100 chars?
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
I'd rather you organize this so chip->devs.release and the get_device is always sent instead of having the possiblity for a put_device that doesn't call release
/Jarkko
Hi,
On 16.02.21 at 17:11, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 06:09:50PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 06:04:42PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 16, 2021 at 08:53:42AM -0400, Jason Gunthorpe wrote:
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip)
BTW, this naming is crap.
- 2x tpm
- char is useless
-> tpm2_add_device
Actually, tpm2s_add_device() add put it to tpm2-space.c.
No, tpms_add_device() :-)
(sorry)
/Jarkko
I strongly assume you mean tmp2_add_device() :) I will move and rename the function accordingly.
Thanks, Lino
...
- get_device(&chip->dev);
- chip->devs.release = tpm_devs_release;
- chip->devs.devt =
MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);Isn't this less than 100 chars?
Still best kept under 80 if 'reasonable'?
Really it is just split in the wrong place: chip->devs.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Feb 16, 2021 at 04:31:26PM +0000, David Laight wrote:
...
- get_device(&chip->dev);
- chip->devs.release = tpm_devs_release;
- chip->devs.devt =
MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);Isn't this less than 100 chars?
Still best kept under 80 if 'reasonable'?
Really it is just split in the wrong place: chip->devs.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
Well it looks crap IMHO. Would be more reasonable to have it in a single like. And it is legit too, since it is accepted by checkpatch.
You might break the lines within 80 chars if it is somehow "logically" consistent.
David
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
/Jarkko
Hi
On 16.02.21 at 17:04, Jarkko Sakkinen wrote:
- /*
* 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);Isn't this less than 100 chars?
I just chose the same formatting that the original code used. Personally I prefer what David suggested, so if there is no objection against it I will format it this way.
Regards, Lino
Hi,
On 16.02.21 at 13:53, Jason Gunthorpe wrote:
On Tue, Feb 16, 2021 at 01:31:00AM +0100, Lino Sanfilippo wrote:
+static int tpm_add_tpm2_char_device(struct tpm_chip *chip) +{
- int rc;
- device_initialize(&chip->devs);
- chip->devs.parent = chip->dev.parent;
- 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);goto out_put_devs;- }
- return 0;
+out_put_devs:
- put_device(&chip->devs);
I'd rather you organize this so chip->devs.release and the get_device is always sent instead of having the possiblity for a put_device that doesn't call release
Agreed, I will change it. It should not make a difference in terms of correctness but I see that it is less confusing if both error cases are handled similarly (plus its only a minimal change).
Best regards, Lino
On 2/15/21 7:31 PM, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- 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 moving the TPM 2 character device handling from tpm_chip_alloc() to tpm_add_char_device() which is called at a later point in time when the flag has been set in case of TPM2.
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 by putting chip->devs in tpm_chip_unregister().
Finally move the new implemenation for the TPM 2 handling into a new function to avoid multiple checks for the TPM_CHIP_FLAG_TPM2 flag in the good case and error cases.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Co-developed-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com Cc: stable@vger.kernel.org
I know you'll post another version, but anyway:
Tested-by: Stefan Berger stefanb@linux.ibm.com
Hi Stefan,
On 16.02.21 at 17:52, Stefan Berger wrote:
On 2/15/21 7:31 PM, Lino Sanfilippo wrote:
From: Lino Sanfilippo l.sanfilippo@kunbus.com
The following sequence of operations results in a refcount warning:
- Open device /dev/tpmrm
- Remove module tpm_tis_spi
- 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 moving the TPM 2 character device handling from tpm_chip_alloc() to tpm_add_char_device() which is called at a later point in time when the flag has been set in case of TPM2.
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 by putting chip->devs in tpm_chip_unregister().
Finally move the new implemenation for the TPM 2 handling into a new function to avoid multiple checks for the TPM_CHIP_FLAG_TPM2 flag in the good case and error cases.
Fixes: fdc915f7f719 ("tpm: expose spaces via a device link /dev/tpmrm<n>") Fixes: 8979b02aaf1d ("tpm: Fix reference count to main device") Co-developed-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Jason Gunthorpe jgg@ziepe.ca Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com Cc: stable@vger.kernel.org
I know you'll post another version, but anyway:
Tested-by: Stefan Berger stefanb@linux.ibm.com
Thank you for testing this, I will send a v5 shortly.
Regards, Lino
linux-stable-mirror@lists.linaro.org
-
David Laight -
Jarkko Sakkinen -
Jason Gunthorpe -
Lino Sanfilippo -
Stefan Berger