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

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

[Nayna Jain]

This patch add validation in tpm2_get_pcr_allocation to avoid
access beyond response buffer length.

Suggested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 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;
 	int rc;
-	int i;
+	int i = 0;
 
 	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];
+
 	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;
+		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

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

[Jarkko Sakkinen]

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@linux.vnet.ibm.com>
> Signed-off-by: Nayna Jain <nayna@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.

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

Why do you need to initialize it?

>  
>  	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?

> +
>  	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) * ".

> +		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.

/Jarkko

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

[Nayna]

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@linux.vnet.ibm.com>
>> Signed-off-by: Nayna Jain <nayna@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
>

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

[Jarkko Sakkinen]

On Sun, Jan 29, 2017 at 10:48:39PM +0530, Nayna wrote:
> 
> 
> 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@linux.vnet.ibm.com>
> > > Signed-off-by: Nayna Jain <nayna@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 ?

Oops. You are correct it is done there:

if (len != be32_to_cpu(header->length))
	return -EFAULT;

So need to do this.

/Jarkko

/Jarkko

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

[Nayna]

On 01/30/2017 02:50 AM, Jarkko Sakkinen wrote:
> On Sun, Jan 29, 2017 at 10:48:39PM +0530, Nayna wrote:
>>
>>
>> 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@linux.vnet.ibm.com>
>>>> Signed-off-by: Nayna Jain <nayna@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 ?
>
> Oops. You are correct it is done there:
>
> if (len != be32_to_cpu(header->length))
> 	return -EFAULT;
>
> So need to do this.

To be sure, means nothing need to be done in this. Right ?

And guess this was the only thing you meant by broken for this patch.

I will do other two smaller changes as I send the whole new patchset.

Thanks & Regards,
   - Nayna

>
> /Jarkko
>
> /Jarkko
>

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

[Jarkko Sakkinen]

On Mon, Jan 30, 2017 at 08:28:30AM +0530, Nayna wrote:
> 
> 
> On 01/30/2017 02:50 AM, Jarkko Sakkinen wrote:
> > On Sun, Jan 29, 2017 at 10:48:39PM +0530, Nayna wrote:
> > > 
> > > 
> > > 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@linux.vnet.ibm.com>
> > > > > Signed-off-by: Nayna Jain <nayna@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 ?
> > 
> > Oops. You are correct it is done there:
> > 
> > if (len != be32_to_cpu(header->length))
> > 	return -EFAULT;
> > 
> > So need to do this.
> 
> To be sure, means nothing need to be done in this. Right ?

This is correct.

> And guess this was the only thing you meant by broken for this patch.
> 
> I will do other two smaller changes as I send the whole new patchset.
> 
> Thanks & Regards,
>   - Nayna

/Jarkko