| Message ID | 1549897385-10091-2-git-send-email-liam.merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] tpm_tis: fix loop that cancels any seizure by a lower locality | expand |
On 2/11/19 10:03 AM, Liam Merwick wrote: > Defensive check to prevent future caller passing incorrect address > or catch if the MMIO address parameters were not all changed together. > > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > --- > > I've been running static analysis tools on QEMU and one reports this check. > While it's just theoretically correct (impossible to hit with current code), > fixing this helps minimise noise and find other issues using those static > analyzers as well as defending against the addition of future bugs. > > hw/tpm/tpm_tis.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c > index 61a130beef35..860c2ace7d99 100644 > --- a/hw/tpm/tpm_tis.c > +++ b/hw/tpm/tpm_tis.c > @@ -100,6 +100,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > > static uint8_t tpm_tis_locality_from_addr(hwaddr addr) > { > + assert(addr < TPM_TIS_ADDR_SIZE); > return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7); > } > If we want to add a check then we should check the value of the locality that's being derived from the address here since that one serves as an index into the array. However, we shouldn't need it with the way we register the amount of MMIO memory derived from the number of localities and the way we then derive the locality from the address. diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c index 61a130beef..797f107246 100644 --- a/hw/tpm/tpm_tis.c +++ b/hw/tpm/tpm_tis.c @@ -100,7 +100,10 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, static uint8_t tpm_tis_locality_from_addr(hwaddr addr) { - return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7); + uint8_t locty = (uint8_t)(addr >> TPM_TIS_LOCALITY_SHIFT); + fprintf(stderr, "addr=%08lx locty=%u\n", addr, locty); + assert(TPM_TIS_IS_VALID_LOCTY(locty)); + return locty; } static void tpm_tis_show_buffer(const unsigned char *buffer,
diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c index 61a130beef35..860c2ace7d99 100644 --- a/hw/tpm/tpm_tis.c +++ b/hw/tpm/tpm_tis.c @@ -100,6 +100,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, static uint8_t tpm_tis_locality_from_addr(hwaddr addr) { + assert(addr < TPM_TIS_ADDR_SIZE); return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7); }
Defensive check to prevent future caller passing incorrect address or catch if the MMIO address parameters were not all changed together. Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> --- I've been running static analysis tools on QEMU and one reports this check. While it's just theoretically correct (impossible to hit with current code), fixing this helps minimise noise and find other issues using those static analyzers as well as defending against the addition of future bugs. hw/tpm/tpm_tis.c | 1 + 1 file changed, 1 insertion(+)