[PATCH] hw/tpm/tpm_tis_common.c: Assert that locty is in range

Peter Maydell posted 1 patch 2 days, 18 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220513163827.26281-1-peter.maydell@linaro.org
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>
hw/tpm/tpm_tis_common.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH] hw/tpm/tpm_tis_common.c: Assert that locty is in range
Posted by Peter Maydell 2 days, 18 hours ago
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",
@@ -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) {
-- 
2.25.1
Re: [PATCH] hw/tpm/tpm_tis_common.c: Assert that locty is in range
Posted by Philippe Mathieu-Daudé via 2 days, 18 hours ago
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>