* Re: [PATCH v5] tpm: Check size of response before accessing data [not found] <1484361394-13581-1-git-send-email-stefanb@linux.vnet.ibm.com> @ 2017-01-16 13:24 ` Jarkko Sakkinen 2017-01-16 13:25 ` Jarkko Sakkinen 2017-01-16 14:46 ` Stefan Berger 0 siblings, 2 replies; 4+ messages in thread From: Jarkko Sakkinen @ 2017-01-16 13:24 UTC (permalink / raw) To: Stefan Berger; +Cc: tpmdd-devel, linux-security-module, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] tpm: Check size of response before accessing data 2017-01-16 13:24 ` [PATCH v5] tpm: Check size of response before accessing data Jarkko Sakkinen @ 2017-01-16 13:25 ` Jarkko Sakkinen 2017-01-16 15:38 ` Stefan Berger 2017-01-16 14:46 ` Stefan Berger 1 sibling, 1 reply; 4+ messages in thread From: Jarkko Sakkinen @ 2017-01-16 13:25 UTC (permalink / raw) To: Stefan Berger; +Cc: tpmdd-devel, linux-security-module, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] tpm: Check size of response before accessing data 2017-01-16 13:25 ` Jarkko Sakkinen @ 2017-01-16 15:38 ` Stefan Berger 0 siblings, 0 replies; 4+ messages in thread From: Stefan Berger @ 2017-01-16 15:38 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: tpmdd-devel, linux-security-module, linux-kernel 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] tpm: Check size of response before accessing data 2017-01-16 13:24 ` [PATCH v5] tpm: Check size of response before accessing data Jarkko Sakkinen 2017-01-16 13:25 ` Jarkko Sakkinen @ 2017-01-16 14:46 ` Stefan Berger 1 sibling, 0 replies; 4+ messages in thread From: Stefan Berger @ 2017-01-16 14:46 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: tpmdd-devel, linux-security-module, linux-kernel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-16 15:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1484361394-13581-1-git-send-email-stefanb@linux.vnet.ibm.com>
2017-01-16 13:24 ` [PATCH v5] tpm: Check size of response before accessing data Jarkko Sakkinen
2017-01-16 13:25 ` Jarkko Sakkinen
2017-01-16 15:38 ` Stefan Berger
2017-01-16 14:46 ` Stefan Berger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®