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

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

Posted by Peter Maydell 1 year, 11 months 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 Stefan Berger 1 year, 11 months ago


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.

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

Posted by Peter Maydell 1 year, 11 months ago

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

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

Posted by Philippe Mathieu-Daudé via 1 year, 11 months 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>