tpm: fix incorrect success returns from tpm_try_transmit()

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

Signed-off-by James Bottomley <[email protected]>

---

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

drivers/char/tpm/tpm-interface.c +1 -1

index 129f640424b7..ac7ebab6140c 100644
@@ -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;

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip

Quoted 23 lines

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

Signed-off-by James Bottomley <[email protected]>

---

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

drivers/char/tpm/tpm-interface.c +0 -0

index 129f640424b7..ac7ebab6140c 100644
@@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip

*chip,

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip

Quoted 9 lines

drivers/char/tpm/tpm-interface.c +1 -1

 				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,

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, · line 554

Quoted 10 lines

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

 		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?

Quoted 53 lines · James Bottomley

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip

Quoted 23 lines

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

Signed-off-by James Bottomley <[email protected]>

---

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

drivers/char/tpm/tpm-interface.c +0 -0

index 129f640424b7..ac7ebab6140c 100644
@@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip

*chip,

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip

Quoted 9 lines

drivers/char/tpm/tpm-interface.c +1 -1

 				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,

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, · line 554

Quoted 10 lines

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

 		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;

Quoted 1 line

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

Quoted 51 lines · James Bottomley

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

Signed-off-by James Bottomley <[email protected]

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

drivers/char/tpm/tpm-interface.c +1 -1

index 129f640424b7..ac7ebab6140c 100644
@@ -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;

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

 	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;

Quoted 1 line

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?

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Thursday, January 03, 2019 17:24
To: Jarkko Sakkinen <[email protected]>
Cc: Winkler, Tomas <[email protected]>; linux- [email protected]
Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

Quoted 51 lines · Jarkko Sakkinen

Quoted 45 lines · James Bottomley

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

Signed-off-by James Bottomley <[email protected]

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

drivers/char/tpm/tpm-interface.c +1 -1

index 129f640424b7..ac7ebab6140c 100644
@@ -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;

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

 	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;
 

Quoted 1 line

if (need_locality)

Thanks James and sorry for latency (holiday season). Just a small suggestion. I would just:

Quoted 4 lines

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.

Quoted 77 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected] ]
Sent: Thursday, January 03, 2019 17:24
To: Jarkko Sakkinen <[email protected]>
Cc: Winkler, Tomas <[email protected]>; linux- [email protected]
Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

Quoted 56 lines · Jarkko Sakkinen

Quoted 44 lines · James Bottomley

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

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

drivers/char/tpm/tpm-interface.c +0 -0

index 129f640424b7..ac7ebab6140c 100644
@@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct

tpm_chip
*chip,

drivers/char/tpm/tpm-interface.c +1 -1

 				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:

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, · line 554

Quoted 7 lines

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

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

Quoted 4 lines

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)

Quoted 9 lines

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.

Quoted 87 lines · Tomas

Quoted 77 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected] ]
Sent: Thursday, January 03, 2019 17:24
To: Jarkko Sakkinen <[email protected]>
Cc: Winkler, Tomas <[email protected]>; linux- [email protected]
Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

Quoted 56 lines · Jarkko Sakkinen

Quoted 44 lines · James Bottomley

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

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

drivers/char/tpm/tpm-interface.c +0 -0

index 129f640424b7..ac7ebab6140c 100644
@@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct

tpm_chip
*chip,

drivers/char/tpm/tpm-interface.c +1 -1

 				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:

drivers/char/tpm/tpm-interface.c static ssize_t tpm_try_transmit(struct tpm_chip *chip, · line 554

Quoted 7 lines

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

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

Quoted 4 lines

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)

Quoted 9 lines

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.

Quoted 4 lines

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.

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