Message ID | 20220513163827.26281-1-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | hw/tpm/tpm_tis_common.c: Assert that locty is in range | expand |
On Fri, May 13, 2022 at 6:43 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > In tpm_tis_mmio_read(), tpm_tis_mmio_write() and > tpm_tis_dump_state(), we calculate a locality index with > tpm_tis_locality_from_addr() and then use it as an index into the > s->loc[] array. In all these cases, the array index can't overflow > because the MemoryRegion is sized to be TPM_TIS_NUM_LOCALITIES << > TPM_TIS_LOCALITY_SHIFT bytes. However, Coverity can't see that, and > it complains (CID 1487138, 1487180, 1487188, 1487198, 1487240). > > Add assertions that the calculated locality index is valid, which > will help Coverity and also catch any potential future bug where > the MemoryRegion isn't sized exactly. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Tested with 'make check' only... > > hw/tpm/tpm_tis_common.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c > index e700d821816..81edae410c8 100644 > --- a/hw/tpm/tpm_tis_common.c > +++ b/hw/tpm/tpm_tis_common.c > @@ -295,6 +295,8 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) > uint8_t locty = tpm_tis_locality_from_addr(addr); > hwaddr base = addr & ~0xfff; > > + assert(TPM_TIS_IS_VALID_LOCTY(locty)); > + > printf("tpm_tis: active locality : %d\n" > "tpm_tis: state of locality %d : %d\n" > "tpm_tis: register dump:\n", This one was here ...: https://lore.kernel.org/qemu-devel/20220330235723.68033-1-philippe.mathieu.daude@gmail.com/ > @@ -336,6 +338,8 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > uint32_t avail; > uint8_t v; > > + assert(TPM_TIS_IS_VALID_LOCTY(locty)); > + > if (tpm_backend_had_startup_error(s->be_driver)) { > return 0; > } > @@ -458,6 +462,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) { ... but not these, so: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 5/13/22 12:38, Peter Maydell wrote: > In tpm_tis_mmio_read(), tpm_tis_mmio_write() and > tpm_tis_dump_state(), we calculate a locality index with > tpm_tis_locality_from_addr() and then use it as an index into the > s->loc[] array. In all these cases, the array index can't overflow > because the MemoryRegion is sized to be TPM_TIS_NUM_LOCALITIES << > TPM_TIS_LOCALITY_SHIFT bytes. However, Coverity can't see that, and > it complains (CID 1487138, 1487180, 1487188, 1487198, 1487240). > > Add assertions that the calculated locality index is valid, which > will help Coverity and also catch any potential future bug where > the MemoryRegion isn't sized exactly. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> I trust that the 3 fixes resolve the 5 CIDs. Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > Tested with 'make check' only... > > hw/tpm/tpm_tis_common.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c > index e700d821816..81edae410c8 100644 > --- a/hw/tpm/tpm_tis_common.c > +++ b/hw/tpm/tpm_tis_common.c > @@ -295,6 +295,8 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) > uint8_t locty = tpm_tis_locality_from_addr(addr); > hwaddr base = addr & ~0xfff; > > + assert(TPM_TIS_IS_VALID_LOCTY(locty)); > + > printf("tpm_tis: active locality : %d\n" > "tpm_tis: state of locality %d : %d\n" > "tpm_tis: register dump:\n", > @@ -336,6 +338,8 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, > uint32_t avail; > uint8_t v; > > + assert(TPM_TIS_IS_VALID_LOCTY(locty)); > + > if (tpm_backend_had_startup_error(s->be_driver)) { > return 0; > } > @@ -458,6 +462,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) { All 3 of your fixes below are after the 3 existing calls to tpm_tis_locality_from_addr(). Would Coverity be happy if we were to move the asserts into that one function? I am fine with this patch, though.
On Wed, 18 May 2022 at 14:46, Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 5/13/22 12:38, Peter Maydell wrote: > > In tpm_tis_mmio_read(), tpm_tis_mmio_write() and > > tpm_tis_dump_state(), we calculate a locality index with > > tpm_tis_locality_from_addr() and then use it as an index into the > > s->loc[] array. In all these cases, the array index can't overflow > > because the MemoryRegion is sized to be TPM_TIS_NUM_LOCALITIES << > > TPM_TIS_LOCALITY_SHIFT bytes. However, Coverity can't see that, and > > it complains (CID 1487138, 1487180, 1487188, 1487198, 1487240). > > All 3 of your fixes below are after the 3 existing calls to > tpm_tis_locality_from_addr(). Would Coverity be happy if we were to move > the asserts into that one function? I am fine with this patch, though. Yes, I think Coverity would be happy either way. There's not a lot in it, but I picked this way round because in theory one might want in a hypothetical future situation to have a different kind of error checking for a callsite that did an address-to-locality lookup: it's not inherently of itself never possible it can fail. thanks -- PMM
diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c index e700d821816..81edae410c8 100644 --- a/hw/tpm/tpm_tis_common.c +++ b/hw/tpm/tpm_tis_common.c @@ -295,6 +295,8 @@ static void tpm_tis_dump_state(TPMState *s, hwaddr addr) uint8_t locty = tpm_tis_locality_from_addr(addr); hwaddr base = addr & ~0xfff; + assert(TPM_TIS_IS_VALID_LOCTY(locty)); + printf("tpm_tis: active locality : %d\n" "tpm_tis: state of locality %d : %d\n" "tpm_tis: register dump:\n", @@ -336,6 +338,8 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr, uint32_t avail; uint8_t v; + assert(TPM_TIS_IS_VALID_LOCTY(locty)); + if (tpm_backend_had_startup_error(s->be_driver)) { return 0; } @@ -458,6 +462,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) {
In tpm_tis_mmio_read(), tpm_tis_mmio_write() and tpm_tis_dump_state(), we calculate a locality index with tpm_tis_locality_from_addr() and then use it as an index into the s->loc[] array. In all these cases, the array index can't overflow because the MemoryRegion is sized to be TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT bytes. However, Coverity can't see that, and it complains (CID 1487138, 1487180, 1487188, 1487198, 1487240). Add assertions that the calculated locality index is valid, which will help Coverity and also catch any potential future bug where the MemoryRegion isn't sized exactly. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Tested with 'make check' only... hw/tpm/tpm_tis_common.c | 6 ++++++ 1 file changed, 6 insertions(+)