All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vincent Whitchurch <vincent.whitchurch@axis.com>
To: Peter Huewe <peterhuewe@gmx.de>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: <kernel@axis.com>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	<linux-integrity@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: [PATCH 1/2] tpm: Fix tpmrm reference counting
Date: Tue, 15 Jun 2021 11:14:08 +0200	[thread overview]
Message-ID: <20210615091410.17007-1-vincent.whitchurch@axis.com> (raw)

The code added by commit 8979b02aaf1d6de8 ("tpm: Fix reference count to
main device") tries to take an extra reference to the main device only
for TPM2 by looking at the flags, but the flags are actually not set
at the time when tpm_chip_alloc() is called, so no extra reference is
ever taken, leading to a use-after-free if the TPM modules are removed
when the tpmrm device is in use.

 ==================================================================
 BUG: KASAN: use-after-free in __mutex_lock+0xe0/0xbd0
 Read of size 8 at addr ffff888116c6acc0 by task sh/1210

 CPU: 0 PID: 1210 Comm: sh Not tainted 5.13.0-rc5+ #200
 Call Trace:
  __mutex_lock+0xe0/0xbd0
  tpm2_del_space+0x24/0xa0 [tpm]
  tpmrm_release+0x3f/0x50 [tpm]
  __fput+0x110/0x3c0
  task_work_run+0x94/0xd0
  do_exit+0x683/0x13e0
  do_syscall_64+0x3c/0x80

 Allocated by task 1153:
  kasan_save_stack+0x19/0x40
  __kasan_kmalloc+0x7f/0xa0
  tpm_chip_alloc+0x3b/0x360 [tpm]
  tpmm_chip_alloc+0x11/0x70 [tpm]
  tpm_tis_core_init+0xce/0x570 [tpm_tis_core]
  pnp_device_probe+0x9c/0x100
  ...

 Freed by task 1243:
  kfree+0x121/0x340
  device_release+0x59/0xf0
  kobject_put+0xa5/0x120
  release_nodes+0x37f/0x3f0
  driver_detach+0x7c/0xf0
  bus_remove_driver+0x86/0x110
  __x64_sys_delete_module+0x27b/0x320
  ...
 ==================================================================

The real fix to the problem which that commit tried to solve is to make
tpmm_chip_alloc() put the ->devs device in the devm release function,
since that is never done anywhere currently.  This is safe since
device_initialize() is always called on ->devs.  No conditional
reference taking is needed.

Fixes: 8979b02aaf1d6de8 ("tpm: Fix reference count to main device")
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/char/tpm/tpm-chip.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ddaeceb7e109..029ee61c38de 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -358,10 +358,9 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	/* 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)
+	 * is in the tpm_devs_release
 	 */
-	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);
@@ -402,6 +401,14 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 }
 EXPORT_SYMBOL_GPL(tpm_chip_alloc);
 
+static void tpmm_chip_release(void *data)
+{
+	struct tpm_chip *chip = data;
+
+	put_device(&chip->devs);
+	put_device(&chip->dev);
+}
+
 /**
  * tpmm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: parent device to which the chip is associated
@@ -419,9 +426,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 	if (IS_ERR(chip))
 		return chip;
 
-	rc = devm_add_action_or_reset(pdev,
-				      (void (*)(void *)) put_device,
-				      &chip->dev);
+	rc = devm_add_action_or_reset(pdev, tpmm_chip_release, chip);
 	if (rc)
 		return ERR_PTR(rc);
 
-- 
2.28.0


             reply	other threads:[~2021-06-15  9:14 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15  9:14 Vincent Whitchurch [this message]
2021-06-15  9:14 ` [PATCH 2/2] tpm: Fix crash on tmprm release Vincent Whitchurch
2021-06-15 13:18   ` Jarkko Sakkinen
2021-06-17  5:44     ` Vincent Whitchurch
2022-03-03  2:03   ` Stefan Berger
2021-06-16 18:53 ` [PATCH 1/2] tpm: Fix tpmrm reference counting Jason Gunthorpe
2021-06-17  5:38   ` Vincent Whitchurch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210615091410.17007-1-vincent.whitchurch@axis.com \
    --to=vincent.whitchurch@axis.com \
    --cc=jarkko@kernel.org \
    --cc=jgg@ziepe.ca \
    --cc=kernel@axis.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterhuewe@gmx.de \
    --cc=stefanb@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.