Linux-Integrity Archive on lore.kernel.org
 help / Atom feed
* [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
@ 2018-12-31 18:27 James Bottomley
  2019-01-03 12:59 ` Jarkko Sakkinen
  0 siblings, 1 reply; 9+ messages in thread
From: James Bottomley @ 2018-12-31 18:27 UTC (permalink / raw)
  To: Jarkko Sakkinen, Tomas Winkler; +Cc: linux-integrity

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)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2018-12-31 18:27 [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() James Bottomley
@ 2019-01-03 12:59 ` Jarkko Sakkinen
  2019-01-03 13:37   ` Winkler, Tomas
  2019-01-03 15:23   ` James Bottomley
  0 siblings, 2 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-03 12:59 UTC (permalink / raw)
  To: James Bottomley; +Cc: Tomas Winkler, linux-integrity

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 12:59 ` Jarkko Sakkinen
@ 2019-01-03 13:37   ` Winkler, Tomas
  2019-01-03 15:03     ` Jarkko Sakkinen
  2019-01-03 15:23   ` James Bottomley
  1 sibling, 1 reply; 9+ messages in thread
From: Winkler, Tomas @ 2019-01-03 13:37 UTC (permalink / raw)
  To: Jarkko Sakkinen, James Bottomley; +Cc: linux-integrity

> 
> 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 13:37   ` Winkler, Tomas
@ 2019-01-03 15:03     ` Jarkko Sakkinen
  0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-03 15:03 UTC (permalink / raw)
  To: Winkler, Tomas; +Cc: James Bottomley, linux-integrity

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 12:59 ` Jarkko Sakkinen
  2019-01-03 13:37   ` Winkler, Tomas
@ 2019-01-03 15:23   ` James Bottomley
  2019-01-03 15:34     ` Winkler, Tomas
  1 sibling, 1 reply; 9+ messages in thread
From: James Bottomley @ 2019-01-03 15:23 UTC (permalink / raw)
  To: Jarkko Sakkinen; +Cc: Tomas Winkler, linux-integrity

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 15:23   ` James Bottomley
@ 2019-01-03 15:34     ` Winkler, Tomas
  2019-01-03 16:17       ` James Bottomley
  2019-01-10 17:16       ` Jarkko Sakkinen
  0 siblings, 2 replies; 9+ messages in thread
From: Winkler, Tomas @ 2019-01-03 15:34 UTC (permalink / raw)
  To: James Bottomley, Jarkko Sakkinen; +Cc: linux-integrity



> -----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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 15:34     ` Winkler, Tomas
@ 2019-01-03 16:17       ` James Bottomley
  2019-01-10 17:16         ` Jarkko Sakkinen
  2019-01-10 17:16       ` Jarkko Sakkinen
  1 sibling, 1 reply; 9+ messages in thread
From: James Bottomley @ 2019-01-03 16:17 UTC (permalink / raw)
  To: Winkler, Tomas, Jarkko Sakkinen; +Cc: linux-integrity

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 15:34     ` Winkler, Tomas
  2019-01-03 16:17       ` James Bottomley
@ 2019-01-10 17:16       ` Jarkko Sakkinen
  1 sibling, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-10 17:16 UTC (permalink / raw)
  To: Winkler, Tomas; +Cc: James Bottomley, linux-integrity

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
  2019-01-03 16:17       ` James Bottomley
@ 2019-01-10 17:16         ` Jarkko Sakkinen
  0 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2019-01-10 17:16 UTC (permalink / raw)
  To: James Bottomley; +Cc: Winkler, Tomas, linux-integrity

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-31 18:27 [PATCH] tpm: fix incorrect success returns from tpm_try_transmit() James Bottomley
2019-01-03 12:59 ` Jarkko Sakkinen
2019-01-03 13:37   ` Winkler, Tomas
2019-01-03 15:03     ` Jarkko Sakkinen
2019-01-03 15:23   ` James Bottomley
2019-01-03 15:34     ` Winkler, Tomas
2019-01-03 16:17       ` James Bottomley
2019-01-10 17:16         ` Jarkko Sakkinen
2019-01-10 17:16       ` Jarkko Sakkinen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox