diff mbox series

[v6,6/7] tpm: ensure that the output of PCR read contains the correct digest size

Message ID 20181204082138.24600-7-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series tpm: retrieve digest size of unknown algorithms from TPM | expand

Commit Message

Roberto Sassu Dec. 4, 2018, 8:21 a.m. UTC 
This patch protects against data corruption that could happen in the bus,
by checking that the digest size returned by the TPM during a PCR read
matches the size of the algorithm passed 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>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Dec. 5, 2018, 12:09 a.m. UTC | #1 
On Tue, Dec 04, 2018 at 09:21:37AM +0100, Roberto Sassu wrote:
>  	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>  	digest_size = be16_to_cpu(out->digest_size);
> -	if (digest_size > sizeof(digest->digest)) {
> +	if (digest_size > sizeof(digest->digest) ||
> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>  		rc = -EINVAL;
>  		goto out;
>  	}

Just noticed this but you must squash 4-6 because applying only
previous commits will result a broken tree. It will be much bigger
commit but won't be broken.

I think you should also feed min_rsp_body_length as you should be
able to precalculate.

Last time I was asking why this isn't a bug fix. It is even for
the existing code. The existing code should have a bug fix that
checks that the received digest size so that it is the expected
SHA1 size before we can apply this commit.

/Jarkko
Jarkko Sakkinen Dec. 5, 2018, 12:46 a.m. UTC | #2 
On Tue, Dec 04, 2018 at 04:09:10PM -0800, Jarkko Sakkinen wrote:
> On Tue, Dec 04, 2018 at 09:21:37AM +0100, Roberto Sassu wrote:
> >  	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
> >  	digest_size = be16_to_cpu(out->digest_size);
> > -	if (digest_size > sizeof(digest->digest)) {
> > +	if (digest_size > sizeof(digest->digest) ||
> > +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
> >  		rc = -EINVAL;
> >  		goto out;
> >  	}
> 
> Just noticed this but you must squash 4-6 because applying only
> previous commits will result a broken tree. It will be much bigger
> commit but won't be broken.
> 
> I think you should also feed min_rsp_body_length as you should be
> able to precalculate.
> 
> Last time I was asking why this isn't a bug fix. It is even for
> the existing code. The existing code should have a bug fix that
> checks that the received digest size so that it is the expected
> SHA1 size before we can apply this commit.

My bad. This is not the same deal as the code because in the old code we
always copy a constant block. Here we use the variable as parameter for
memcpy() so it is better to check the size. You can ignore the last
paragraph completely. Sorry, had to double check this one.

There is no need to do any type of bug fix for the current tree.

Still 4-6 need to be squashed in order to not put purposely the tree
into broken state.

/Jarko
Roberto Sassu Dec. 6, 2018, 6:07 p.m. UTC | #3 
On 12/5/2018 1:46 AM, Jarkko Sakkinen wrote:
> On Tue, Dec 04, 2018 at 04:09:10PM -0800, Jarkko Sakkinen wrote:
>> On Tue, Dec 04, 2018 at 09:21:37AM +0100, Roberto Sassu wrote:
>>>   	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>>>   	digest_size = be16_to_cpu(out->digest_size);
>>> -	if (digest_size > sizeof(digest->digest)) {
>>> +	if (digest_size > sizeof(digest->digest) ||
>>> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>>>   		rc = -EINVAL;
>>>   		goto out;
>>>   	}
>>
>> Just noticed this but you must squash 4-6 because applying only
>> previous commits will result a broken tree. It will be much bigger
>> commit but won't be broken.
>>
>> I think you should also feed min_rsp_body_length as you should be
>> able to precalculate.
>>
>> Last time I was asking why this isn't a bug fix. It is even for
>> the existing code. The existing code should have a bug fix that
>> checks that the received digest size so that it is the expected
>> SHA1 size before we can apply this commit.
> 
> My bad. This is not the same deal as the code because in the old code we
> always copy a constant block. Here we use the variable as parameter for
> memcpy() so it is better to check the size. You can ignore the last
> paragraph completely. Sorry, had to double check this one.
> 
> There is no need to do any type of bug fix for the current tree.
> 
> Still 4-6 need to be squashed in order to not put purposely the tree
> into broken state.

Ok. I keep the description of 5, and add few details from 4 and 6.

Roberto


> /Jarko
>
Jarkko Sakkinen Dec. 12, 2018, 6:18 p.m. UTC | #4 
On Thu, Dec 06, 2018 at 07:07:34PM +0100, Roberto Sassu wrote:
> > Still 4-6 need to be squashed in order to not put purposely the tree
> > into broken state.
> 
> Ok. I keep the description of 5, and add few details from 4 and 6.

Yeah, keeping patches small is one thing, but the commits should not
be crippled. Therefore, I rather have these squashed.

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a804f0b7126a..d6d29480348c 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,15 +179,28 @@  struct tpm2_pcr_read_out {
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		  struct tpm_digest *digest, 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_allocated_banks &&
+		     chip->allocated_banks[i].alg_id != digest->alg_id; i++)
+			;
+
+		if (i == chip->nr_allocated_banks)
+			return -EINVAL;
+
+		expected_digest_size = chip->allocated_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -207,7 +220,8 @@  int tpm2_pcr_read(struct tpm_chip *chip, u32 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->digest)) {
+	if (digest_size > sizeof(digest->digest) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
 		rc = -EINVAL;
 		goto out;
 	}