[PATCH] tpm: Fix null pointer dereference on chip register error path

[PATCH] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

If clk_enable is not defined and chip initialization
is canceled code hits null dereference.

Easily reproducible with vTPM init fail:
  swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy

BUG: kernel NULL pointer dereference, address: 00000000
...
Call Trace:
 tpm_chip_start+0x9d/0xa0 [tpm]
 tpm_chip_register+0x10/0x1a0 [tpm]
 vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
 process_one_work+0x214/0x5a0
 worker_thread+0x134/0x3e0
 ? process_one_work+0x5a0/0x5a0
 kthread+0xd4/0x100
 ? process_one_work+0x5a0/0x5a0
 ? kthread_park+0x90/0x90
 ret_from_fork+0x19/0x24

Signed-off-by: Milan Broz <gmazyland@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/char/tpm/tpm-chip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 90325e1749fb..4c2af643d698 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -95,7 +95,8 @@ int tpm_chip_start(struct tpm_chip *chip)
 	if (chip->locality == -1) {
 		ret = tpm_request_locality(chip);
 		if (ret) {
-			chip->ops->clk_enable(chip, false);
+			if (chip->ops->clk_enable)
+				chip->ops->clk_enable(chip, false);
 			return ret;
 		}
 	}
-- 
2.20.1

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[James Morris]

On Wed, 12 Jun 2019, Milan Broz wrote:

> If clk_enable is not defined and chip initialization
> is canceled code hits null dereference.
> 
> Easily reproducible with vTPM init fail:
>   swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy
> 
> BUG: kernel NULL pointer dereference, address: 00000000
> ...
> Call Trace:
>  tpm_chip_start+0x9d/0xa0 [tpm]
>  tpm_chip_register+0x10/0x1a0 [tpm]
>  vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
>  process_one_work+0x214/0x5a0
>  worker_thread+0x134/0x3e0
>  ? process_one_work+0x5a0/0x5a0
>  kthread+0xd4/0x100
>  ? process_one_work+0x5a0/0x5a0
>  ? kthread_park+0x90/0x90
>  ret_from_fork+0x19/0x24
> 
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> Cc: stable@vger.kernel.org


Reviewed-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.1.9, v4.19.50, v4.14.125, v4.9.181, v4.4.181.

v5.1.9: Build OK!
v4.19.50: Failed to apply! Possible dependencies:
    100b16a6f290 ("tpm: sort objects in the Makefile")
    29b47ce98759 ("tpm: move TPM space code out of tpm_transmit()")
    412eb585587a ("tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter")
    5faafbab77e3 ("tpm: remove @space from tpm_transmit()")
    70a3199a7101 ("tpm: factor out tpm_get_timeouts()")
    719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
    899102bc4518 ("tpm2: add new tpm2 commands according to TCG 1.36")
    9db7fe187c54 ("tpm: factor out tpm_startup function")
    9e1b74a63f77 ("tpm: add support for nonblocking operation")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    b34b77a99b1a ("tpm: declare struct tpm_header")
    c3465a370fb3 ("tpm: move tpm_validate_commmand() to tpm2-space.c")
    c3d477a725ef ("tpm: add ptr to the tpm_space struct to file_priv")
    c4df71d43a5b ("tpm: encapsulate tpm_dev_transmit()")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")

v4.14.125: Failed to apply! Possible dependencies:
    09dd144f72e7 ("tpm: Add explicit endianness cast")
    0bfb23746052 ("tpm: Move eventlog files to a subdirectory")
    100b16a6f290 ("tpm: sort objects in the Makefile")
    58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
    67cb8e113ecd ("tpm: rename event log provider files")
    719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
    9b01b5356629 ("tpm: Move shared eventlog functions to common.c")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
    fd3ec3663718 ("tpm: move tpm_eventlog.h outside of drivers folder")

v4.9.181: Failed to apply! Possible dependencies:
    02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
    100b16a6f290 ("tpm: sort objects in the Makefile")
    2528a64664f8 ("tpm: define a generic open() method for ascii & bios measurements")
    719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
    748935eeb72c ("tpm: have event log use the tpm_chip")
    7518a21a9da3 ("tpm: drop tpm1_chip_register(/unregister)")
    9b01b5356629 ("tpm: Move shared eventlog functions to common.c")
    b1a9b7b602c5 ("tpm: replace symbolic permission with octal for securityfs files")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    cd9b7631a888 ("tpm: replace dynamically allocated bios_dir with a static array")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")

