Check for error return values from get_burstcount.

If the TPM we're connecting to uses a static burst count, it will report a burst count of zero throughout the response read. However, get_burstcount assumes that a response of zero indicates that the TPM is not ready to receive more data. In this case, it returns a negative error code, which is passed on to tpm_tis_{write,read}_bytes as a u16, causing them to read/write far too many bytes.

This patch checks for negative return codes and bails out from recv_data and tpm_tis_send_data.

---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

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

index e3bf31b..d0301dc 100644

drivers/char/tpm/tpm_tis_core.c +7 -0

 		rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
 					burstcnt, buf + size);

@@ -272,6 +278,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 
 	while (count < len - 1) {
 		burstcnt = min_t(int, get_burstcount(chip), len - count - 1);
+		if (burstcnt < 0) {
+			dev_err(&chip->dev,
+				"Unable to read burstcount in %s:%d (%s)\n",
+				__FILE__, __LINE__, __func__);
+			rc = burstcnt;
+			goto out_err;
+		}
 		rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
 					 burstcnt, buf + count);
 		if (rc < 0)

Signature · 1 line

2.8.0.rc3.226.g39d4020

Quoted 7 lines · Josh Zimmerman

If the TPM we're connecting to uses a static burst count, it will report a burst count of zero throughout the response read. However, get_burstcount assumes that a response of zero indicates that the TPM is not ready to receive more data. In this case, it returns a negative error code, which is passed on to tpm_tis_{write,read}_bytes as a u16, causing them to read/write far too many bytes.

Quoted 2 lines

This patch checks for negative return codes and bails out from recv_data and tpm_tis_send_data.

I guess this would need a "Fixes:" tag, wouldn't it?

I would also add

Cc [email protected]

Quoted 4 lines

---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Quoted 5 lines

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

index e3bf31b..d0301dc 100644

drivers/char/tpm/tpm_tis_core.c static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) · line 194

Quoted 9 lines

drivers/char/tpm/tpm_tis_core.c +6 -0

@@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
                               chip->timeout_c,
                               &priv->read_queue, true) == 0) {
              burstcnt = min_t(int, get_burstcount(chip), count - size);
+             if (burstcnt < 0) {
+                     dev_err(&chip->dev,
+                             "Unable to read burstcount in %s:%d (%s)\n",
+                             __FILE__, __LINE__, __func__);
+                     return rc;
+             }

"return burstcnt;"

I guess __func__ would be enough to deduce the call site?

Quoted 5 lines

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

              rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality),
                                      burstcnt, buf + size);

@@ -272,6 +278,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len)
 

Quoted 13 lines

drivers/char/tpm/tpm_tis_core.c +7 -1

      while (count < len - 1) {
              burstcnt = min_t(int, get_burstcount(chip), len - count - 1);
+             if (burstcnt < 0) {
+                     dev_err(&chip->dev,
+                             "Unable to read burstcount in %s:%d (%s)\n",
+                             __FILE__, __LINE__, __func__);
+                     rc = burstcnt;
+                     goto out_err;
+             }
              rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
                                       burstcnt, buf + count);
              if (rc < 0)
--

Quoted 1 line

2.8.0.rc3.226.g39d4020

/Jarkko

Quoted 8 lines · Jarkko Sakkinen

Quoted 7 lines · Josh Zimmerman

If the TPM we're connecting to uses a static burst count, it will report a burst count of zero throughout the response read. However, get_burstcount assumes that a response of zero indicates that the TPM is not ready to receive more data. In this case, it returns a negative error code, which is passed on to tpm_tis_{write,read}_bytes as a u16, causing them to read/write far too many bytes.

Quoted 4 lines

Quoted 2 lines

This patch checks for negative return codes and bails out from recv_data and tpm_tis_send_data.

I guess this would need a "Fixes:" tag, wouldn't it?

Ah, yes. Good point.

Quoted 3 lines

I would also add

Cc [email protected]

Will do when I mail the updated patch.

Quoted 4 lines

Quoted 4 lines

---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Quoted 16 lines

Quoted 5 lines

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

index e3bf31b..d0301dc 100644

drivers/char/tpm/tpm_tis_core.c static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) · line 194

Quoted 9 lines

drivers/char/tpm/tpm_tis_core.c +6 -0

@@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
                               chip->timeout_c,
                               &priv->read_queue, true) == 0) {
              burstcnt = min_t(int, get_burstcount(chip), count - size);
+             if (burstcnt < 0) {
+                     dev_err(&chip->dev,
+                             "Unable to read burstcount in %s:%d (%s)\n",
+                             __FILE__, __LINE__, __func__);
+                     return rc;
+             }

"return burstcnt;"

Done, thanks!

Quoted 1 line

I guess __func__ would be enough to deduce the call site?

It is not; would you suggest removing it since it's redundant with __LINE__ and __FILE__? Or is there some other macro you know of that could be useful for determining the call site? (I had included it initially for the sake of ease of log reading, but that's perhaps not very compelling.)

Quoted 41 lines · Josh Zimmerman

Quoted 8 lines · Jarkko Sakkinen

Quoted 7 lines · Josh Zimmerman

If the TPM we're connecting to uses a static burst count, it will report a burst count of zero throughout the response read. However, get_burstcount assumes that a response of zero indicates that the TPM is not ready to receive more data. In this case, it returns a negative error code, which is passed on to tpm_tis_{write,read}_bytes as a u16, causing them to read/write far too many bytes.

Quoted 4 lines

Quoted 2 lines

This patch checks for negative return codes and bails out from recv_data and tpm_tis_send_data.

I guess this would need a "Fixes:" tag, wouldn't it?

Ah, yes. Good point.

Quoted 3 lines

I would also add

Cc [email protected]

Will do when I mail the updated patch.

Quoted 4 lines

Quoted 4 lines

---
 drivers/char/tpm/tpm_tis_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Quoted 16 lines

Quoted 5 lines

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

index e3bf31b..d0301dc 100644

drivers/char/tpm/tpm_tis_core.c static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) · line 194

Quoted 9 lines

drivers/char/tpm/tpm_tis_core.c +6 -0

@@ -186,6 +186,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
                               chip->timeout_c,
                               &priv->read_queue, true) == 0) {
              burstcnt = min_t(int, get_burstcount(chip), count - size);
+             if (burstcnt < 0) {
+                     dev_err(&chip->dev,
+                             "Unable to read burstcount in %s:%d (%s)\n",
+                             __FILE__, __LINE__, __func__);
+                     return rc;
+             }

"return burstcnt;"

Quoted 8 lines

Done, thanks!

Quoted 1 line

I guess __func__ would be enough to deduce the call site?

It is not; would you suggest removing it since it's redundant with __LINE__ and __FILE__? Or is there some other macro you know of that could be useful for determining the call site? (I had included it initially for the sake of ease of log reading, but that's perhaps not very compelling.)

It's an internal function to tpm_tis_core.c and there are exactly two call sites for it. Just by printing __func__ you can determine where it fails.