| Message ID | 1548859550-32019-1-git-send-email-liam.merwick@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | tpm_tis: validate locality values don't overrun array | expand |
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) {
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
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 >
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
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) {
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(+)