[Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Liam Merwick]

Assert that various locality values don't exceed TPM_TIS_NUM_LOCALITIES
by adding specific calls to assert(TPM_TIS_NUM_LOCALITIES(l)) in order
to help static code analysis.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/tpm/tpm_tis.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index fd6bb9b59a96..2267bd8c346f 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -241,6 +241,9 @@ static void tpm_tis_abort(TPMState *s)
 {
     s->rw_offset = 0;
 
+    assert(TPM_TIS_IS_VALID_LOCTY(s->next_locty));
+    assert(TPM_TIS_IS_VALID_LOCTY(s->aborting_locty));
+
     trace_tpm_tis_abort(s->next_locty);
 
     /*
@@ -531,6 +534,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
     uint16_t len;
     uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
 
+    assert(TPM_TIS_IS_VALID_LOCTY(locty));
+
     trace_tpm_tis_mmio_write(size, addr, val);
 
     if (locty == 4) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Stefan Berger]

On 1/30/19 9:45 AM, Liam Merwick wrote:
> Assert that various locality values don't exceed TPM_TIS_NUM_LOCALITIES
> by adding specific calls to assert(TPM_TIS_NUM_LOCALITIES(l)) in order
> to help static code analysis.
>
> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>   hw/tpm/tpm_tis.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index fd6bb9b59a96..2267bd8c346f 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -241,6 +241,9 @@ static void tpm_tis_abort(TPMState *s)
>   {
>       s->rw_offset = 0;
>   
> +    assert(TPM_TIS_IS_VALID_LOCTY(s->next_locty));

There are 2 callers of tpm_tis_abort: tpm_tis_prep_abort and 
tpm_tis_request_completed. Both are checking s->next_locality already. 
The former in line 270

     assert(TPM_TIS_IS_VALID_LOCTY(newlocty));

     s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */
     s->next_locty = newlocty;  /* locality after successful abort */

and the latter in line 321:

     if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
         tpm_tis_abort(s);
     }

So this check would be redundant.


> +    assert(TPM_TIS_IS_VALID_LOCTY(s->aborting_locty));
> +

