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 <stef...@linux.vnet.ibm.com>
>> Signed-off-by: Nayna Jain <na...@linux.vnet.ibm.com>
>
> 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
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
