Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()

From: Nayna
Date: Sun Jan 29 2017 - 12:20:42 EST




On 01/29/2017 08:10 PM, Jarkko Sakkinen wrote:
On Fri, Jan 27, 2017 at 10:25:49AM -0500, Nayna Jain wrote:
This patch add validation in tpm2_get_pcr_allocation to avoid
access beyond response buffer length.

Suggested-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxxxxxxx>

This validation looks broken to me.

---
drivers/char/tpm/tpm2-cmd.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 4aad84c..02c1ea7 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1008,9 +1008,13 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
struct tpm2_pcr_selection pcr_selection;
struct tpm_buf buf;
void *marker;
- unsigned int count = 0;
+ void *end;
+ void *pcr_select_offset;
+ unsigned int count;
+ u32 sizeof_pcr_selection;
+ u32 resp_len;

Very cosmetic but we almos almost universally use the acronym 'rsp' in
the TPM driver.

Sure will update.


int rc;
- int i;
+ int i = 0;

Why do you need to initialize it?

Because in out: count is replaced with i.
And it is replaced because now for loop can break even before reaching count, because of new buffer checks.


rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
if (rc)
@@ -1034,15 +1038,29 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
}

marker = &buf.data[TPM_HEADER_SIZE + 9];
+
+ resp_len = be32_to_cpup((__be32 *)&buf.data[2]);
+ end = &buf.data[resp_len];

What if the response contains larger length than the buffer size?

Isn't this check need to be done in tpm_transmit_cmd for all responses ?
Though, it seems it is not done there as well.

And to understand what do we expect max buffer length. PAGE_SIZE or TPM_BUFSIZE ?


+
for (i = 0; i < count; i++) {
+ pcr_select_offset = marker +
+ offsetof(struct tpm2_pcr_selection, size_of_select);
+ if (pcr_select_offset >= end) {
+ rc = -EFAULT;
+ break;
+ }
+
memcpy(&pcr_selection, marker, sizeof(pcr_selection));
chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
- marker = marker + sizeof(struct tpm2_pcr_selection);
+ sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
+ sizeof(pcr_selection.size_of_select) +
+ sizeof(u8) * pcr_selection.size_of_select;

Remove "sizeof(u8) * ".

Sure.

+ marker = marker + sizeof_pcr_selection;
}

out:
- if (count < ARRAY_SIZE(chip->active_banks))
- chip->active_banks[count] = TPM2_ALG_ERROR;
+ if (i < ARRAY_SIZE(chip->active_banks))
+ chip->active_banks[i] = TPM2_ALG_ERROR;

tpm_buf_destroy(&buf);

--
2.5.0


I'm sorry but this commit is changing too much. You need to redo the
whole commit and resend the patch set with these fixes. You can keep
Reviewed-by and Tested-by in 1/2 but have to remove them from 2/2.

Sure, will do.

Thanks & Regards,
- Nayna



/Jarkko