[PATCH] system/physmem: Where we assume we have a RAM MR, assert it

[PATCH] system/physmem: Where we assume we have a RAM MR, assert it

[Peter Maydell]

In the functions invalidate_and_set_dirty() and
cpu_physical_memory_snapshot_and_clear_dirty(), we assume that we
are dealing with RAM memory regions. In this case we know that
memory_region_get_ram_addr() will succeed. Assert this before we
use the returned ram_addr_t in arithmetic.

This makes Coverity happier about these functions: it otherwise
complains that we might have an arithmetic overflow that stems
from the possible -1 return from memory_region_get_ram_addr().

Resolves: Coverity CID 1547629, 1547715

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 system/physmem.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 9a3b3a76360..87554d68ea4 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -894,13 +894,19 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
 {
     DirtyMemoryBlocks *blocks;
-    ram_addr_t start = memory_region_get_ram_addr(mr) + offset;
+    ram_addr_t start, first, last;
     unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL);
-    ram_addr_t first = QEMU_ALIGN_DOWN(start, align);
-    ram_addr_t last  = QEMU_ALIGN_UP(start + length, align);
     DirtyBitmapSnapshot *snap;
     unsigned long page, end, dest;
 
+    start = memory_region_get_ram_addr(mr);
+    /* We know we're only called for RAM MemoryRegions */
+    assert(start != RAM_ADDR_INVALID);
+    start += offset;
+
+    first = QEMU_ALIGN_DOWN(start, align);
+    last  = QEMU_ALIGN_UP(start + length, align);
+
     snap = g_malloc0(sizeof(*snap) +
                      ((last - first) >> (TARGET_PAGE_BITS + 3)));
     snap->start = first;
@@ -2630,7 +2636,11 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
     uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
-    addr += memory_region_get_ram_addr(mr);
+    ram_addr_t ramaddr = memory_region_get_ram_addr(mr);
+
+    /* We know we're only called for RAM MemoryRegions */
+    assert(ramaddr != RAM_ADDR_INVALID);
+    addr += ramaddr;
 
     /* No early return if dirty_log_mask is or becomes 0, because
      * cpu_physical_memory_set_dirty_range will still call
-- 
2.34.1

Re: [PATCH] system/physmem: Where we assume we have a RAM MR, assert it

[Peter Xu]

On Tue, Jul 23, 2024 at 06:05:13PM +0100, Peter Maydell wrote:
> In the functions invalidate_and_set_dirty() and
> cpu_physical_memory_snapshot_and_clear_dirty(), we assume that we
> are dealing with RAM memory regions. In this case we know that
> memory_region_get_ram_addr() will succeed. Assert this before we
> use the returned ram_addr_t in arithmetic.
> 
> This makes Coverity happier about these functions: it otherwise
> complains that we might have an arithmetic overflow that stems
> from the possible -1 return from memory_region_get_ram_addr().
> 
> Resolves: Coverity CID 1547629, 1547715
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH] system/physmem: Where we assume we have a RAM MR, assert it

[David Hildenbrand]

On 23.07.24 19:05, Peter Maydell wrote:
> In the functions invalidate_and_set_dirty() and
> cpu_physical_memory_snapshot_and_clear_dirty(), we assume that we
> are dealing with RAM memory regions. In this case we know that
> memory_region_get_ram_addr() will succeed. Assert this before we
> use the returned ram_addr_t in arithmetic.
> 
> This makes Coverity happier about these functions: it otherwise
> complains that we might have an arithmetic overflow that stems
> from the possible -1 return from memory_region_get_ram_addr().
> 
> Resolves: Coverity CID 1547629, 1547715
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH] system/physmem: Where we assume we have a RAM MR, assert it

[Peter Maydell]

On Tue, 23 Jul 2024 at 18:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the functions invalidate_and_set_dirty() and
> cpu_physical_memory_snapshot_and_clear_dirty(), we assume that we
> are dealing with RAM memory regions. In this case we know that
> memory_region_get_ram_addr() will succeed. Assert this before we
> use the returned ram_addr_t in arithmetic.
>
> This makes Coverity happier about these functions: it otherwise
> complains that we might have an arithmetic overflow that stems
> from the possible -1 return from memory_region_get_ram_addr().
>
> Resolves: Coverity CID 1547629, 1547715
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> --

I'm doing a target-arm pullreq so I'll take this patch
through that, unless you'd prefer otherwise.

thanks
-- PMM