| Message ID | 1484361394-13581-1-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 13, 2017 at 09:36:34PM -0500, Stefan Berger wrote: > Make sure that we have not received less bytes than what is indicated > in the header of the TPM response. Also, check the number of bytes in > the response before accessing its data. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> There are some things that I want to comment after all but I can give now Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> I also noticed that this patch is not CC'd to linux-kernel. > --- > > v5: > - Fixed a bug related to tpm_getcap() having to allow to return > only a header rather than having to receive a full response > --- > drivers/char/tpm/tpm-interface.c | 64 ++++++++++++++++++++++++--------- > drivers/char/tpm/tpm-sysfs.c | 29 +++++++++------ > drivers/char/tpm/tpm.h | 7 ++-- > drivers/char/tpm/tpm2-cmd.c | 76 +++++++++++++++++++++++++++++----------- > drivers/char/tpm/tpm_tis_core.c | 3 +- > 5 files changed, 128 insertions(+), 51 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index fecdd3f..4da6ef8 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -434,7 +434,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, > * A positive number for a TPM error. > */ > ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, > - int len, unsigned int flags, const char *desc) > + size_t len, size_t min_rx_length, You haven't documented min_rx_length. > + unsigned int flags, const char *desc) > { > const struct tpm_output_header *header; > int err; > @@ -446,6 +447,9 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, > return -EFAULT; > > header = cmd; > + if (len < be32_to_cpu(header->length) || > + be32_to_cpu(header->length) < min_rx_length) > + return -EFAULT; > > err = be32_to_cpu(header->return_code); > if (err != 0 && desc) > @@ -468,7 +472,8 @@ static const struct tpm_input_header tpm_getcap_header = { > }; > > ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > - const char *desc) > + const char *desc, size_t min_cap_length, > + u32 *rlength) > { > struct tpm_cmd_t tpm_cmd; > int rc; > @@ -491,10 +496,12 @@ ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); > tpm_cmd.params.getcap_in.subcap = cpu_to_be32(subcap_id); > } > - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0, > - desc); > + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, > + TPM_HEADER_SIZE + min_cap_length, 0, desc); > if (!rc) > *cap = tpm_cmd.params.getcap_out.cap; > + if (rlength) > + *rlength = be32_to_cpu(tpm_cmd.header.out.length); > return rc; > } > EXPORT_SYMBOL_GPL(tpm_getcap); > @@ -515,7 +522,8 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type) > start_cmd.header.in = tpm_startup_header; > > start_cmd.params.startup_in.startup_type = startup_type; > - return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, 0, > + return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, > + TPM_HEADER_SIZE, 0, > "attempting to start the TPM"); > } > > @@ -525,6 +533,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) > unsigned long new_timeout[4]; > unsigned long old_timeout[4]; > ssize_t rc; > + u32 rlength; > > if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS) > return 0; > @@ -546,7 +555,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) > return 0; > } > > - rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL); > + rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL, > + 0, &rlength); > if (rc == TPM_ERR_INVALID_POSTINIT) { > /* The TPM is not started, we are the first to talk to it. > Execute a startup command. */ > @@ -555,8 +565,12 @@ int tpm_get_timeouts(struct tpm_chip *chip) > return rc; > > rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, > - "attempting to determine the timeouts"); > + "attempting to determine the timeouts", > + 0, &rlength); > } > + if (rlength < TPM_HEADER_SIZE + sizeof(cap.timeout)) > + rc = -EFAULT; > + > if (rc) { > dev_err(&chip->dev, > "A TPM error (%zd) occurred attempting to determine the timeouts\n", > @@ -606,7 +620,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) > chip->timeout_d = usecs_to_jiffies(new_timeout[3]); > > rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap, > - "attempting to determine the durations"); > + "attempting to determine the durations", > + sizeof(cap.duration), NULL); > if (rc) > return rc; > > @@ -657,8 +672,9 @@ static int tpm_continue_selftest(struct tpm_chip *chip) > struct tpm_cmd_t cmd; > > cmd.header.in = continue_selftest_header; > - rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, 0, > - "continue selftest"); > + rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, > + CONTINUE_SELFTEST_RESULT_SIZE, > + 0, "continue selftest"); > return rc; > } > > @@ -677,7 +693,9 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) > > cmd.header.in = pcrread_header; > cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx); > - rc = tpm_transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE, 0, > + rc = tpm_transmit_cmd(chip, &cmd, > + READ_PCR_RESULT_SIZE, > + READ_PCR_RESULT_SIZE, 0, > "attempting to read a pcr value"); > > if (rc == 0) > @@ -775,7 +793,8 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) > cmd.header.in = pcrextend_header; > cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx); > memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE); > - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, > + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, > + EXTEND_PCR_RESULT_SIZE, 0, > "attempting extend a PCR value"); > > tpm_put_ops(chip); > @@ -879,7 +898,8 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen) > if (chip == NULL) > return -ENODEV; > > - rc = tpm_transmit_cmd(chip, cmd, buflen, 0, "attempting tpm_cmd"); > + rc = tpm_transmit_cmd(chip, cmd, buflen, TPM_HEADER_SIZE, > + 0, "attempting tpm_cmd"); > > tpm_put_ops(chip); > return rc; > @@ -981,15 +1001,16 @@ int tpm_pm_suspend(struct device *dev) > cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(tpm_suspend_pcr); > memcpy(cmd.params.pcrextend_in.hash, dummy_hash, > TPM_DIGEST_SIZE); > - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, > + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, > + EXTEND_PCR_RESULT_SIZE, 0, > "extending dummy pcr before suspend"); > } > > /* now do the actual savestate */ > for (try = 0; try < TPM_RETRY; try++) { > cmd.header.in = savestate_header; > - rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, 0, > - NULL); > + rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, > + SAVESTATE_RESULT_SIZE, 0, NULL); > > /* > * If the TPM indicates that it is too busy to respond to > @@ -1051,7 +1072,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) > { > struct tpm_chip *chip; > struct tpm_cmd_t tpm_cmd; > - u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA); > + u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA), rlength; > int err, total = 0, retries = 5; > u8 *dest = out; > > @@ -1074,11 +1095,20 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) > > err = tpm_transmit_cmd(chip, &tpm_cmd, > TPM_GETRANDOM_RESULT_SIZE + num_bytes, > + offsetof(struct tpm_cmd_t, > + params.getrandom_out.rng_data), > 0, "attempting get random"); > if (err) > break; > > recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len); > + > + rlength = be32_to_cpu(tpm_cmd.header.out.length); > + if (rlength < offsetof(struct tpm_cmd_t, > + params.getrandom_out.rng_data) + recd) { > + total = -EFAULT; > + break; > + } > memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd); > > dest += recd; > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > index 848ad65..5e8100c 100644 > --- a/drivers/char/tpm/tpm-sysfs.c > +++ b/drivers/char/tpm/tpm-sysfs.c > @@ -39,8 +39,9 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, > struct tpm_chip *chip = to_tpm_chip(dev); > > tpm_cmd.header.in = tpm_readpubek_header; > - err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, 0, > - "attempting to read the PUBEK"); > + err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, > + TPM_HEADER_SIZE + 28 + 256, > + 0, "attempting to read the PUBEK"); There are bunch of these but this is worse than having just a named constant containing a number. That arithmetic looks horrible. There are many of these. BTW, what is difference between the value stored in READ_PUBEK_RESULT_SIZE and the value you are calculating? > if (err) > goto out; > > @@ -95,7 +96,8 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr, > struct tpm_chip *chip = to_tpm_chip(dev); > > rc = tpm_getcap(chip, TPM_CAP_PROP_PCR, &cap, > - "attempting to determine the number of PCRS"); > + "attempting to determine the number of PCRS", > + sizeof(cap.num_pcrs), NULL); > if (rc) > return 0; > > @@ -120,7 +122,8 @@ static ssize_t enabled_show(struct device *dev, struct device_attribute *attr, > ssize_t rc; > > rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, > - "attempting to determine the permanent enabled state"); > + "attempting to determine the permanent enabled state", > + sizeof(cap.perm_flags), NULL); > if (rc) > return 0; > > @@ -136,7 +139,8 @@ static ssize_t active_show(struct device *dev, struct device_attribute *attr, > ssize_t rc; > > rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, > - "attempting to determine the permanent active state"); > + "attempting to determine the permanent active state", > + sizeof(cap.perm_flags), NULL); > if (rc) > return 0; > > @@ -152,7 +156,8 @@ static ssize_t owned_show(struct device *dev, struct device_attribute *attr, > ssize_t rc; > > rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap, > - "attempting to determine the owner state"); > + "attempting to determine the owner state", > + sizeof(cap.owned), NULL); > if (rc) > return 0; > > @@ -168,7 +173,8 @@ static ssize_t temp_deactivated_show(struct device *dev, > ssize_t rc; > > rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap, > - "attempting to determine the temporary state"); > + "attempting to determine the temporary state", > + sizeof(cap.stclear_flags), NULL); > if (rc) > return 0; > > @@ -186,7 +192,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, > char *str = buf; > > rc = tpm_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap, > - "attempting to determine the manufacturer"); > + "attempting to determine the manufacturer", > + sizeof(cap.manufacturer_id), NULL); > if (rc) > return 0; > str += sprintf(str, "Manufacturer: 0x%x\n", > @@ -194,7 +201,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, > > /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ > rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, > - "attempting to determine the 1.2 version"); > + "attempting to determine the 1.2 version", > + sizeof(cap.tpm_version_1_2), NULL); > if (!rc) { > str += sprintf(str, > "TCG version: %d.%d\nFirmware version: %d.%d\n", > @@ -205,7 +213,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, > } else { > /* Otherwise just use TPM_STRUCT_VER */ > rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, > - "attempting to determine the 1.1 version"); > + "attempting to determine the 1.1 version", > + sizeof(cap.tpm_version), NULL); > if (rc) > return 0; > str += sprintf(str, > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 1ae9768..b7b12498 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -493,10 +493,11 @@ enum tpm_transmit_flags { > > ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, > unsigned int flags); > -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, int len, > - unsigned int flags, const char *desc); > +ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, size_t len, > + size_t min_tx_length, unsigned int flags, > + const char *desc); > ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, > - const char *desc); > + const char *desc, size_t min_cap_length, u32 *rlength); > int tpm_get_timeouts(struct tpm_chip *); > int tpm1_auto_startup(struct tpm_chip *chip); > int tpm_do_selftest(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 6eda239..9e8256b 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -248,6 +248,10 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { > (sizeof(struct tpm_input_header) + \ > sizeof(struct tpm2_pcr_read_in)) > > +#define TPM2_PCR_READ_OUT_SIZE \ > + (sizeof(struct tpm_output_header) + \ > + sizeof(struct tpm2_pcr_read_out)) Another example where this commit goes wrong. Would be better just to have #define TPM2_PCR_READ_OUT_SIZE <number>. We anyway want to drop those special structures eventually (eg tpm2_pcr_read_out) so lets not bind more stuff to them. > + > static const struct tpm_input_header tpm2_pcrread_header = { > .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > .length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE), > @@ -280,8 +284,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) > sizeof(cmd.params.pcrread_in.pcr_select)); > cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7); > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, > - "attempting to read a pcr value"); > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM2_PCR_READ_OUT_SIZE, > + 0, "attempting to read a pcr value"); > if (rc == 0) { > buf = cmd.params.pcrread_out.digest; > memcpy(res_buf, buf, TPM_DIGEST_SIZE); > @@ -327,7 +331,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash) > cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1); > memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE); > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, 0, > "attempting extend a PCR value"); > > return rc; > @@ -356,7 +360,7 @@ static const struct tpm_input_header tpm2_getrandom_header = { > int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > { > struct tpm2_cmd cmd; > - u32 recd; > + u32 recd, rlength; > u32 num_bytes; > int err; > int total = 0; > @@ -373,13 +377,19 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > cmd.header.in = tpm2_getrandom_header; > cmd.params.getrandom_in.size = cpu_to_be16(num_bytes); > > - err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, > - "attempting get random"); > + err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), > + offsetof(struct tpm2_cmd, > + params.getrandom_out.buffer), > + 0, "attempting get random"); > if (err) > break; > > recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size), > num_bytes); > + rlength = be32_to_cpu(cmd.header.out.length); > + if (rlength < offsetof(struct tpm2_cmd, > + params.getrandom_out.buffer) + recd) > + return -EFAULT; > memcpy(dest, cmd.params.getrandom_out.buffer, recd); > > dest += recd; > @@ -394,6 +404,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > (sizeof(struct tpm_input_header) + \ > sizeof(struct tpm2_get_tpm_pt_in)) > > +#define TPM2_GET_TPM_PT_OUT_SIZE \ > + (sizeof(struct tpm_output_header) + \ > + sizeof(struct tpm2_get_tpm_pt_out)) > + > static const struct tpm_input_header tpm2_get_tpm_pt_header = { > .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), > @@ -445,7 +459,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > { > unsigned int blob_len; > struct tpm_buf buf; > - u32 hash; > + u32 hash, rlength; > int i; > int rc; > > @@ -510,7 +524,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > goto out; > } > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, "sealing data"); > + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, > + TPM_HEADER_SIZE + 4, > + 0, "sealing data"); > if (rc) > goto out; > > @@ -519,6 +535,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > rc = -E2BIG; > goto out; > } > + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf)->header.out.length); > + if (rlength < TPM_HEADER_SIZE + 4 + blob_len) { > + rc = -EFAULT; > + goto out; > + } > > memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len); > payload->blob_len = blob_len; > @@ -588,7 +609,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip, > goto out; > } > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "loading blob"); > + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, > + TPM_HEADER_SIZE + 4, flags, "loading blob"); > if (!rc) > *blob_handle = be32_to_cpup( > (__be32 *) &buf.data[TPM_HEADER_SIZE]); > @@ -626,8 +648,8 @@ static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, > > tpm_buf_append_u32(&buf, handle); > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, > - "flushing context"); > + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, TPM_HEADER_SIZE, > + flags, "flushing context"); > if (rc) > dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle, > rc); > @@ -657,6 +679,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, > u16 data_len; > u8 *data; > int rc; > + u32 rlength; > > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL); > if (rc) > @@ -671,13 +694,21 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, > options->blobauth /* hmac */, > TPM_DIGEST_SIZE); > > - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "unsealing"); > + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, > + TPM_HEADER_SIZE + 4 + 2, flags, "unsealing"); > if (rc > 0) > rc = -EPERM; > > if (!rc) { > data_len = be16_to_cpup( > (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]); > + > + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf) > + ->header.out.length); > + if (rlength < TPM_HEADER_SIZE + 4 + 2 + data_len) { > + rc = -EFAULT; > + goto out; > + } > data = &buf.data[TPM_HEADER_SIZE + 6]; > > memcpy(payload->key, data, data_len - 1); > @@ -685,6 +716,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, > payload->migratable = data[data_len - 1]; > } > > +out: > tpm_buf_destroy(&buf); > return rc; > } > @@ -739,7 +771,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id); > cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc); > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), > + TPM2_GET_TPM_PT_OUT_SIZE, 0, desc); > if (!rc) > *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value); > > @@ -773,8 +806,8 @@ static int tpm2_startup(struct tpm_chip *chip, u16 startup_type) > cmd.header.in = tpm2_startup_header; > > cmd.params.startup_in.startup_type = cpu_to_be16(startup_type); > - return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, > - "attempting to start the TPM"); > + return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, > + 0, "attempting to start the TPM"); > } > > #define TPM2_SHUTDOWN_IN_SIZE \ > @@ -802,7 +835,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type) > cmd.header.in = tpm2_shutdown_header; > cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type); > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, "stopping the TPM"); > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, > + 0, "stopping the TPM"); > > /* In places where shutdown command is sent there's no much we can do > * except print the error code on a system failure. > @@ -865,8 +899,8 @@ static int tpm2_start_selftest(struct tpm_chip *chip, bool full) > cmd.header.in = tpm2_selftest_header; > cmd.params.selftest_in.full_test = full; > > - rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, 0, > - "continue selftest"); > + rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, > + TPM_HEADER_SIZE, 0, "continue selftest"); > > /* At least some prototype chips seem to give RC_TESTING error > * immediately. This is a workaround for that. > @@ -916,7 +950,8 @@ static int tpm2_do_selftest(struct tpm_chip *chip) > cmd.params.pcrread_in.pcr_select[1] = 0x00; > cmd.params.pcrread_in.pcr_select[2] = 0x00; > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, > + 0, NULL); > if (rc < 0) > break; > > @@ -949,7 +984,8 @@ int tpm2_probe(struct tpm_chip *chip) > cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100); > cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); > > - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); > + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, > + 0, NULL); > if (rc < 0) > return rc; > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 7993678..025ddb5 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -552,7 +552,8 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); > else > - return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc); > + return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, > + 0, NULL); > } > > /* Register the IRQ and issue a command that will cause an interrupt. If an > -- > 2.4.3 > /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Mon, Jan 16, 2017 at 03:24:09PM +0200, Jarkko Sakkinen wrote: > On Fri, Jan 13, 2017 at 09:36:34PM -0500, Stefan Berger wrote: > > Make sure that we have not received less bytes than what is indicated > > in the header of the TPM response. Also, check the number of bytes in > > the response before accessing its data. > > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > There are some things that I want to comment after all but I can give > now > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > I also noticed that this patch is not CC'd to linux-kernel. Please go through the whole patch and remove arithmetic from every possible place where you only end up with a constant. /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On 01/16/2017 08:24 AM, Jarkko Sakkinen wrote: > On Fri, Jan 13, 2017 at 09:36:34PM -0500, Stefan Berger wrote: >> Make sure that we have not received less bytes than what is indicated >> in the header of the TPM response. Also, check the number of bytes in >> the response before accessing its data. >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > There are some things that I want to comment after all but I can give > now > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > I also noticed that this patch is not CC'd to linux-kernel. > >> --- >> >> v5: >> - Fixed a bug related to tpm_getcap() having to allow to return >> only a header rather than having to receive a full response >> --- >> drivers/char/tpm/tpm-interface.c | 64 ++++++++++++++++++++++++--------- >> drivers/char/tpm/tpm-sysfs.c | 29 +++++++++------ >> drivers/char/tpm/tpm.h | 7 ++-- >> drivers/char/tpm/tpm2-cmd.c | 76 +++++++++++++++++++++++++++++----------- >> drivers/char/tpm/tpm_tis_core.c | 3 +- >> 5 files changed, 128 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c >> index fecdd3f..4da6ef8 100644 >> --- a/drivers/char/tpm/tpm-interface.c >> +++ b/drivers/char/tpm/tpm-interface.c >> @@ -434,7 +434,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, >> * A positive number for a TPM error. >> */ >> ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, >> - int len, unsigned int flags, const char *desc) >> + size_t len, size_t min_rx_length, > You haven't documented min_rx_length. > >> + unsigned int flags, const char *desc) >> { >> const struct tpm_output_header *header; >> int err; >> @@ -446,6 +447,9 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, >> return -EFAULT; >> >> header = cmd; >> + if (len < be32_to_cpu(header->length) || >> + be32_to_cpu(header->length) < min_rx_length) >> + return -EFAULT; >> >> err = be32_to_cpu(header->return_code); >> if (err != 0 && desc) >> @@ -468,7 +472,8 @@ static const struct tpm_input_header tpm_getcap_header = { >> }; >> >> ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >> - const char *desc) >> + const char *desc, size_t min_cap_length, >> + u32 *rlength) >> { >> struct tpm_cmd_t tpm_cmd; >> int rc; >> @@ -491,10 +496,12 @@ ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >> tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); >> tpm_cmd.params.getcap_in.subcap = cpu_to_be32(subcap_id); >> } >> - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0, >> - desc); >> + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, >> + TPM_HEADER_SIZE + min_cap_length, 0, desc); >> if (!rc) >> *cap = tpm_cmd.params.getcap_out.cap; >> + if (rlength) >> + *rlength = be32_to_cpu(tpm_cmd.header.out.length); >> return rc; >> } >> EXPORT_SYMBOL_GPL(tpm_getcap); >> @@ -515,7 +522,8 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type) >> start_cmd.header.in = tpm_startup_header; >> >> start_cmd.params.startup_in.startup_type = startup_type; >> - return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, 0, >> + return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, >> + TPM_HEADER_SIZE, 0, >> "attempting to start the TPM"); >> } >> >> @@ -525,6 +533,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) >> unsigned long new_timeout[4]; >> unsigned long old_timeout[4]; >> ssize_t rc; >> + u32 rlength; >> >> if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS) >> return 0; >> @@ -546,7 +555,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) >> return 0; >> } >> >> - rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL); >> + rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL, >> + 0, &rlength); >> if (rc == TPM_ERR_INVALID_POSTINIT) { >> /* The TPM is not started, we are the first to talk to it. >> Execute a startup command. */ >> @@ -555,8 +565,12 @@ int tpm_get_timeouts(struct tpm_chip *chip) >> return rc; >> >> rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, >> - "attempting to determine the timeouts"); >> + "attempting to determine the timeouts", >> + 0, &rlength); >> } >> + if (rlength < TPM_HEADER_SIZE + sizeof(cap.timeout)) >> + rc = -EFAULT; >> + >> if (rc) { >> dev_err(&chip->dev, >> "A TPM error (%zd) occurred attempting to determine the timeouts\n", >> @@ -606,7 +620,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) >> chip->timeout_d = usecs_to_jiffies(new_timeout[3]); >> >> rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap, >> - "attempting to determine the durations"); >> + "attempting to determine the durations", >> + sizeof(cap.duration), NULL); >> if (rc) >> return rc; >> >> @@ -657,8 +672,9 @@ static int tpm_continue_selftest(struct tpm_chip *chip) >> struct tpm_cmd_t cmd; >> >> cmd.header.in = continue_selftest_header; >> - rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, 0, >> - "continue selftest"); >> + rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, >> + CONTINUE_SELFTEST_RESULT_SIZE, >> + 0, "continue selftest"); >> return rc; >> } >> >> @@ -677,7 +693,9 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) >> >> cmd.header.in = pcrread_header; >> cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx); >> - rc = tpm_transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE, 0, >> + rc = tpm_transmit_cmd(chip, &cmd, >> + READ_PCR_RESULT_SIZE, >> + READ_PCR_RESULT_SIZE, 0, >> "attempting to read a pcr value"); >> >> if (rc == 0) >> @@ -775,7 +793,8 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) >> cmd.header.in = pcrextend_header; >> cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx); >> memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE); >> - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, >> + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, >> + EXTEND_PCR_RESULT_SIZE, 0, >> "attempting extend a PCR value"); >> >> tpm_put_ops(chip); >> @@ -879,7 +898,8 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen) >> if (chip == NULL) >> return -ENODEV; >> >> - rc = tpm_transmit_cmd(chip, cmd, buflen, 0, "attempting tpm_cmd"); >> + rc = tpm_transmit_cmd(chip, cmd, buflen, TPM_HEADER_SIZE, >> + 0, "attempting tpm_cmd"); >> >> tpm_put_ops(chip); >> return rc; >> @@ -981,15 +1001,16 @@ int tpm_pm_suspend(struct device *dev) >> cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(tpm_suspend_pcr); >> memcpy(cmd.params.pcrextend_in.hash, dummy_hash, >> TPM_DIGEST_SIZE); >> - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, >> + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, >> + EXTEND_PCR_RESULT_SIZE, 0, >> "extending dummy pcr before suspend"); >> } >> >> /* now do the actual savestate */ >> for (try = 0; try < TPM_RETRY; try++) { >> cmd.header.in = savestate_header; >> - rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, 0, >> - NULL); >> + rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, >> + SAVESTATE_RESULT_SIZE, 0, NULL); >> >> /* >> * If the TPM indicates that it is too busy to respond to >> @@ -1051,7 +1072,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) >> { >> struct tpm_chip *chip; >> struct tpm_cmd_t tpm_cmd; >> - u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA); >> + u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA), rlength; >> int err, total = 0, retries = 5; >> u8 *dest = out; >> >> @@ -1074,11 +1095,20 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) >> >> err = tpm_transmit_cmd(chip, &tpm_cmd, >> TPM_GETRANDOM_RESULT_SIZE + num_bytes, >> + offsetof(struct tpm_cmd_t, >> + params.getrandom_out.rng_data), >> 0, "attempting get random"); >> if (err) >> break; >> >> recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len); >> + >> + rlength = be32_to_cpu(tpm_cmd.header.out.length); >> + if (rlength < offsetof(struct tpm_cmd_t, >> + params.getrandom_out.rng_data) + recd) { >> + total = -EFAULT; >> + break; >> + } >> memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd); >> >> dest += recd; >> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c >> index 848ad65..5e8100c 100644 >> --- a/drivers/char/tpm/tpm-sysfs.c >> +++ b/drivers/char/tpm/tpm-sysfs.c >> @@ -39,8 +39,9 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, >> struct tpm_chip *chip = to_tpm_chip(dev); >> >> tpm_cmd.header.in = tpm_readpubek_header; >> - err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, 0, >> - "attempting to read the PUBEK"); >> + err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, >> + TPM_HEADER_SIZE + 28 + 256, >> + 0, "attempting to read the PUBEK"); > There are bunch of these but this is worse than having just > a named constant containing a number. That arithmetic looks > horrible. 76 for (i = 0; i < 256; i++) { 77 str += sprintf(str, "%02X ", data[i + 28]); 78 if ((i + 1) % 16 == 0) 79 str += sprintf(str, "\n"); 80 } Look at the following. It ends up with 28 + 256 matching exactly the stuff above. > > There are many of these. BTW, what is difference between the > value stored in READ_PUBEK_RESULT_SIZE and the value you are > calculating? > > >> if (err) >> goto out; >> >> @@ -95,7 +96,8 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr, >> struct tpm_chip *chip = to_tpm_chip(dev); >> >> rc = tpm_getcap(chip, TPM_CAP_PROP_PCR, &cap, >> - "attempting to determine the number of PCRS"); >> + "attempting to determine the number of PCRS", >> + sizeof(cap.num_pcrs), NULL); >> if (rc) >> return 0; >> >> @@ -120,7 +122,8 @@ static ssize_t enabled_show(struct device *dev, struct device_attribute *attr, >> ssize_t rc; >> >> rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, >> - "attempting to determine the permanent enabled state"); >> + "attempting to determine the permanent enabled state", >> + sizeof(cap.perm_flags), NULL); >> if (rc) >> return 0; >> >> @@ -136,7 +139,8 @@ static ssize_t active_show(struct device *dev, struct device_attribute *attr, >> ssize_t rc; >> >> rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, >> - "attempting to determine the permanent active state"); >> + "attempting to determine the permanent active state", >> + sizeof(cap.perm_flags), NULL); >> if (rc) >> return 0; >> >> @@ -152,7 +156,8 @@ static ssize_t owned_show(struct device *dev, struct device_attribute *attr, >> ssize_t rc; >> >> rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap, >> - "attempting to determine the owner state"); >> + "attempting to determine the owner state", >> + sizeof(cap.owned), NULL); >> if (rc) >> return 0; >> >> @@ -168,7 +173,8 @@ static ssize_t temp_deactivated_show(struct device *dev, >> ssize_t rc; >> >> rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap, >> - "attempting to determine the temporary state"); >> + "attempting to determine the temporary state", >> + sizeof(cap.stclear_flags), NULL); >> if (rc) >> return 0; >> >> @@ -186,7 +192,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, >> char *str = buf; >> >> rc = tpm_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap, >> - "attempting to determine the manufacturer"); >> + "attempting to determine the manufacturer", >> + sizeof(cap.manufacturer_id), NULL); >> if (rc) >> return 0; >> str += sprintf(str, "Manufacturer: 0x%x\n", >> @@ -194,7 +201,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, >> >> /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ >> rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, >> - "attempting to determine the 1.2 version"); >> + "attempting to determine the 1.2 version", >> + sizeof(cap.tpm_version_1_2), NULL); >> if (!rc) { >> str += sprintf(str, >> "TCG version: %d.%d\nFirmware version: %d.%d\n", >> @@ -205,7 +213,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, >> } else { >> /* Otherwise just use TPM_STRUCT_VER */ >> rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, >> - "attempting to determine the 1.1 version"); >> + "attempting to determine the 1.1 version", >> + sizeof(cap.tpm_version), NULL); >> if (rc) >> return 0; >> str += sprintf(str, >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >> index 1ae9768..b7b12498 100644 >> --- a/drivers/char/tpm/tpm.h >> +++ b/drivers/char/tpm/tpm.h >> @@ -493,10 +493,11 @@ enum tpm_transmit_flags { >> >> ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, >> unsigned int flags); >> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, int len, >> - unsigned int flags, const char *desc); >> +ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, size_t len, >> + size_t min_tx_length, unsigned int flags, >> + const char *desc); >> ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, >> - const char *desc); >> + const char *desc, size_t min_cap_length, u32 *rlength); >> int tpm_get_timeouts(struct tpm_chip *); >> int tpm1_auto_startup(struct tpm_chip *chip); >> int tpm_do_selftest(struct tpm_chip *chip); >> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >> index 6eda239..9e8256b 100644 >> --- a/drivers/char/tpm/tpm2-cmd.c >> +++ b/drivers/char/tpm/tpm2-cmd.c >> @@ -248,6 +248,10 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { >> (sizeof(struct tpm_input_header) + \ >> sizeof(struct tpm2_pcr_read_in)) >> >> +#define TPM2_PCR_READ_OUT_SIZE \ >> + (sizeof(struct tpm_output_header) + \ >> + sizeof(struct tpm2_pcr_read_out)) > Another example where this commit goes wrong. Would be better > just to have > > #define TPM2_PCR_READ_OUT_SIZE <number>. We anyway want to > drop those special structures eventually (eg tpm2_pcr_read_out) > so lets not bind more stuff to them. Look at the code right above it. It follows a pattern in this case. > >> + >> static const struct tpm_input_header tpm2_pcrread_header = { >> .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >> .length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE), >> @@ -280,8 +284,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) >> sizeof(cmd.params.pcrread_in.pcr_select)); >> cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7); >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, >> - "attempting to read a pcr value"); >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM2_PCR_READ_OUT_SIZE, >> + 0, "attempting to read a pcr value"); >> if (rc == 0) { >> buf = cmd.params.pcrread_out.digest; >> memcpy(res_buf, buf, TPM_DIGEST_SIZE); >> @@ -327,7 +331,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash) >> cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1); >> memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE); >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, 0, >> "attempting extend a PCR value"); >> >> return rc; >> @@ -356,7 +360,7 @@ static const struct tpm_input_header tpm2_getrandom_header = { >> int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) >> { >> struct tpm2_cmd cmd; >> - u32 recd; >> + u32 recd, rlength; >> u32 num_bytes; >> int err; >> int total = 0; >> @@ -373,13 +377,19 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) >> cmd.header.in = tpm2_getrandom_header; >> cmd.params.getrandom_in.size = cpu_to_be16(num_bytes); >> >> - err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, >> - "attempting get random"); >> + err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), >> + offsetof(struct tpm2_cmd, >> + params.getrandom_out.buffer), >> + 0, "attempting get random"); >> if (err) >> break; >> >> recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size), >> num_bytes); >> + rlength = be32_to_cpu(cmd.header.out.length); >> + if (rlength < offsetof(struct tpm2_cmd, >> + params.getrandom_out.buffer) + recd) >> + return -EFAULT; >> memcpy(dest, cmd.params.getrandom_out.buffer, recd); >> >> dest += recd; >> @@ -394,6 +404,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) >> (sizeof(struct tpm_input_header) + \ >> sizeof(struct tpm2_get_tpm_pt_in)) >> >> +#define TPM2_GET_TPM_PT_OUT_SIZE \ >> + (sizeof(struct tpm_output_header) + \ >> + sizeof(struct tpm2_get_tpm_pt_out)) >> + >> static const struct tpm_input_header tpm2_get_tpm_pt_header = { >> .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >> .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), >> @@ -445,7 +459,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> { >> unsigned int blob_len; >> struct tpm_buf buf; >> - u32 hash; >> + u32 hash, rlength; >> int i; >> int rc; >> >> @@ -510,7 +524,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> goto out; >> } >> >> - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, "sealing data"); >> + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, >> + TPM_HEADER_SIZE + 4, >> + 0, "sealing data"); >> if (rc) >> goto out; >> >> @@ -519,6 +535,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >> rc = -E2BIG; >> goto out; >> } >> + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf)->header.out.length); >> + if (rlength < TPM_HEADER_SIZE + 4 + blob_len) { >> + rc = -EFAULT; >> + goto out; >> + } >> >> memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len); >> payload->blob_len = blob_len; >> @@ -588,7 +609,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip, >> goto out; >> } >> >> - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "loading blob"); >> + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, >> + TPM_HEADER_SIZE + 4, flags, "loading blob"); >> if (!rc) >> *blob_handle = be32_to_cpup( >> (__be32 *) &buf.data[TPM_HEADER_SIZE]); >> @@ -626,8 +648,8 @@ static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, >> >> tpm_buf_append_u32(&buf, handle); >> >> - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, >> - "flushing context"); >> + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, TPM_HEADER_SIZE, >> + flags, "flushing context"); >> if (rc) >> dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle, >> rc); >> @@ -657,6 +679,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> u16 data_len; >> u8 *data; >> int rc; >> + u32 rlength; >> >> rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL); >> if (rc) >> @@ -671,13 +694,21 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> options->blobauth /* hmac */, >> TPM_DIGEST_SIZE); >> >> - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "unsealing"); >> + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, >> + TPM_HEADER_SIZE + 4 + 2, flags, "unsealing"); >> if (rc > 0) >> rc = -EPERM; >> >> if (!rc) { >> data_len = be16_to_cpup( >> (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]); >> + >> + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf) >> + ->header.out.length); >> + if (rlength < TPM_HEADER_SIZE + 4 + 2 + data_len) { >> + rc = -EFAULT; >> + goto out; >> + } >> data = &buf.data[TPM_HEADER_SIZE + 6]; >> >> memcpy(payload->key, data, data_len - 1); >> @@ -685,6 +716,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, >> payload->migratable = data[data_len - 1]; >> } >> >> +out: >> tpm_buf_destroy(&buf); >> return rc; >> } >> @@ -739,7 +771,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, >> cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id); >> cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc); >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), >> + TPM2_GET_TPM_PT_OUT_SIZE, 0, desc); >> if (!rc) >> *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value); >> >> @@ -773,8 +806,8 @@ static int tpm2_startup(struct tpm_chip *chip, u16 startup_type) >> cmd.header.in = tpm2_startup_header; >> >> cmd.params.startup_in.startup_type = cpu_to_be16(startup_type); >> - return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, >> - "attempting to start the TPM"); >> + return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, >> + 0, "attempting to start the TPM"); >> } >> >> #define TPM2_SHUTDOWN_IN_SIZE \ >> @@ -802,7 +835,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type) >> cmd.header.in = tpm2_shutdown_header; >> cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type); >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, "stopping the TPM"); >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, >> + 0, "stopping the TPM"); >> >> /* In places where shutdown command is sent there's no much we can do >> * except print the error code on a system failure. >> @@ -865,8 +899,8 @@ static int tpm2_start_selftest(struct tpm_chip *chip, bool full) >> cmd.header.in = tpm2_selftest_header; >> cmd.params.selftest_in.full_test = full; >> >> - rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, 0, >> - "continue selftest"); >> + rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, >> + TPM_HEADER_SIZE, 0, "continue selftest"); >> >> /* At least some prototype chips seem to give RC_TESTING error >> * immediately. This is a workaround for that. >> @@ -916,7 +950,8 @@ static int tpm2_do_selftest(struct tpm_chip *chip) >> cmd.params.pcrread_in.pcr_select[1] = 0x00; >> cmd.params.pcrread_in.pcr_select[2] = 0x00; >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, >> + 0, NULL); >> if (rc < 0) >> break; >> >> @@ -949,7 +984,8 @@ int tpm2_probe(struct tpm_chip *chip) >> cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100); >> cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); >> >> - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); >> + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, >> + 0, NULL); >> if (rc < 0) >> return rc; >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index 7993678..025ddb5 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -552,7 +552,8 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) >> if (chip->flags & TPM_CHIP_FLAG_TPM2) >> return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); >> else >> - return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc); >> + return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, >> + 0, NULL); >> } >> >> /* Register the IRQ and issue a command that will cause an interrupt. If an >> -- >> 2.4.3 >> > /Jarkko > ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On 01/16/2017 08:25 AM, Jarkko Sakkinen wrote: > On Mon, Jan 16, 2017 at 03:24:09PM +0200, Jarkko Sakkinen wrote: >> On Fri, Jan 13, 2017 at 09:36:34PM -0500, Stefan Berger wrote: >>> Make sure that we have not received less bytes than what is indicated >>> in the header of the TPM response. Also, check the number of bytes in >>> the response before accessing its data. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> There are some things that I want to comment after all but I can give >> now >> >> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >> >> I also noticed that this patch is not CC'd to linux-kernel. > Please go through the whole patch and remove arithmetic > from every possible place where you only end up with a > constant. sizeof(cap.timeout) -- what do you want to do about that? Is that legitimate or do you want a number then??? What about offsetof's? ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Fri, Jan 13, 2017 at 09:36:34PM -0500, Stefan Berger wrote: > header = cmd; > + if (len < be32_to_cpu(header->length) || > + be32_to_cpu(header->length) < min_rx_length) > + return -EFAULT; > err = be32_to_cpu(header->return_code); > if (err != 0 && desc) Your earlier message points out the problem with this order, it seems like we want a valid TPM error code to take precedence over a short message. Isn't this better than hacking up things that check for POSTINIT? if (len < be32_to_cpu(header->length)) return -EFAULT; err = be32_to_cpu(header->return_code); if (err != 0) [..] if (be32_to_cpu(header->length) < min_rx_length) return -EFAULT; ? Jason ------------------------------------------------------------------------------ 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-interface.c b/drivers/char/tpm/tpm-interface.c index fecdd3f..4da6ef8 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -434,7 +434,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, * A positive number for a TPM error. */ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, - int len, unsigned int flags, const char *desc) + size_t len, size_t min_rx_length, + unsigned int flags, const char *desc) { const struct tpm_output_header *header; int err; @@ -446,6 +447,9 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, return -EFAULT; header = cmd; + if (len < be32_to_cpu(header->length) || + be32_to_cpu(header->length) < min_rx_length) + return -EFAULT; err = be32_to_cpu(header->return_code); if (err != 0 && desc) @@ -468,7 +472,8 @@ static const struct tpm_input_header tpm_getcap_header = { }; ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, - const char *desc) + const char *desc, size_t min_cap_length, + u32 *rlength) { struct tpm_cmd_t tpm_cmd; int rc; @@ -491,10 +496,12 @@ ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); tpm_cmd.params.getcap_in.subcap = cpu_to_be32(subcap_id); } - rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0, - desc); + rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, + TPM_HEADER_SIZE + min_cap_length, 0, desc); if (!rc) *cap = tpm_cmd.params.getcap_out.cap; + if (rlength) + *rlength = be32_to_cpu(tpm_cmd.header.out.length); return rc; } EXPORT_SYMBOL_GPL(tpm_getcap); @@ -515,7 +522,8 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type) start_cmd.header.in = tpm_startup_header; start_cmd.params.startup_in.startup_type = startup_type; - return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, 0, + return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, + TPM_HEADER_SIZE, 0, "attempting to start the TPM"); } @@ -525,6 +533,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) unsigned long new_timeout[4]; unsigned long old_timeout[4]; ssize_t rc; + u32 rlength; if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS) return 0; @@ -546,7 +555,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) return 0; } - rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL); + rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL, + 0, &rlength); if (rc == TPM_ERR_INVALID_POSTINIT) { /* The TPM is not started, we are the first to talk to it. Execute a startup command. */ @@ -555,8 +565,12 @@ int tpm_get_timeouts(struct tpm_chip *chip) return rc; rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, - "attempting to determine the timeouts"); + "attempting to determine the timeouts", + 0, &rlength); } + if (rlength < TPM_HEADER_SIZE + sizeof(cap.timeout)) + rc = -EFAULT; + if (rc) { dev_err(&chip->dev, "A TPM error (%zd) occurred attempting to determine the timeouts\n", @@ -606,7 +620,8 @@ int tpm_get_timeouts(struct tpm_chip *chip) chip->timeout_d = usecs_to_jiffies(new_timeout[3]); rc = tpm_getcap(chip, TPM_CAP_PROP_TIS_DURATION, &cap, - "attempting to determine the durations"); + "attempting to determine the durations", + sizeof(cap.duration), NULL); if (rc) return rc; @@ -657,8 +672,9 @@ static int tpm_continue_selftest(struct tpm_chip *chip) struct tpm_cmd_t cmd; cmd.header.in = continue_selftest_header; - rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, 0, - "continue selftest"); + rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, + CONTINUE_SELFTEST_RESULT_SIZE, + 0, "continue selftest"); return rc; } @@ -677,7 +693,9 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) cmd.header.in = pcrread_header; cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx); - rc = tpm_transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE, 0, + rc = tpm_transmit_cmd(chip, &cmd, + READ_PCR_RESULT_SIZE, + READ_PCR_RESULT_SIZE, 0, "attempting to read a pcr value"); if (rc == 0) @@ -775,7 +793,8 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) cmd.header.in = pcrextend_header; cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx); memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE); - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, + EXTEND_PCR_RESULT_SIZE, 0, "attempting extend a PCR value"); tpm_put_ops(chip); @@ -879,7 +898,8 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen) if (chip == NULL) return -ENODEV; - rc = tpm_transmit_cmd(chip, cmd, buflen, 0, "attempting tpm_cmd"); + rc = tpm_transmit_cmd(chip, cmd, buflen, TPM_HEADER_SIZE, + 0, "attempting tpm_cmd"); tpm_put_ops(chip); return rc; @@ -981,15 +1001,16 @@ int tpm_pm_suspend(struct device *dev) cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(tpm_suspend_pcr); memcpy(cmd.params.pcrextend_in.hash, dummy_hash, TPM_DIGEST_SIZE); - rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0, + rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, + EXTEND_PCR_RESULT_SIZE, 0, "extending dummy pcr before suspend"); } /* now do the actual savestate */ for (try = 0; try < TPM_RETRY; try++) { cmd.header.in = savestate_header; - rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, 0, - NULL); + rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, + SAVESTATE_RESULT_SIZE, 0, NULL); /* * If the TPM indicates that it is too busy to respond to @@ -1051,7 +1072,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) { struct tpm_chip *chip; struct tpm_cmd_t tpm_cmd; - u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA); + u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA), rlength; int err, total = 0, retries = 5; u8 *dest = out; @@ -1074,11 +1095,20 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) err = tpm_transmit_cmd(chip, &tpm_cmd, TPM_GETRANDOM_RESULT_SIZE + num_bytes, + offsetof(struct tpm_cmd_t, + params.getrandom_out.rng_data), 0, "attempting get random"); if (err) break; recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len); + + rlength = be32_to_cpu(tpm_cmd.header.out.length); + if (rlength < offsetof(struct tpm_cmd_t, + params.getrandom_out.rng_data) + recd) { + total = -EFAULT; + break; + } memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd); dest += recd; diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index 848ad65..5e8100c 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -39,8 +39,9 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, struct tpm_chip *chip = to_tpm_chip(dev); tpm_cmd.header.in = tpm_readpubek_header; - err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, 0, - "attempting to read the PUBEK"); + err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, + TPM_HEADER_SIZE + 28 + 256, + 0, "attempting to read the PUBEK"); if (err) goto out; @@ -95,7 +96,8 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr, struct tpm_chip *chip = to_tpm_chip(dev); rc = tpm_getcap(chip, TPM_CAP_PROP_PCR, &cap, - "attempting to determine the number of PCRS"); + "attempting to determine the number of PCRS", + sizeof(cap.num_pcrs), NULL); if (rc) return 0; @@ -120,7 +122,8 @@ static ssize_t enabled_show(struct device *dev, struct device_attribute *attr, ssize_t rc; rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, - "attempting to determine the permanent enabled state"); + "attempting to determine the permanent enabled state", + sizeof(cap.perm_flags), NULL); if (rc) return 0; @@ -136,7 +139,8 @@ static ssize_t active_show(struct device *dev, struct device_attribute *attr, ssize_t rc; rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, - "attempting to determine the permanent active state"); + "attempting to determine the permanent active state", + sizeof(cap.perm_flags), NULL); if (rc) return 0; @@ -152,7 +156,8 @@ static ssize_t owned_show(struct device *dev, struct device_attribute *attr, ssize_t rc; rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap, - "attempting to determine the owner state"); + "attempting to determine the owner state", + sizeof(cap.owned), NULL); if (rc) return 0; @@ -168,7 +173,8 @@ static ssize_t temp_deactivated_show(struct device *dev, ssize_t rc; rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap, - "attempting to determine the temporary state"); + "attempting to determine the temporary state", + sizeof(cap.stclear_flags), NULL); if (rc) return 0; @@ -186,7 +192,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, char *str = buf; rc = tpm_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap, - "attempting to determine the manufacturer"); + "attempting to determine the manufacturer", + sizeof(cap.manufacturer_id), NULL); if (rc) return 0; str += sprintf(str, "Manufacturer: 0x%x\n", @@ -194,7 +201,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, - "attempting to determine the 1.2 version"); + "attempting to determine the 1.2 version", + sizeof(cap.tpm_version_1_2), NULL); if (!rc) { str += sprintf(str, "TCG version: %d.%d\nFirmware version: %d.%d\n", @@ -205,7 +213,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, } else { /* Otherwise just use TPM_STRUCT_VER */ rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, - "attempting to determine the 1.1 version"); + "attempting to determine the 1.1 version", + sizeof(cap.tpm_version), NULL); if (rc) return 0; str += sprintf(str, diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 1ae9768..b7b12498 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -493,10 +493,11 @@ enum tpm_transmit_flags { ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, unsigned int flags); -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, int len, - unsigned int flags, const char *desc); +ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, size_t len, + size_t min_tx_length, unsigned int flags, + const char *desc); ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, - const char *desc); + const char *desc, size_t min_cap_length, u32 *rlength); int tpm_get_timeouts(struct tpm_chip *); int tpm1_auto_startup(struct tpm_chip *chip); int tpm_do_selftest(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 6eda239..9e8256b 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -248,6 +248,10 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { (sizeof(struct tpm_input_header) + \ sizeof(struct tpm2_pcr_read_in)) +#define TPM2_PCR_READ_OUT_SIZE \ + (sizeof(struct tpm_output_header) + \ + sizeof(struct tpm2_pcr_read_out)) + static const struct tpm_input_header tpm2_pcrread_header = { .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), .length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE), @@ -280,8 +284,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf) sizeof(cmd.params.pcrread_in.pcr_select)); cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7); - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, - "attempting to read a pcr value"); + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM2_PCR_READ_OUT_SIZE, + 0, "attempting to read a pcr value"); if (rc == 0) { buf = cmd.params.pcrread_out.digest; memcpy(res_buf, buf, TPM_DIGEST_SIZE); @@ -327,7 +331,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash) cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1); memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE); - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, 0, "attempting extend a PCR value"); return rc; @@ -356,7 +360,7 @@ static const struct tpm_input_header tpm2_getrandom_header = { int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) { struct tpm2_cmd cmd; - u32 recd; + u32 recd, rlength; u32 num_bytes; int err; int total = 0; @@ -373,13 +377,19 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) cmd.header.in = tpm2_getrandom_header; cmd.params.getrandom_in.size = cpu_to_be16(num_bytes); - err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, - "attempting get random"); + err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), + offsetof(struct tpm2_cmd, + params.getrandom_out.buffer), + 0, "attempting get random"); if (err) break; recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size), num_bytes); + rlength = be32_to_cpu(cmd.header.out.length); + if (rlength < offsetof(struct tpm2_cmd, + params.getrandom_out.buffer) + recd) + return -EFAULT; memcpy(dest, cmd.params.getrandom_out.buffer, recd); dest += recd; @@ -394,6 +404,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) (sizeof(struct tpm_input_header) + \ sizeof(struct tpm2_get_tpm_pt_in)) +#define TPM2_GET_TPM_PT_OUT_SIZE \ + (sizeof(struct tpm_output_header) + \ + sizeof(struct tpm2_get_tpm_pt_out)) + static const struct tpm_input_header tpm2_get_tpm_pt_header = { .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), @@ -445,7 +459,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, { unsigned int blob_len; struct tpm_buf buf; - u32 hash; + u32 hash, rlength; int i; int rc; @@ -510,7 +524,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip, goto out; } - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, "sealing data"); + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, + TPM_HEADER_SIZE + 4, + 0, "sealing data"); if (rc) goto out; @@ -519,6 +535,11 @@ int tpm2_seal_trusted(struct tpm_chip *chip, rc = -E2BIG; goto out; } + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf)->header.out.length); + if (rlength < TPM_HEADER_SIZE + 4 + blob_len) { + rc = -EFAULT; + goto out; + } memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len); payload->blob_len = blob_len; @@ -588,7 +609,8 @@ static int tpm2_load_cmd(struct tpm_chip *chip, goto out; } - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "loading blob"); + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, + TPM_HEADER_SIZE + 4, flags, "loading blob"); if (!rc) *blob_handle = be32_to_cpup( (__be32 *) &buf.data[TPM_HEADER_SIZE]); @@ -626,8 +648,8 @@ static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, tpm_buf_append_u32(&buf, handle); - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, - "flushing context"); + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, TPM_HEADER_SIZE, + flags, "flushing context"); if (rc) dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle, rc); @@ -657,6 +679,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, u16 data_len; u8 *data; int rc; + u32 rlength; rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL); if (rc) @@ -671,13 +694,21 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, options->blobauth /* hmac */, TPM_DIGEST_SIZE); - rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "unsealing"); + rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, + TPM_HEADER_SIZE + 4 + 2, flags, "unsealing"); if (rc > 0) rc = -EPERM; if (!rc) { data_len = be16_to_cpup( (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]); + + rlength = be32_to_cpu(((struct tpm2_cmd *)&buf) + ->header.out.length); + if (rlength < TPM_HEADER_SIZE + 4 + 2 + data_len) { + rc = -EFAULT; + goto out; + } data = &buf.data[TPM_HEADER_SIZE + 6]; memcpy(payload->key, data, data_len - 1); @@ -685,6 +716,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip, payload->migratable = data[data_len - 1]; } +out: tpm_buf_destroy(&buf); return rc; } @@ -739,7 +771,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id); cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc); + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), + TPM2_GET_TPM_PT_OUT_SIZE, 0, desc); if (!rc) *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value); @@ -773,8 +806,8 @@ static int tpm2_startup(struct tpm_chip *chip, u16 startup_type) cmd.header.in = tpm2_startup_header; cmd.params.startup_in.startup_type = cpu_to_be16(startup_type); - return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, - "attempting to start the TPM"); + return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, + 0, "attempting to start the TPM"); } #define TPM2_SHUTDOWN_IN_SIZE \ @@ -802,7 +835,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type) cmd.header.in = tpm2_shutdown_header; cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type); - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, "stopping the TPM"); + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, + 0, "stopping the TPM"); /* In places where shutdown command is sent there's no much we can do * except print the error code on a system failure. @@ -865,8 +899,8 @@ static int tpm2_start_selftest(struct tpm_chip *chip, bool full) cmd.header.in = tpm2_selftest_header; cmd.params.selftest_in.full_test = full; - rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, 0, - "continue selftest"); + rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, + TPM_HEADER_SIZE, 0, "continue selftest"); /* At least some prototype chips seem to give RC_TESTING error * immediately. This is a workaround for that. @@ -916,7 +950,8 @@ static int tpm2_do_selftest(struct tpm_chip *chip) cmd.params.pcrread_in.pcr_select[1] = 0x00; cmd.params.pcrread_in.pcr_select[2] = 0x00; - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, + 0, NULL); if (rc < 0) break; @@ -949,7 +984,8 @@ int tpm2_probe(struct tpm_chip *chip) cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100); cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); - rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); + rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), TPM_HEADER_SIZE, + 0, NULL); if (rc < 0) return rc; diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 7993678..025ddb5 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -552,7 +552,8 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) if (chip->flags & TPM_CHIP_FLAG_TPM2) return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); else - return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc); + return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, + 0, NULL); } /* Register the IRQ and issue a command that will cause an interrupt. If an
Make sure that we have not received less bytes than what is indicated in the header of the TPM response. Also, check the number of bytes in the response before accessing its data. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- v5: - Fixed a bug related to tpm_getcap() having to allow to return only a header rather than having to receive a full response --- drivers/char/tpm/tpm-interface.c | 64 ++++++++++++++++++++++++--------- drivers/char/tpm/tpm-sysfs.c | 29 +++++++++------ drivers/char/tpm/tpm.h | 7 ++-- drivers/char/tpm/tpm2-cmd.c | 76 +++++++++++++++++++++++++++++----------- drivers/char/tpm/tpm_tis_core.c | 3 +- 5 files changed, 128 insertions(+), 51 deletions(-)