> -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx] > Sent: Thursday, January 03, 2019 17:24 > To: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx> > Cc: Winkler, Tomas <tomas.winkler@xxxxxxxxx>; linux- > integrity@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() > > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote: > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote: > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > runtime_pm" we have been returning success from tpm_try_transmit() > > > even if an error occurred. The reason is that the introduction of > > > rc = tpm_go_idle() at the end of processing overwrites the value of > > > rc if it contains an error code (mostly with success). Fix this by > > > writing the return to a new variable rc1 instead. > > > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from > > > runtime_pm" > > > Cc: stable@xxxxxxxxxxxxxxx > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > > om> > > > > > > --- > > > > > > Note: the goto out looks fishy as well. The only go_idle > > > implementor is tpm_crb and that can return a timeout as -ETIME, so > > > it looks like it would then loop forever > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > > b/drivers/char/tpm/tpm-interface.c > > > index 129f640424b7..ac7ebab6140c 100644 > > > --- a/drivers/char/tpm/tpm-interface.c > > > +++ b/drivers/char/tpm/tpm-interface.c > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > > *chip, > > > unsigned int flags) > > > { > > > struct tpm_output_header *header = (void *)buf; > > > - int rc; > > > + int rc, rc1; > > > ssize_t len = 0; > > > u32 count, ordinal; > > > unsigned long stop; > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip > > > *chip, > > > dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc); > > > > > > out: > > > - rc = tpm_go_idle(chip, flags); > > > - if (rc) > > > + rc1 = tpm_go_idle(chip, flags); > > > + if (rc1) > > > goto out; > > > > > > if (need_locality) > > > > Thanks James and sorry for latency (holiday season). Just a small > > suggestion. I would just: > > > > if (tpm_go_idle(chip, flags)) > > goto out; > > > > What do you think? > > That it doesn't solve the loop forever with no warning problem. If anything, I > think the correct thing is probably > > rc1 = tpm_go_idle(chip, flags); > if (rc1) > dev_err(&chip->dev, "go idle failed with %d\n", rc1); > > so we log the problem and move on. If it is a timeout, it will likely show up on > the next TPM operation. Since this is the only caller of tpm_go_idle(), I think all > looping should be done inside that function, but we should probably wait for > Tomas to comment since he wrote it. > We've already fixed it, I forgot myself , we were drinking too much :) https://patchwork.kernel.org/patch/10643565/ Not sure why it was dropped. Thanks Tomas
