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 <address@hidden>
> ---
>   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) {