| Message ID | 20161027174000.GA4432@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > 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. > > Fixes: 1107d065fdf1 (tpm_tis: Introduce intermediate layer for TPM access) > Signed-off-by: Josh Zimmerman <joshz@google.com> > > --- > Changelog v4: > - Add short description to Fixes tag line. > - Remove some unnecessary information in dev_err statements. > Changelog v3: > - Add signed-off-by. > Changelog v2: > - Fix typo (rc->burstcnt) > > --- > drivers/char/tpm/tpm_tis_core.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index e3bf31b..0f2c233 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -186,6 +186,10 @@ 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) { It is much more readable to directly check get_burstcount return value, 'count - size' is protected be above condition to not reach negative value. burstcnt = get_burstcount(chip) if (brustcnt < 0) ... > + dev_err(&chip->dev, "Unable to read burstcount\n"); > + return burstcnt; > + } > > rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality), > burstcnt, buf + size); > @@ -272,6 +276,11 @@ 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) { Same here > + dev_err(&chip->dev, "Unable to read burstcount\n"); > + rc = burstcnt; > + goto out_err; > + } > rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), > burstcnt, buf + count); > if (rc < 0) > -- > 2.8.0.rc3.226.g39d4020 Thanks Tomas ------------------------------------------------------------------------------ The Command Line: Reinvented for Modern Developers Did the resurgence of CLI tooling catch you by surprise? Reconnect with the command line and become more productive. Learn the new .NET and ASP.NET CLI. Get your free copy! http://sdm.link/telerik
Thanks, that's a fair point. Updated patch sent. Josh On Thu, Oct 27, 2016 at 1:31 PM, Winkler, Tomas <tomas.winkler@intel.com> wrote: > > > > 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. > > > > Fixes: 1107d065fdf1 (tpm_tis: Introduce intermediate layer for TPM > access) > > Signed-off-by: Josh Zimmerman <joshz@google.com> > > > > --- > > Changelog v4: > > - Add short description to Fixes tag line. > > - Remove some unnecessary information in dev_err statements. > > Changelog v3: > > - Add signed-off-by. > > Changelog v2: > > - Fix typo (rc->burstcnt) > > > > --- > > drivers/char/tpm/tpm_tis_core.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_ > core.c > > index e3bf31b..0f2c233 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -186,6 +186,10 @@ 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) { > > It is much more readable to directly check get_burstcount return value, > 'count - size' is protected be above condition to not reach negative value. > burstcnt = get_burstcount(chip) > if (brustcnt < 0) ... > > > > + dev_err(&chip->dev, "Unable to read burstcount\n"); > > + return burstcnt; > > + } > > > > rc = tpm_tis_read_bytes(priv, > TPM_DATA_FIFO(priv->locality), > > burstcnt, buf + size); > > @@ -272,6 +276,11 @@ 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) { > Same here > > + dev_err(&chip->dev, "Unable to read burstcount\n"); > > + rc = burstcnt; > > + goto out_err; > > + } > > rc = tpm_tis_write_bytes(priv, > TPM_DATA_FIFO(priv->locality), > > burstcnt, buf + count); > > if (rc < 0) > > -- > > 2.8.0.rc3.226.g39d4020 > > Thanks > Tomas > ------------------------------------------------------------------------------ The Command Line: Reinvented for Modern Developers Did the resurgence of CLI tooling catch you by surprise? Reconnect with the command line and become more productive. Learn the new .NET and ASP.NET CLI. Get your free copy! http://sdm.link/telerik
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index e3bf31b..0f2c233 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -186,6 +186,10 @@ 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\n"); + return burstcnt; + } rc = tpm_tis_read_bytes(priv, TPM_DATA_FIFO(priv->locality), burstcnt, buf + size); @@ -272,6 +276,11 @@ 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\n"); + rc = burstcnt; + goto out_err; + } rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), burstcnt, buf + count); if (rc < 0)
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. Fixes: 1107d065fdf1 (tpm_tis: Introduce intermediate layer for TPM access) Signed-off-by: Josh Zimmerman <joshz@google.com> --- Changelog v4: - Add short description to Fixes tag line. - Remove some unnecessary information in dev_err statements. Changelog v3: - Add signed-off-by. Changelog v2: - Fix typo (rc->burstcnt) --- drivers/char/tpm/tpm_tis_core.c | 9 +++++++++ 1 file changed, 9 insertions(+)