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