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

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

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