s->aborting_locty only serves as an index into the array in 
tpm_tis_abort and only under the condition

     if (s->aborting_locty == s->next_locty) {

As state above s->next_locty has been tested for being a valid locality 
already elsewhere and we don't need to test it another time. They way 
the code accesses this variable we do not need this check, either. 
Besides that there is this comment in the code in line 272, which would 
make this assert wrong.


     s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */





>       trace_tpm_tis_abort(s->next_locty);
>   
>       /*
> @@ -531,6 +534,8 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>       uint16_t len;
>       uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
>   
> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));
> +

We also do not need this check here since we are registering 0x5000 
bytes of MMIO space, which gives us addresses [0x0..0x4fff], from which 
we calculate the locality with a '>> 12':

static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
{
     return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
}

this is where we register the MMIO memory:

     memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
                           s, "tpm-tis-mmio",
                           TPM_TIS_NUM_LOCALITIES << 
TPM_TIS_LOCALITY_SHIFT);

The locality cannot be out-of-bounds.

   Stefan


>       trace_tpm_tis_mmio_write(size, addr, val);
>   
>       if (locty == 4) {

Re: [Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Liam Merwick]

Hi Stefan,

Thanks for the detailed explanations.

On 04/02/2019 18:05, Stefan Berger wrote:
> On 1/30/19 9:45 AM, Liam Merwick wrote:
>> Assert that various locality values don't exceed TPM_TIS_NUM_LOCALITIES
>> by adding specific calls to assert(TPM_TIS_NUM_LOCALITIES(l)) in order
>> to help static code analysis.
>>
>> Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
>> ---
>>   hw/tpm/tpm_tis.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index fd6bb9b59a96..2267bd8c346f 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -241,6 +241,9 @@ static void tpm_tis_abort(TPMState *s)
>>   {
>>       s->rw_offset = 0;
>> +    assert(TPM_TIS_IS_VALID_LOCTY(s->next_locty));
> 
> There are 2 callers of tpm_tis_abort: tpm_tis_prep_abort and 
> tpm_tis_request_completed. Both are checking s->next_locality already. 
> The former in line 270
> 
>      assert(TPM_TIS_IS_VALID_LOCTY(newlocty));
> 
>      s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */
>      s->next_locty = newlocty;  /* locality after successful abort */
> 
> and the latter in line 321:
> 
>      if (TPM_TIS_IS_VALID_LOCTY(s->next_locty)) {
>          tpm_tis_abort(s);
>      }
> 
> So this check would be redundant.


Agreed.


> 
> 
>> +    assert(TPM_TIS_IS_VALID_LOCTY(s->aborting_locty));
>> +
> 
> s->aborting_locty only serves as an index into the array in 
> tpm_tis_abort and only under the condition
> 
>      if (s->aborting_locty == s->next_locty) {
> 
> As state above s->next_locty has been tested for being a valid locality 
> already elsewhere and we don't need to test it another time. They way 
> the code accesses this variable we do not need this check, either. 
> Besides that there is this comment in the code in line 272, which would 
> make this assert wrong.
> 
> 
>      s->aborting_locty = locty; /* may also be TPM_TIS_NO_LOCALITY */
> 
> 

Indeed.

> 
> 
> 
>>       trace_tpm_tis_abort(s->next_locty);
>>       /*
>> @@ -531,6 +534,8 @@ static void tpm_tis_mmio_write(void *opaque, 
>> hwaddr addr,
>>       uint16_t len;
>>       uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
>> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));
>> +
> 
> We also do not need this check here since we are registering 0x5000 
> bytes of MMIO space, which gives us addresses [0x0..0x4fff], from which 
> we calculate the locality with a '>> 12':
> 
> static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
> {

In that case would it be good to add this check to enforce the address 
range?

assert(addr < TPM_TIS_ADDR_SIZE);


>      return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
> }
> 
> this is where we register the MMIO memory:
> 
>      memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
>                            s, "tpm-tis-mmio",
>                            TPM_TIS_NUM_LOCALITIES << 
> TPM_TIS_LOCALITY_SHIFT);
> 
> The locality cannot be out-of-bounds.




 From staring at the code, I do have one other question - why does the 
check of the lower localities below only check 'l < locty - 1' before 
setting s->loc[locty] - it seems like s->loc[locty - 1] is skipped.


  627                 /* cancel any seize by a lower locality */
  628                 for (l = 0; l < locty - 1; l++) {
  629                     s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
  630                 }
  631
  632                 s->loc[locty].access |= TPM_TIS_ACCESS_SEIZE;


Regards,
Liam

Re: [Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Stefan Berger]

On 2/8/19 3:10 PM, Liam Merwick wrote:
>
>
>>
>>
>>
>>> trace_tpm_tis_abort(s->next_locty);
>>>       /*
>>> @@ -531,6 +534,8 @@ static void tpm_tis_mmio_write(void *opaque, 
>>> hwaddr addr,
>>>       uint16_t len;
>>>       uint32_t mask = (size == 1) ? 0xff : ((size == 2) ? 0xffff : ~0);
>>> +    assert(TPM_TIS_IS_VALID_LOCTY(locty));
>>> +
>>
>> We also do not need this check here since we are registering 0x5000 
>> bytes of MMIO space, which gives us addresses [0x0..0x4fff], from 
>> which we calculate the locality with a '>> 12':
>>
>> static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
>> {
>
> In that case would it be good to add this check to enforce the address 
> range?
>
> assert(addr < TPM_TIS_ADDR_SIZE);


There would be something fundamentally wrong in how the dispatching of 
MMIO addresses works in QEMU. I don't think we should need it...


>
>
>>      return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
>> }
>>
>> this is where we register the MMIO memory:
>>
>>      memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
>>                            s, "tpm-tis-mmio",
>>                            TPM_TIS_NUM_LOCALITIES << 
>> TPM_TIS_LOCALITY_SHIFT);
>>
>> The locality cannot be out-of-bounds.
>
>
>
>
> From staring at the code, I do have one other question - why does the 
> check of the lower localities below only check 'l < locty - 1' before 
> setting s->loc[locty] - it seems like s->loc[locty - 1] is skipped.
>
>
>  627                 /* cancel any seize by a lower locality */
>  628                 for (l = 0; l < locty - 1; l++) {
>  629                     s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
>  630                 }


Uuuh. The loop is clearing the SEIZE flag on localities lower than the 
current one. This works fine for locty >= 1, but not for locty = 0. I 
think there's a bug here.



>  631
>  632                 s->loc[locty].access |= TPM_TIS_ACCESS_SEIZE;
>
>
> Regards,
> Liam
>

Re: [Qemu-devel] [PATCH] tpm_tis: validate locality values don't overrun array

[Stefan Berger]

On 2/8/19 3:38 PM, Stefan Berger wrote:
> On 2/8/19 3:10 PM, Liam Merwick wrote:
>>
>>
>>
>> From staring at the code, I do have one other question - why does the 
>> check of the lower localities below only check 'l < locty - 1' before 
>> setting s->loc[locty] - it seems like s->loc[locty - 1] is skipped.
>>
>>
>>  627                 /* cancel any seize by a lower locality */
>>  628                 for (l = 0; l < locty - 1; l++) {
>>  629                     s->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
>>  630                 }
>
>
> Uuuh. The loop is clearing the SEIZE flag on localities lower than the 
> current one. This works fine for locty >= 1, but not for locty = 0. I 
> think there's a bug here.


Actually, the compiler is smarter than that. With locty = 0, and l = 0,  
l < locty - 1 evaluates to 'false' and the loop doesn't get executed! 
Lucky me.

    Stefan