v4.4.181: Failed to apply! Possible dependencies:
    02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
    036bb38ffb3e ("tpm_tis: Ensure interrupts are disabled when the driver starts")
    100b16a6f290 ("tpm: sort objects in the Makefile")
    23d06ff700f5 ("tpm: drop tpm_atmel specific fields from tpm_vendor_specific")
    25112048cd59 ("tpm: rework tpm_get_timeouts()")
    41a5e1cf1fe1 ("tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy")
    4d627e672bd0 ("tpm_tis: Do not fall back to a hardcoded address for TPM2")
    4eea703caaac ("tpm: drop 'iobase' from struct tpm_vendor_specific")
    51dd43dff74b ("tpm_tis: Use devm_ioremap_resource")
    55a889c2cb13 ("tpm_crb: Use the common ACPI definition of struct acpi_tpm2")
    56671c893e0e ("tpm: drop 'locality' from struct tpm_vendor_specific")
    570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
    57dacc2b4ce5 ("tpm: tpm_tis: Share common data between phys")
    719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
    7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    d30b8e4f68ef ("tpm: cleanup tpm_tis_remove()")
    d4956524f1b0 ("tpm: drop manufacturer_id from struct tpm_vendor_specific")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")
    e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
    ee1779840d09 ("tpm: drop 'base' from struct tpm_vendor_specific")
    ef7b81dc7864 ("tpm_tis: Disable interrupt auto probing on a per-device basis")


How should we proceed with this patch?

--
Thanks,
Sasha

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

On 14/06/2019 23:56, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all

It should be only # v5.1+

And seems I also forgot to add some cc for the original patch, sorry.

The referenced patch is here
https://lore.kernel.org/linux-integrity/20190612084210.13562-1-gmazyland@gmail.com/

Milan

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

Hi,

is there any problem in this with the trivial patch below?

I just get the same crash again with stable 5.1 kernel...

Milan


On 12/06/2019 10:42, Milan Broz wrote:
> If clk_enable is not defined and chip initialization
> is canceled code hits null dereference.
> 
> Easily reproducible with vTPM init fail:
>   swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy
> 
> BUG: kernel NULL pointer dereference, address: 00000000
> ...
> Call Trace:
>  tpm_chip_start+0x9d/0xa0 [tpm]
>  tpm_chip_register+0x10/0x1a0 [tpm]
>  vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
>  process_one_work+0x214/0x5a0
>  worker_thread+0x134/0x3e0
>  ? process_one_work+0x5a0/0x5a0
>  kthread+0xd4/0x100
>  ? process_one_work+0x5a0/0x5a0
>  ? kthread_park+0x90/0x90
>  ret_from_fork+0x19/0x24
> 
> Signed-off-by: Milan Broz <gmazyland@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/char/tpm/tpm-chip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 90325e1749fb..4c2af643d698 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -95,7 +95,8 @@ int tpm_chip_start(struct tpm_chip *chip)
>  	if (chip->locality == -1) {
>  		ret = tpm_request_locality(chip);
>  		if (ret) {
> -			chip->ops->clk_enable(chip, false);
> +			if (chip->ops->clk_enable)
> +				chip->ops->clk_enable(chip, false);
>  			return ret;
>  		}
>  	}
> 

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote:
> Hi,
> 
> is there any problem in this with the trivial patch below?
> 
> I just get the same crash again with stable 5.1 kernel...
> 
> Milan

I'm sorry but I'm seeing this patch for the first time.

/Jarkko

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Tue, Jul 02, 2019 at 09:37:51PM +0300, Jarkko Sakkinen wrote:
> On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote:
> > Hi,
> > 
> > is there any problem in this with the trivial patch below?
> > 
> > I just get the same crash again with stable 5.1 kernel...
> > 
> > Milan
> 
> I'm sorry but I'm seeing this patch for the first time.

OK, I finally reviewed your patch. Thank for finding this sever bug! I
just have a few remarks.

First of all, we need to add the necessary fixes tag to the patch, which
will tell the commit ID that caused the crash and the one who we should
blame:

Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")

It was a piece of a fairly large and complex refactorization [1] but it
is not really a legit excuse and this is very unfortunate.

I think it'd be better to take a different path how this will be fixed.

Right after tpm_go_idle(), please add these:

static void tpm_clk_enable(struct tpm_chip *chip)
{
	if (chip->ops->clk_enable)
		chip->ops->clk_enable(chip);
}

static void tpm_tpm_clk_disable(struct tpm_chip *chip)
{
	if (chip->ops->tpm_clk_disable)
		chip->ops->tpm_clk_disable(chip);
}

This is consistent with the other callbacks and gives better guarantees
that a similar thing won't happen the next time when something happens
to the call sites. This is what I should have done in my patch set to
zero out the risk but failed to do that.

You should categorically include the corresponding subsystem
maintainers. Please check [2]. It is not like I would ignore any
patches. It is more related to the risk of human error.

Like many people, I filter any mailing list emails out of my inbox and
go through them at some point. This will cause a random number of days
of latency and with extremely bad luck I even might miss a patch
completely.

