* [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
@ 2017-01-27 15:25 Nayna Jain
2017-01-29 14:40 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Nayna Jain @ 2017-01-27 15:25 UTC (permalink / raw)
To: tpmdd-devel
Cc: peterhuewe, tpmdd, jarkko.sakkinen, jgunthorpe,
linux-security-module, linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
2017-01-27 15:25 [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation() Nayna Jain
@ 2017-01-29 14:40 ` Jarkko Sakkinen
2017-01-29 17:18 ` Nayna
0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2017-01-29 14:40 UTC (permalink / raw)
To: Nayna Jain
Cc: tpmdd-devel, peterhuewe, tpmdd, jgunthorpe,
linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
2017-01-29 14:40 ` Jarkko Sakkinen
@ 2017-01-29 17:18 ` Nayna
2017-01-29 21:20 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Nayna @ 2017-01-29 17:18 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: tpmdd-devel, peterhuewe, tpmdd, jgunthorpe,
linux-security-module, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
2017-01-29 17:18 ` Nayna
@ 2017-01-29 21:20 ` Jarkko Sakkinen
2017-01-30 2:58 ` Nayna
0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2017-01-29 21:20 UTC (permalink / raw)
To: Nayna
Cc: tpmdd-devel, peterhuewe, tpmdd, jgunthorpe,
linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
2017-01-29 21:20 ` Jarkko Sakkinen
@ 2017-01-30 2:58 ` Nayna
2017-01-30 21:47 ` Jarkko Sakkinen
0 siblings, 1 reply; 6+ messages in thread
From: Nayna @ 2017-01-30 2:58 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: tpmdd-devel, peterhuewe, tpmdd, jgunthorpe,
linux-security-module, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation()
2017-01-30 2:58 ` Nayna
@ 2017-01-30 21:47 ` Jarkko Sakkinen
0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2017-01-30 21:47 UTC (permalink / raw)
To: Nayna
Cc: tpmdd-devel, peterhuewe, tpmdd, jgunthorpe,
linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-30 21:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-27 15:25 [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation() Nayna Jain
2017-01-29 14:40 ` Jarkko Sakkinen
2017-01-29 17:18 ` Nayna
2017-01-29 21:20 ` Jarkko Sakkinen
2017-01-30 2:58 ` Nayna
2017-01-30 21:47 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).