diff mbox series

tpm: tpm_try_transmit() ignore value of go_to_idle()

Message ID 20181015111434.7777-1-tomas.winkler@intel.com
State New
Headers show
Series tpm: tpm_try_transmit() ignore value of go_to_idle() | expand

Commit Message

Winkler, Tomas Oct. 15, 2018, 11:14 a.m. UTC
Ignore the return value of go_to_idle() in tpm_try_transmit().
Once it may shadow the return value of actual tpm operation,
second the consequent command will fail as well and the error
will be caought anyway.
Last fix wrong goto, that jumped back instead of forward.

Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Jason Gunthorpe Oct. 16, 2018, 5:41 a.m. UTC | #1
On Mon, Oct 15, 2018 at 02:14:34PM +0300, Tomas Winkler wrote:
> Ignore the return value of go_to_idle() in tpm_try_transmit().
> Once it may shadow the return value of actual tpm operation,
> second the consequent command will fail as well and the error
> will be caought anyway.
> Last fix wrong goto, that jumped back instead of forward.
> 
> Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>  drivers/char/tpm/tpm-interface.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 129f640424b7..f69c711bf74a 100644
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -547,9 +547,7 @@ 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)
> -		goto out;
> +	(void)tpm_go_idle(chip, flags);

We don't really use this style in the kernel, AFAIK.

Jason
Winkler, Tomas Oct. 16, 2018, 6:20 a.m. UTC | #2
> 
> On Mon, Oct 15, 2018 at 02:14:34PM +0300, Tomas Winkler wrote:
> > Ignore the return value of go_to_idle() in tpm_try_transmit().
> > Once it may shadow the return value of actual tpm operation, second
> > the consequent command will fail as well and the error will be caought
> > anyway.
> > Last fix wrong goto, that jumped back instead of forward.
> >
> > Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from
> > runtime_pm")
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > drivers/char/tpm/tpm-interface.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 129f640424b7..f69c711bf74a 100644
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -547,9 +547,7 @@ 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)
> > -		goto out;
> > +	(void)tpm_go_idle(chip, flags);
> 
> We don't really use this style in the kernel, AFAIK.


Right, there are no many of those; just wanted to emphasize that return value is ignored. I can drop (void).

Thanks
Tomas
Jarkko Sakkinen Oct. 17, 2018, 11:01 p.m. UTC | #3
On Mon, 15 Oct 2018, Tomas Winkler wrote:
> Ignore the return value of go_to_idle() in tpm_try_transmit().
> Once it may shadow the return value of actual tpm operation,
> second the consequent command will fail as well and the error
> will be caought anyway.
> Last fix wrong goto, that jumped back instead of forward.
>
> Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> drivers/char/tpm/tpm-interface.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 129f640424b7..f69c711bf74a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -547,9 +547,7 @@ 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)
> -		goto out;
> +	(void)tpm_go_idle(chip, flags);
>
> 	if (need_locality)
> 		tpm_relinquish_locality(chip, flags);
> -- 
> 2.14.4
>
>

LGTM. Should be probably Cc'd to stable (can add).

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
Winkler, Tomas Oct. 17, 2018, 11:08 p.m. UTC | #4
> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, October 18, 2018 02:01
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Jason Gunthorpe
> <jgg@ziepe.ca>; Nayna Jain <nayna@linux.vnet.ibm.com>; Usyskin,
> Alexander <alexander.usyskin@intel.com>; Struk, Tadeusz
> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org; linux-security-
> module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] tpm: tpm_try_transmit() ignore value of go_to_idle()
> 
> On Mon, 15 Oct 2018, Tomas Winkler wrote:
> > Ignore the return value of go_to_idle() in tpm_try_transmit().
> > Once it may shadow the return value of actual tpm operation, second
> > the consequent command will fail as well and the error will be caought
> > anyway.
> > Last fix wrong goto, that jumped back instead of forward.
> >
> > Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from
> > runtime_pm")
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > drivers/char/tpm/tpm-interface.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 129f640424b7..f69c711bf74a 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -547,9 +547,7 @@ 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)
> > -		goto out;
> > +	(void)tpm_go_idle(chip, flags);
> >
> > 	if (need_locality)
> > 		tpm_relinquish_locality(chip, flags);
> > --
> > 2.14.4
> >
> >
> 
> LGTM. Should be probably Cc'd to stable (can add).
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
I've posted another one, there are more issues in the flow.
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640424b7..f69c711bf74a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -547,9 +547,7 @@  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)
-		goto out;
+	(void)tpm_go_idle(chip, flags);
 
 	if (need_locality)
 		tpm_relinquish_locality(chip, flags);