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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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