[Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address

[Eric Auger]

Coverity points out that this can overflow if n > 31,
because it's only doing 32-bit arithmetic. Let's use 1ULL instead
of 1. Also the formulae used to compute n can be replaced by
the level_shift() macro.

Reported-by: Peter Maydell <***@linaro.org>
Signed-off-by: Eric Auger <***@redhat.com>
---
hw/arm/smmu-common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 01c7be8..3c5f724 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
static inline hwaddr get_block_pte_address(uint64_t pte, int level,
int granule_sz, uint64_t *bsz)
{
- int n = (granule_sz - 3) * (4 - level) + 3;
+ int n = level_shift(level, granule_sz);

- *bsz = 1 << n;
+ *bsz = 1ULL << n;
return PTE_ADDRESS(pte, n);
}

[Eric Auger]

Coverity complains about use of uninitialized Evt struct.
The EVT_SET_TYPE and similar setters use deposit32() on fields
in the struct, so they read the uninitialized existing values.
In cases where we don't set all the fields in the event struct
we'll end up leaking random uninitialized data from QEMU's
stack into the guest.

Initializing the struct with "Evt evt = {};" ought to satisfy
Coverity and fix the data leak.

Signed-off-by: Eric Auger <***@redhat.com>
Reported-by: Peter Maydell <***@linaro.org>
---
hw/arm/smmuv3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index b3026de..42dc521 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)

void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
{
- Evt evt;
+ Evt evt = {};
MemTxResult r;

if (!smmuv3_eventq_enabled(s)) {

[Philippe Mathieu-Daudé]

Post by Eric Auger
Coverity complains about use of uninitialized Evt struct.
The EVT_SET_TYPE and similar setters use deposit32() on fields
in the struct, so they read the uninitialized existing values.
In cases where we don't set all the fields in the event struct
we'll end up leaking random uninitialized data from QEMU's
stack into the guest.
Initializing the struct with "Evt evt = {};" ought to satisfy
Coverity and fix the data leak.
---
hw/arm/smmuv3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index b3026de..42dc521 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt)
void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
{
- Evt evt;
+ Evt evt = {};
MemTxResult r;
if (!smmuv3_eventq_enabled(s)) {

[Peter Maydell]

Hi Eric,

Post by Eric Auger
Coverity points out that this can overflow if n > 31,
because it's only doing 32-bit arithmetic. Let's use 1ULL instead
of 1. Also the formulae used to compute n can be replaced by
the level_shift() macro.

This level_shift() replacement doesn't seems that obvious to me, can you
split it in another patch?

Post by Eric Auger
---
hw/arm/smmu-common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 01c7be8..3c5f724 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz)
static inline hwaddr get_block_pte_address(uint64_t pte, int level,
int granule_sz, uint64_t *bsz)
{
- int n = (granule_sz - 3) * (4 - level) + 3;
+ int n = level_shift(level, granule_sz);

Shouldn't this be level_shift(level + 1, granule_sz)?

No. The two expressions are equivalent, they're
just arranged differently:

level_shift(lvl, gsz)
== gsz + (3 - lvl) * (gsz - 3)
== gsz + (4 - lvl) * (gsz - 3) - (gsz - 3)
== gsz - gsz + (4 - lvl) * (gsz - 3) + 3
== (gsz - 3) * (4 - lvl) + 3

thanks
-- PMM