| Message ID | 1546280851.3079.2.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: fix incorrect success returns from tpm_try_transmit() | expand |
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@vger.kernel.org > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > 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? /Jarkko
> > 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@vger.kernel.org > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > --- > > > > 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? This is wrong as well because of the jump to 'out'. I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct. If it's okay with you I will post my version of the fix. Thanks Tomas
On Thu, Jan 03, 2019 at 01:37:09PM +0000, Winkler, Tomas 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@vger.kernel.org > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > --- > > > > > > 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? > > > This is wrong as well because of the jump to 'out'. > I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct. > If it's okay with you I will post my version of the fix. > Thanks > Tomas Works for me. /Jarkko
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@vger.kernel.org > > 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. James
> -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: Thursday, January 03, 2019 17:24 > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > integrity@vger.kernel.org > 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@vger.kernel.org > > > 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
On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote: > > -----Original Message----- > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com > > ] > > Sent: Thursday, January 03, 2019 17:24 > > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > > integrity@vger.kernel.org > > 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@vger.kernel.org > > > > 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. James
On Thu, Jan 03, 2019 at 03:34:24PM +0000, Winkler, Tomas wrote: > 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 LOL what. Clearly I'm coming from holidays. This fix is in my tree and it was part of my last PR to James (for 5.0). And James' patch applies neither my master and next braches so I suppose he is using some old tree. /Jarkko
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@HansenPartnership.com > > > ] > > > Sent: Thursday, January 03, 2019 17:24 > > > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux- > > > integrity@vger.kernel.org > > > 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@vger.kernel.org > > > > > 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
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)
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@vger.kernel.org Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- 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