* [PATCH] tpm: fix unused-value issues in tpm_try_transmit
@ 2018-10-10 13:38 Gustavo A. R. Silva
2018-10-10 14:06 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-10-10 13:38 UTC (permalink / raw)
To: Tomas Winkler, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-integrity, linux-kernel, Gustavo A. R. Silva
Currently, there are some values assigned to variable *rc*, which
are never actually used in any computation, because such variable
is updated at line 550, before they can be used:
549out:
550 rc = tpm_go_idle(chip, flags);
551 if (rc)
552 goto out;
Fix this by removing such assignments.
Addresses-Coverity-ID: 1470245 ("Unused value")
Addresses-Coverity-ID: 1470250 ("Unused value")
Addresses-Coverity-ID: 1470251 ("Unused value")
Fixes: 627448e85c76 ("tpm: separate cmd_ready/go_idle from runtime_pm")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/char/tpm/tpm-interface.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640..8062736 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -512,7 +512,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
if (chip->ops->req_canceled(chip, status)) {
dev_err(&chip->dev, "Operation Canceled\n");
- rc = -ECANCELED;
goto out;
}
@@ -522,7 +521,6 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
chip->ops->cancel(chip);
dev_err(&chip->dev, "Operation Timed out\n");
- rc = -ETIME;
goto out;
out_recv:
@@ -533,14 +531,12 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
"tpm_transmit: tpm_recv: error %d\n", rc);
goto out;
} else if (len < TPM_HEADER_SIZE) {
- rc = -EFAULT;
goto out;
}
- if (len != be32_to_cpu(header->length)) {
- rc = -EFAULT;
+ if (len != be32_to_cpu(header->length))
goto out;
- }
+
rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
if (rc)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
2018-10-10 13:38 [PATCH] tpm: fix unused-value issues in tpm_try_transmit Gustavo A. R. Silva
@ 2018-10-10 14:06 ` Jason Gunthorpe
2018-10-11 12:01 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2018-10-10 14:06 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Tomas Winkler, Peter Huewe, Jarkko Sakkinen, Arnd Bergmann,
Greg Kroah-Hartman, linux-integrity, linux-kernel
On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote:
> Currently, there are some values assigned to variable *rc*, which
> are never actually used in any computation, because such variable
> is updated at line 550, before they can be used:
>
> 549out:
> 550 rc = tpm_go_idle(chip, flags);
> 551 if (rc)
> 552 goto out;
>
> Fix this by removing such assignments.
Should this be done by not quashing rc during the error unwind rather
than dropping the errors?
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
2018-10-10 14:06 ` Jason Gunthorpe
@ 2018-10-11 12:01 ` Jarkko Sakkinen
2018-10-11 13:27 ` Winkler, Tomas
0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2018-10-11 12:01 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Gustavo A. R. Silva, Tomas Winkler, Peter Huewe, Arnd Bergmann,
Greg Kroah-Hartman, linux-integrity, linux-kernel
On Wed, Oct 10, 2018 at 08:06:38AM -0600, Jason Gunthorpe wrote:
> On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote:
> > Currently, there are some values assigned to variable *rc*, which
> > are never actually used in any computation, because such variable
> > is updated at line 550, before they can be used:
> >
> > 549out:
> > 550 rc = tpm_go_idle(chip, flags);
> > 551 if (rc)
> > 552 goto out;
> >
> > Fix this by removing such assignments.
>
> Should this be done by not quashing rc during the error unwind rather
> than dropping the errors?
Yeah.`
Wondering if tpm_go_idle() should simply be a void-function i.e. issue
just a warning inside (disclaimer: did not revisit its code when writing
this).
> Jason
/Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
2018-10-11 12:01 ` Jarkko Sakkinen
@ 2018-10-11 13:27 ` Winkler, Tomas
2018-10-15 10:41 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Winkler, Tomas @ 2018-10-11 13:27 UTC (permalink / raw)
To: Jarkko Sakkinen, Jason Gunthorpe
Cc: Gustavo A. R. Silva, Peter Huewe, Arnd Bergmann,
Greg Kroah-Hartman, linux-integrity, linux-kernel
>
> On Wed, Oct 10, 2018 at 08:06:38AM -0600, Jason Gunthorpe wrote:
> > On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote:
> > > Currently, there are some values assigned to variable *rc*, which
> > > are never actually used in any computation, because such variable is
> > > updated at line 550, before they can be used:
> > >
> > > 549out:
> > > 550 rc = tpm_go_idle(chip, flags);
> > > 551 if (rc)
> > > 552 goto out;
> > >
> > > Fix this by removing such assignments.
> >
> > Should this be done by not quashing rc during the error unwind rather
> > than dropping the errors?
>
> Yeah.`
>
> Wondering if tpm_go_idle() should simply be a void-function i.e. issue just a
> warning inside (disclaimer: did not revisit its code when writing this).
We did have rather a long discussion about it when it was merged.
There are two flows that may crash
rc = tpm2_commit_space()
but you still can need to
rc = go_idle()
which also may crash which may override the previous value.
Frankly the second one is fatal, the stack will go out of sync.
We may do void here as the stack will crash in a subsequent command.
The 'goto out' is quite a bug, probably caused by code movement.
Thanks
Tomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
2018-10-11 13:27 ` Winkler, Tomas
@ 2018-10-15 10:41 ` Jarkko Sakkinen
2018-10-15 11:18 ` Winkler, Tomas
0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2018-10-15 10:41 UTC (permalink / raw)
To: Winkler, Tomas
Cc: Jason Gunthorpe, Gustavo A. R. Silva, Peter Huewe, Arnd Bergmann,
Greg Kroah-Hartman, linux-integrity, linux-kernel
On Thu, Oct 11, 2018 at 01:27:58PM +0000, Winkler, Tomas wrote:
> >
> > On Wed, Oct 10, 2018 at 08:06:38AM -0600, Jason Gunthorpe wrote:
> > > On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote:
> > > > Currently, there are some values assigned to variable *rc*, which
> > > > are never actually used in any computation, because such variable is
> > > > updated at line 550, before they can be used:
> > > >
> > > > 549out:
> > > > 550 rc = tpm_go_idle(chip, flags);
> > > > 551 if (rc)
> > > > 552 goto out;
> > > >
> > > > Fix this by removing such assignments.
> > >
> > > Should this be done by not quashing rc during the error unwind rather
> > > than dropping the errors?
> >
> > Yeah.`
> >
> > Wondering if tpm_go_idle() should simply be a void-function i.e. issue just a
> > warning inside (disclaimer: did not revisit its code when writing this).
>
> We did have rather a long discussion about it when it was merged.
> There are two flows that may crash
> rc = tpm2_commit_space()
>
> but you still can need to
>
> rc = go_idle()
>
> which also may crash which may override the previous value.
>
> Frankly the second one is fatal, the stack will go out of sync.
> We may do void here as the stack will crash in a subsequent command.
>
> The 'goto out' is quite a bug, probably caused by code movement.
I just looked at the code properly and noticed that there is a regression
caused by 627448e85c76 ("tpm: separate cmd_ready/go_idle from
runtime_pm") i.e. when tpm_go_idle() fails it loops back and retries
tpm_go_idle().
/Jarkko
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] tpm: fix unused-value issues in tpm_try_transmit
2018-10-15 10:41 ` Jarkko Sakkinen
@ 2018-10-15 11:18 ` Winkler, Tomas
0 siblings, 0 replies; 6+ messages in thread
From: Winkler, Tomas @ 2018-10-15 11:18 UTC (permalink / raw)
To: jarkko.sakkinen
Cc: linux-integrity, linux-kernel, jgg, gustavo, arnd, peterhuewe, gregkh
On Mon, 2018-10-15 at 13:41 +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 11, 2018 at 01:27:58PM +0000, Winkler, Tomas wrote:
> > >
> > > On Wed, Oct 10, 2018 at 08:06:38AM -0600, Jason Gunthorpe wrote:
> > > > On Wed, Oct 10, 2018 at 03:38:17PM +0200, Gustavo A. R. Silva wrote:
> > > > > Currently, there are some values assigned to variable *rc*, which
> > > > > are never actually used in any computation, because such variable is
> > > > > updated at line 550, before they can be used:
> > > > >
> > > > > 549out:
> > > > > 550 rc = tpm_go_idle(chip, flags);
> > > > > 551 if (rc)
> > > > > 552 goto out;
> > > > >
> > > > > Fix this by removing such assignments.
> > > >
> > > > Should this be done by not quashing rc during the error unwind rather
> > > > than dropping the errors?
> > >
> > > Yeah.`
> > >
> > > Wondering if tpm_go_idle() should simply be a void-function i.e. issue just a
> > > warning inside (disclaimer: did not revisit its code when writing this).
> >
> > We did have rather a long discussion about it when it was merged.
> > There are two flows that may crash
> > rc = tpm2_commit_space()
> >
> > but you still can need to
> >
> > rc = go_idle()
> >
> > which also may crash which may override the previous value.
> >
> > Frankly the second one is fatal, the stack will go out of sync.
> > We may do void here as the stack will crash in a subsequent command.
> >
> > The 'goto out' is quite a bug, probably caused by code movement.
>
> I just looked at the code properly and noticed that there is a regression
> caused by 627448e85c76 ("tpm: separate cmd_ready/go_idle from
> runtime_pm") i.e. when tpm_go_idle() fails it loops back and retries
> tpm_go_idle().
Yes, that's what I said, this part code was moved forth but no the
label.
Tomas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-15 11:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 13:38 [PATCH] tpm: fix unused-value issues in tpm_try_transmit Gustavo A. R. Silva
2018-10-10 14:06 ` Jason Gunthorpe
2018-10-11 12:01 ` Jarkko Sakkinen
2018-10-11 13:27 ` Winkler, Tomas
2018-10-15 10:41 ` Jarkko Sakkinen
2018-10-15 11:18 ` Winkler, Tomas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).