| Message ID | 20170525211105.843-1-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote: > struct tpm_chip *chip = to_tpm_chip(dev); > + char anti_replay[20]; > > - tpm_cmd.header.in = tpm_readpubek_header; > - err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, > + rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK); > + if (rc) > + return rc; > + > + /* The checksum is ignored so it doesn't matter what the contents are. > + */ > + tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay)); It does matter, we do not want to leak random kernel memory incase it has something sensitive. Zero anti_replay. > + > - /* > - ignore header 10 bytes > - algorithm 32 bits (1 == RSA ) > - encscheme 16 bits > - sigscheme 16 bits > - parameters (RSA 12->bytes: keybit, #primes, expbit) > - keylenbytes 32 bits > - 256 byte modulus > - ignore checksum 20 bytes > - */ Not sure we should delete the comment, tpm buf does not make the parse any clearer. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote: > On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote: > > struct tpm_chip *chip = to_tpm_chip(dev); > > + char anti_replay[20]; > > > > - tpm_cmd.header.in = tpm_readpubek_header; > > - err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, > > + rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK); > > + if (rc) > > + return rc; > > + > > + /* The checksum is ignored so it doesn't matter what the contents are. > > + */ > > + tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay)); > > It does matter, we do not want to leak random kernel memory incase it > has something sensitive. Zero anti_replay. If there was a leak it has existed before this change as tpm_cmd was also allocated from stack. And there is not leak because the checksum is not printed. > > + > > - /* > > - ignore header 10 bytes > > - algorithm 32 bits (1 == RSA ) > > - encscheme 16 bits > > - sigscheme 16 bits > > - parameters (RSA 12->bytes: keybit, #primes, expbit) > > - keylenbytes 32 bits > > - 256 byte modulus > > - ignore checksum 20 bytes > > - */ > > Not sure we should delete the comment, tpm buf does not make the parse > any clearer. I think better idea would be to move struct tpm_readpubek_params_out declaration here and use it to refer different fields. Previously this has been a complete mess. The structure has been declared but it has not been used for anything. I wonder what is the history here... /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, May 25, 2017 at 03:28:01PM -0700, Jarkko Sakkinen wrote: > On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote: > > On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote: > > > struct tpm_chip *chip = to_tpm_chip(dev); > > > + char anti_replay[20]; > > > > > > - tpm_cmd.header.in = tpm_readpubek_header; > > > - err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, > > > + rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK); > > > + if (rc) > > > + return rc; > > > + > > > + /* The checksum is ignored so it doesn't matter what the contents are. > > > + */ > > > + tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay)); > > > > It does matter, we do not want to leak random kernel memory incase it > > has something sensitive. Zero anti_replay. > > If there was a leak it has existed before this change as tpm_cmd was > also allocated from stack. And there is not leak because the checksum is > not printed. It leaks stack memory to the TPM which is not OK. > I think better idea would be to move struct tpm_readpubek_params_out > declaration here and use it to refer different fields. Previously > this That would be better.. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, May 25, 2017 at 04:40:50PM -0600, Jason Gunthorpe wrote: > On Thu, May 25, 2017 at 03:28:01PM -0700, Jarkko Sakkinen wrote: > > On Thu, May 25, 2017 at 03:16:13PM -0600, Jason Gunthorpe wrote: > > > On Thu, May 25, 2017 at 02:11:04PM -0700, Jarkko Sakkinen wrote: > > > > struct tpm_chip *chip = to_tpm_chip(dev); > > > > + char anti_replay[20]; > > > > > > > > - tpm_cmd.header.in = tpm_readpubek_header; > > > > - err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, > > > > + rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK); > > > > + if (rc) > > > > + return rc; > > > > + > > > > + /* The checksum is ignored so it doesn't matter what the contents are. > > > > + */ > > > > + tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay)); > > > > > > It does matter, we do not want to leak random kernel memory incase it > > > has something sensitive. Zero anti_replay. > > > > If there was a leak it has existed before this change as tpm_cmd was > > also allocated from stack. And there is not leak because the checksum is > > not printed. > > It leaks stack memory to the TPM which is not OK. Right, of course, vtpm_tpm_proxy. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index 4bd0997cfa2d..bb5b2053137d 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -20,43 +20,37 @@ #include <linux/device.h> #include "tpm.h" -#define READ_PUBEK_RESULT_SIZE 314 #define READ_PUBEK_RESULT_MIN_BODY_SIZE (28 + 256) #define TPM_ORD_READPUBEK 124 -static const struct tpm_input_header tpm_readpubek_header = { - .tag = cpu_to_be16(TPM_TAG_RQU_COMMAND), - .length = cpu_to_be32(30), - .ordinal = cpu_to_be32(TPM_ORD_READPUBEK) -}; + static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, char *buf) { + struct tpm_buf tpm_buf; u8 *data; - struct tpm_cmd_t tpm_cmd; - ssize_t err; - int i, rc; + ssize_t rc; + int i; char *str = buf; - struct tpm_chip *chip = to_tpm_chip(dev); + char anti_replay[20]; - tpm_cmd.header.in = tpm_readpubek_header; - err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, + rc = tpm_buf_init(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK); + if (rc) + return rc; + + /* The checksum is ignored so it doesn't matter what the contents are. + */ + tpm_buf_append(&tpm_buf, anti_replay, sizeof(anti_replay)); + + rc = tpm_transmit_cmd(chip, NULL, tpm_buf.data, PAGE_SIZE, READ_PUBEK_RESULT_MIN_BODY_SIZE, 0, "attempting to read the PUBEK"); - if (err) - goto out; - - /* - ignore header 10 bytes - algorithm 32 bits (1 == RSA ) - encscheme 16 bits - sigscheme 16 bits - parameters (RSA 12->bytes: keybit, #primes, expbit) - keylenbytes 32 bits - 256 byte modulus - ignore checksum 20 bytes - */ - data = tpm_cmd.params.readpubek_out_buffer; + if (rc) { + tpm_buf_destroy(&tpm_buf); + return 0; + } + + data = &tpm_buf.data[10]; str += sprintf(str, "Algorithm: %02X %02X %02X %02X\n" @@ -80,8 +74,9 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, if ((i + 1) % 16 == 0) str += sprintf(str, "\n"); } -out: + rc = str - buf; + tpm_buf_destroy(&tpm_buf); return rc; } static DEVICE_ATTR_RO(pubek); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index af05c1403c6e..5b79b7e06937 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -339,17 +339,6 @@ enum tpm_sub_capabilities { TPM_CAP_PROP_TIS_DURATION = 0x120, }; -struct tpm_readpubek_params_out { - u8 algorithm[4]; - u8 encscheme[2]; - u8 sigscheme[2]; - __be32 paramsize; - u8 parameters[12]; /*assuming RSA*/ - __be32 keysize; - u8 modulus[256]; - u8 checksum[20]; -} __packed; - typedef union { struct tpm_input_header in; struct tpm_output_header out; @@ -383,8 +372,6 @@ struct tpm_startup_in { } __packed; typedef union { - struct tpm_readpubek_params_out readpubek_out; - u8 readpubek_out_buffer[sizeof(struct tpm_readpubek_params_out)]; struct tpm_pcrread_in pcrread_in; struct tpm_pcrread_out pcrread_out; struct tpm_getrandom_in getrandom_in;
Migrated pubek_show to struct tpm_buf. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- RFC because I cannot test this ATM. drivers/char/tpm/tpm-sysfs.c | 49 ++++++++++++++++++++------------------------ drivers/char/tpm/tpm.h | 13 ------------ 2 files changed, 22 insertions(+), 40 deletions(-)