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


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