| Message ID | 20181106150159.1136-7-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: retrieve digest size of unknown algorithms from TPM | expand |
On Tue, Nov 06, 2018 at 04:01:59PM +0100, Roberto Sassu wrote: > This patch protects against data corruption that could happen in the bus, > by checking that that the digest size returned by the TPM during a PCR read > matches the size of the algorithm passed as argument to tpm2_pcr_read(). > > This check is performed after information about the PCR banks has been > retrieved. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index e2d5b84286a7..3b0b5b032901 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out { > int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, > struct tpm_digest *digest_struct, u16 *digest_size_ptr) > { > + int i; > int rc; > struct tpm_buf buf; > struct tpm2_pcr_read_out *out; > u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; > u16 digest_size; > + u16 expected_digest_size = 0; > > if (pcr_idx >= TPM2_PLATFORM_PCR) > return -EINVAL; > > + if (!digest_size_ptr) { > + for (i = 0; i < chip->nr_active_banks && > + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) > + ; Not sure if the semicolon should be in its own line. ` > + > + if (i == chip->nr_active_banks) > + return -EINVAL; > + > + expected_digest_size = chip->active_banks[i].digest_size; > + } > + > rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); > if (rc) > return rc; > @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, > > out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; > digest_size = be16_to_cpu(out->digest_size); > - if (digest_size > sizeof(digest_struct->digest)) { > + if (digest_size > sizeof(digest_struct->digest) || > + (!digest_size_ptr && digest_size != expected_digest_size)) { > rc = -EINVAL; > goto out; > } > -- > 2.17.1 > Please add Cc: stable@vger.kernel.org. /Jarkko
On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote: > On Tue, Nov 06, 2018 at 04:01:59PM +0100, Roberto Sassu wrote: >> This patch protects against data corruption that could happen in the bus, >> by checking that that the digest size returned by the TPM during a PCR read >> matches the size of the algorithm passed as argument to tpm2_pcr_read(). >> >> This check is performed after information about the PCR banks has been >> retrieved. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> --- >> drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >> index e2d5b84286a7..3b0b5b032901 100644 >> --- a/drivers/char/tpm/tpm2-cmd.c >> +++ b/drivers/char/tpm/tpm2-cmd.c >> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out { >> int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, >> struct tpm_digest *digest_struct, u16 *digest_size_ptr) >> { >> + int i; >> int rc; >> struct tpm_buf buf; >> struct tpm2_pcr_read_out *out; >> u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; >> u16 digest_size; >> + u16 expected_digest_size = 0; >> >> if (pcr_idx >= TPM2_PLATFORM_PCR) >> return -EINVAL; >> >> + if (!digest_size_ptr) { >> + for (i = 0; i < chip->nr_active_banks && >> + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) >> + ; > > Not sure if the semicolon should be in its own line. scripts/Lindent suggests: for (i = 0; i < chip->nr_active_banks && chip->active_banks[i].alg_id != digest_struct->alg_id; i++) ; but this is not accepted by scripts/checkpatch.pl (there are no warnings/errors on the patch I sent). >> + >> + if (i == chip->nr_active_banks) >> + return -EINVAL; >> + >> + expected_digest_size = chip->active_banks[i].digest_size; >> + } >> + >> rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); >> if (rc) >> return rc; >> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, >> >> out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; >> digest_size = be16_to_cpu(out->digest_size); >> - if (digest_size > sizeof(digest_struct->digest)) { >> + if (digest_size > sizeof(digest_struct->digest) || >> + (!digest_size_ptr && digest_size != expected_digest_size)) { >> rc = -EINVAL; >> goto out; >> } >> -- >> 2.17.1 >> > > Please add > > Cc: stable@vger.kernel.org. > > /Jarkko >
On Thu, Nov 08, 2018 at 03:47:39PM +0100, Roberto Sassu wrote: > > > + for (i = 0; i < chip->nr_active_banks && > > > + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) > > > + ; > > > > Not sure if the semicolon should be in its own line. > > scripts/Lindent suggests: > > for (i = 0; i < chip->nr_active_banks && > chip->active_banks[i].alg_id != digest_struct->alg_id; > i++) ; > > but this is not accepted by scripts/checkpatch.pl (there are no > warnings/errors on the patch I sent). Yeah, not a really blocker for me. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote: > On Tue, Nov 06, 2018 at 04:01:59PM +0100, Roberto Sassu wrote: >> This patch protects against data corruption that could happen in the bus, >> by checking that that the digest size returned by the TPM during a PCR read >> matches the size of the algorithm passed as argument to tpm2_pcr_read(). >> >> This check is performed after information about the PCR banks has been >> retrieved. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >> --- >> drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >> index e2d5b84286a7..3b0b5b032901 100644 >> --- a/drivers/char/tpm/tpm2-cmd.c >> +++ b/drivers/char/tpm/tpm2-cmd.c >> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out { >> int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, >> struct tpm_digest *digest_struct, u16 *digest_size_ptr) >> { >> + int i; >> int rc; >> struct tpm_buf buf; >> struct tpm2_pcr_read_out *out; >> u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; >> u16 digest_size; >> + u16 expected_digest_size = 0; >> >> if (pcr_idx >= TPM2_PLATFORM_PCR) >> return -EINVAL; >> >> + if (!digest_size_ptr) { >> + for (i = 0; i < chip->nr_active_banks && >> + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) >> + ; > > Not sure if the semicolon should be in its own line. > ` >> + >> + if (i == chip->nr_active_banks) >> + return -EINVAL; >> + >> + expected_digest_size = chip->active_banks[i].digest_size; >> + } >> + >> rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); >> if (rc) >> return rc; >> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, >> >> out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; >> digest_size = be16_to_cpu(out->digest_size); >> - if (digest_size > sizeof(digest_struct->digest)) { >> + if (digest_size > sizeof(digest_struct->digest) || >> + (!digest_size_ptr && digest_size != expected_digest_size)) { >> rc = -EINVAL; >> goto out; >> } >> -- >> 2.17.1 >> > > Please add > > Cc: stable@vger.kernel.org. Should I do the same for the previous patches? This patch cannot be applied alone. Roberto > /Jarkko >
On Tue, Nov 13, 2018 at 02:08:39PM +0100, Roberto Sassu wrote: > On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote: > > On Tue, Nov 06, 2018 at 04:01:59PM +0100, Roberto Sassu wrote: > > > This patch protects against data corruption that could happen in the bus, > > > by checking that that the digest size returned by the TPM during a PCR read > > > matches the size of the algorithm passed as argument to tpm2_pcr_read(). > > > > > > This check is performed after information about the PCR banks has been > > > retrieved. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > --- > > > drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > > index e2d5b84286a7..3b0b5b032901 100644 > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out { > > > int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, > > > struct tpm_digest *digest_struct, u16 *digest_size_ptr) > > > { > > > + int i; > > > int rc; > > > struct tpm_buf buf; > > > struct tpm2_pcr_read_out *out; > > > u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; > > > u16 digest_size; > > > + u16 expected_digest_size = 0; > > > if (pcr_idx >= TPM2_PLATFORM_PCR) > > > return -EINVAL; > > > + if (!digest_size_ptr) { > > > + for (i = 0; i < chip->nr_active_banks && > > > + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) > > > + ; > > > > Not sure if the semicolon should be in its own line. > > ` > > > + > > > + if (i == chip->nr_active_banks) > > > + return -EINVAL; > > > + > > > + expected_digest_size = chip->active_banks[i].digest_size; > > > + } > > > + > > > rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); > > > if (rc) > > > return rc; > > > @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, > > > out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; > > > digest_size = be16_to_cpu(out->digest_size); > > > - if (digest_size > sizeof(digest_struct->digest)) { > > > + if (digest_size > sizeof(digest_struct->digest) || > > > + (!digest_size_ptr && digest_size != expected_digest_size)) { > > > rc = -EINVAL; > > > goto out; > > > } > > > -- > > > 2.17.1 > > > > > > > Please add > > > > Cc: stable@vger.kernel.org. > > Should I do the same for the previous patches? This patch cannot be > applied alone. > > Roberto No need. It is an issue that we deal with depenendent commits once it is being backported. This could be dependent for example of a commit that is even not in the series so does not make sense to do it now. /Jarkko
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index e2d5b84286a7..3b0b5b032901 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out { int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, struct tpm_digest *digest_struct, u16 *digest_size_ptr) { + int i; int rc; struct tpm_buf buf; struct tpm2_pcr_read_out *out; u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0}; u16 digest_size; + u16 expected_digest_size = 0; if (pcr_idx >= TPM2_PLATFORM_PCR) return -EINVAL; + if (!digest_size_ptr) { + for (i = 0; i < chip->nr_active_banks && + chip->active_banks[i].alg_id != digest_struct->alg_id; i++) + ; + + if (i == chip->nr_active_banks) + return -EINVAL; + + expected_digest_size = chip->active_banks[i].digest_size; + } + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ); if (rc) return rc; @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE]; digest_size = be16_to_cpu(out->digest_size); - if (digest_size > sizeof(digest_struct->digest)) { + if (digest_size > sizeof(digest_struct->digest) || + (!digest_size_ptr && digest_size != expected_digest_size)) { rc = -EINVAL; goto out; }
This patch protects against data corruption that could happen in the bus, by checking that that the digest size returned by the TPM during a PCR read matches the size of the algorithm passed as argument to tpm2_pcr_read(). This check is performed after information about the PCR banks has been retrieved. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)