As a last remark put patch signed-off-by's and similar tags as last in
your patch and put cc and fixes (in that order) before them.

[1] https://lore.kernel.org/linux-integrity/20190205224723.19671-1-jarkko.sakkinen@linux.intel.com/
[2] https://www.kernel.org/doc/html/v5.1/process/submitting-patches.html#select-the-recipients-for-your-patch

/Jarkko

[PATCH v2] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

If clk_enable is not defined and chip initialization
is canceled code hits null dereference.

Easily reproducible with vTPM init fail:
  swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy

BUG: kernel NULL pointer dereference, address: 00000000
...
Call Trace:
 tpm_chip_start+0x9d/0xa0 [tpm]
 tpm_chip_register+0x10/0x1a0 [tpm]
 vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
 process_one_work+0x214/0x5a0
 worker_thread+0x134/0x3e0
 ? process_one_work+0x5a0/0x5a0
 kthread+0xd4/0x100
 ? process_one_work+0x5a0/0x5a0
 ? kthread_park+0x90/0x90
 ret_from_fork+0x19/0x24

Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
Cc: stable@vger.kernel.org # v5.1+
Signed-off-by: Milan Broz <gmazyland@gmail.com>
---
 drivers/char/tpm/tpm-chip.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 90325e1749fb..db6ac6f83948 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -77,6 +77,18 @@ static int tpm_go_idle(struct tpm_chip *chip)
 	return chip->ops->go_idle(chip);
 }
 
