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

[Stefan Berger <stefanb@linux.ibm.com>]

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
>