tpm: fix unused-value issues in tpm_try_transmit

PATCH In review Gustavo A. R. Silva 4 participants 6 messages Oct 10, 2018 – Oct 15, 2018

Subsystems (2)

Keys-trusted TPM device driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-10-10 13:38 UTC #0 lore raw

PATCH 1/1 tpm: fix unused-value issues in tpm_try_transmit

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 <[email protected]>

---
 drivers/char/tpm/tpm-interface.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/char/tpm/tpm-interface.c +2 -6

index 129f640..8062736 100644
@@ -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)

Signature · 1 line

2.7.4

Jason Gunthorpe

reviewer

2018-10-10 14:06 UTC #1 lore raw

Quoted 10 lines · 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.

Should this be done by not quashing rc during the error unwind rather than dropping the errors?

Jason

Jarkko Sakkinen

2018-10-11 12:01 UTC #2 lore raw

Quoted 10 lines · Jason Gunthorpe

Quoted 9 lines · 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;

Quoted 4 lines

Quoted 1 line
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).

Quoted 1 line

Jason

/Jarkko

Winkler, Tomas

2018-10-11 13:28 UTC #3 lore raw

Quoted 14 lines · Jason Gunthorpe

Quoted 12 lines · Gustavo A. R. Silva
Quoted 10 lines
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.

Quoted 7 lines

Quoted 2 lines
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

Jarkko Sakkinen

2018-10-15 10:42 UTC #4 lore raw

Quoted 28 lines · Tomas

Quoted 14 lines · Jason Gunthorpe
Quoted 12 lines · Gustavo A. R. Silva
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
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

Quoted 8 lines

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

Winkler, Tomas

2018-10-15 11:18 UTC #5 lore raw

Quoted 36 lines · Jarkko Sakkinen

Quoted 35 lines · Tomas
Quoted 14 lines · Jason Gunthorpe
Quoted 12 lines · Gustavo A. R. Silva
Quoted 10 lines
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.
Quoted 7 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 1 line
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