+static void tpm_clk_enable(struct tpm_chip *chip)
+{
+	if (chip->ops->clk_enable)
+		chip->ops->clk_enable(chip, true);
+}
+
+static void tpm_clk_disable(struct tpm_chip *chip)
+{
+	if (chip->ops->clk_enable)
+		chip->ops->clk_enable(chip, false);
+}
+
 /**
  * tpm_chip_start() - power on the TPM
  * @chip:	a TPM chip to use
@@ -89,13 +101,12 @@ int tpm_chip_start(struct tpm_chip *chip)
 {
 	int ret;
 
-	if (chip->ops->clk_enable)
-		chip->ops->clk_enable(chip, true);
+	tpm_clk_enable(chip);
 
 	if (chip->locality == -1) {
 		ret = tpm_request_locality(chip);
 		if (ret) {
-			chip->ops->clk_enable(chip, false);
+			tpm_clk_disable(chip);
 			return ret;
 		}
 	}
@@ -103,8 +114,7 @@ int tpm_chip_start(struct tpm_chip *chip)
 	ret = tpm_cmd_ready(chip);
 	if (ret) {
 		tpm_relinquish_locality(chip);
-		if (chip->ops->clk_enable)
-			chip->ops->clk_enable(chip, false);
+		tpm_clk_disable(chip);
 		return ret;
 	}
 
@@ -124,8 +134,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
 {
 	tpm_go_idle(chip);
 	tpm_relinquish_locality(chip);
-	if (chip->ops->clk_enable)
-		chip->ops->clk_enable(chip, false);
+	tpm_clk_disable(chip);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
 
-- 
2.20.1

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Thu, 2019-07-04 at 02:01 +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 02, 2019 at 09:37:51PM +0300, Jarkko Sakkinen wrote:
> > On Fri, 2019-06-28 at 09:56 +0200, Milan Broz wrote:
> > > Hi,
> > > 
> > > is there any problem in this with the trivial patch below?
> > > 
> > > I just get the same crash again with stable 5.1 kernel...
> > > 
> > > Milan
> > 
> > I'm sorry but I'm seeing this patch for the first time.
> 
> OK, I finally reviewed your patch. Thank for finding this sever bug! I
> just have a few remarks.
> 
> First of all, we need to add the necessary fixes tag to the patch, which
> will tell the commit ID that caused the crash and the one who we should
> blame:
> 
> Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
> 
> It was a piece of a fairly large and complex refactorization [1] but it
> is not really a legit excuse and this is very unfortunate.
> 
> I think it'd be better to take a different path how this will be fixed.
> 
> Right after tpm_go_idle(), please add these:
> 
> static void tpm_clk_enable(struct tpm_chip *chip)
> {
> 	if (chip->ops->clk_enable)
> 		chip->ops->clk_enable(chip);
> }
> 
> static void tpm_tpm_clk_disable(struct tpm_chip *chip)
> {
> 	if (chip->ops->tpm_clk_disable)
> 		chip->ops->tpm_clk_disable(chip);
> }
> 
> This is consistent with the other callbacks and gives better guarantees
> that a similar thing won't happen the next time when something happens
> to the call sites. This is what I should have done in my patch set to
> zero out the risk but failed to do that.
> 
> You should categorically include the corresponding subsystem
> maintainers. Please check [2]. It is not like I would ignore any
> patches. It is more related to the risk of human error.
> 
> Like many people, I filter any mailing list emails out of my inbox and
> go through them at some point. This will cause a random number of days
> of latency and with extremely bad luck I even might miss a patch
> completely.
> 
> As a last remark put patch signed-off-by's and similar tags as last in
> your patch and put cc and fixes (in that order) before them.
> 
> [1] 
> https://lore.kernel.org/linux-integrity/20190205224723.19671-1-jarkko.sakkinen@linux.intel.com/
> [2] 
> https://www.kernel.org/doc/html/v5.1/process/submitting-patches.html#select-the-recipients-for-your-patch


So how should we work this one out? Do you want to create v2 or do I
create a new patch and put reported-by tag. Both work for me. I just
need to know this.

/Jarkko

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Milan Broz]

On 05/07/2019 19:52, Jarkko Sakkinen wrote:
> 
> So how should we work this one out? Do you want to create v2 or do I
> create a new patch and put reported-by tag. Both work for me. I just
> need to know this.

Please fix you mail filters, the patch was sent more than day ago.

https://lore.kernel.org/linux-integrity/20190704072615.31143-1-gmazyland@gmail.com/T/#u

Milan

Re: [PATCH] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Fri, 2019-07-05 at 23:23 +0200, Milan Broz wrote:
> On 05/07/2019 19:52, Jarkko Sakkinen wrote:
> > So how should we work this one out? Do you want to create v2 or do I
> > create a new patch and put reported-by tag. Both work for me. I just
> > need to know this.
> 
> Please fix you mail filters, the patch was sent more than day ago.
> 
> 
https://lore.kernel.org/linux-integrity/20190704072615.31143-1-gmazyland@gmail.com/T/#u
> 
> Milan

Nothing wrong with my mail filters, it was actually waiting in my inbox.
Just didn't notice it that it was part of the emails I downloaded to my
laptop with fdm on Friday (because in rush to spend weekend). Didn't
have time to go through all of them during that day.

/Jarkko

Re: [PATCH v2] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Thu, 2019-07-04 at 09:26 +0200, Milan Broz wrote:
> If clk_enable is not defined and chip initialization
> is canceled code hits null dereference.
> 
> Easily reproducible with vTPM init fail:
>   swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy
> 
> BUG: kernel NULL pointer dereference, address: 00000000
> ...
> Call Trace:
>  tpm_chip_start+0x9d/0xa0 [tpm]
>  tpm_chip_register+0x10/0x1a0 [tpm]
>  vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
>  process_one_work+0x214/0x5a0
>  worker_thread+0x134/0x3e0
>  ? process_one_work+0x5a0/0x5a0
>  kthread+0xd4/0x100
>  ? process_one_work+0x5a0/0x5a0
>  ? kthread_park+0x90/0x90
>  ret_from_fork+0x19/0x24
> 
> Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
> Cc: stable@vger.kernel.org # v5.1+
> Signed-off-by: Milan Broz <gmazyland@gmail.com>

Looks legit.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2] tpm: Fix null pointer dereference on chip register error path

[Jarkko Sakkinen]

On Mon, 2019-07-08 at 17:34 +0300, Jarkko Sakkinen wrote:
> On Thu, 2019-07-04 at 09:26 +0200, Milan Broz wrote:
> > If clk_enable is not defined and chip initialization
> > is canceled code hits null dereference.
> > 
> > Easily reproducible with vTPM init fail:
> >   swtpm chardev --tpmstate dir=nonexistent_dir --tpm2 --vtpm-proxy
> > 
> > BUG: kernel NULL pointer dereference, address: 00000000
> > ...
> > Call Trace:
> >  tpm_chip_start+0x9d/0xa0 [tpm]
> >  tpm_chip_register+0x10/0x1a0 [tpm]
> >  vtpm_proxy_work+0x11/0x30 [tpm_vtpm_proxy]
> >  process_one_work+0x214/0x5a0
> >  worker_thread+0x134/0x3e0
> >  ? process_one_work+0x5a0/0x5a0
> >  kthread+0xd4/0x100
> >  ? process_one_work+0x5a0/0x5a0
> >  ? kthread_park+0x90/0x90
> >  ret_from_fork+0x19/0x24
> > 
> > Fixes: 719b7d81f204 ("tpm: introduce tpm_chip_start() and tpm_chip_stop()")
> > Cc: stable@vger.kernel.org # v5.1+
> > Signed-off-by: Milan Broz <gmazyland@gmail.com>
> 
> Looks legit.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Please check master and next branches from

  git://git.infradead.org/users/jjs/linux-tpmdd.git

/Jarkko