On Thu, Jan 03, 2019 at 08:17:18AM -0800, James Bottomley wrote: > On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote: > > > -----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@HansenPartnersh > > > > > ip.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. > > Taking the trouble to gather error returns and then ignoring them is > not a good practice (it's actually been the bane of filesystems for a > while). If you want to do it this way, tpm_go_idle() needs to be a > void function that emits an error message for every problem condition. I'm happy to take a patch that adds logging in. /Jarkko
