address_space_write_rom_internal can take in a NULL pointer for ptr if
it's only doing cache flushes instead of populating the ROM.

However, if building with --enable-ubsan, incrementing buf causes ubsan
to go off when doing cache flushes, since it will trigger on pointer
arithmetic on a NULL pointer, even if that NULL pointer doesn't get
dereferenced.

To fix this, we can move the buf incrementing to only be done when
writing data to ROM, since that's the only point where it gets
dereferenced and should be non-NULL.

Found by running:
qemu-system-aarch64 \
-machine virt \
-accel kvm

When built with --enable-ubsan.

Signed-off-by: Joe Komlodi <komlodi@google.com>
---
 system/physmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/system/physmem.c b/system/physmem.c
index 16cf557d1a..ccd2b50da3 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3204,6 +3204,7 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
             case WRITE_DATA:
                 memcpy(ram_ptr, buf, l);
                 invalidate_and_set_dirty(mr, addr1, l);
+                buf += l;
                 break;
             case FLUSH_CACHE:
                 flush_idcache_range((uintptr_t)ram_ptr, (uintptr_t)ram_ptr, l);
@@ -3211,7 +3212,6 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
             }
         }
         len -= l;
-        buf += l;
         addr += l;
     }
     return MEMTX_OK;
-- 
2.49.0.967.g6a0df3ecc3-goog

On Mon, 5 May 2025 at 23:23, Joe Komlodi <komlodi@google.com> wrote:
>
> address_space_write_rom_internal can take in a NULL pointer for ptr if
> it's only doing cache flushes instead of populating the ROM.
>
> However, if building with --enable-ubsan, incrementing buf causes ubsan
> to go off when doing cache flushes, since it will trigger on pointer
> arithmetic on a NULL pointer, even if that NULL pointer doesn't get
> dereferenced.
>
> To fix this, we can move the buf incrementing to only be done when
> writing data to ROM, since that's the only point where it gets
> dereferenced and should be non-NULL.
>
> Found by running:
> qemu-system-aarch64 \
> -machine virt \
> -accel kvm
>
> When built with --enable-ubsan.
>
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> ---
>  system/physmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 16cf557d1a..ccd2b50da3 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3204,6 +3204,7 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
>              case WRITE_DATA:
>                  memcpy(ram_ptr, buf, l);
>                  invalidate_and_set_dirty(mr, addr1, l);
> +                buf += l;
>                  break;

very minor, but I think the buf += l would be slightly better
one line up, next to the memcpy(). That way we keep the
"copy more data from buf" and "advance buf the corresponding
amount" next to each other, rather than separating them by
the set-dirty operation on the MR.

Anyway
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

On Mon, May 05, 2025 at 10:22:36PM +0000, Joe Komlodi wrote:
> address_space_write_rom_internal can take in a NULL pointer for ptr if
> it's only doing cache flushes instead of populating the ROM.
> 
> However, if building with --enable-ubsan, incrementing buf causes ubsan
> to go off when doing cache flushes, since it will trigger on pointer
> arithmetic on a NULL pointer, even if that NULL pointer doesn't get
> dereferenced.
> 
> To fix this, we can move the buf incrementing to only be done when
> writing data to ROM, since that's the only point where it gets
> dereferenced and should be non-NULL.
> 
> Found by running:
> qemu-system-aarch64 \
> -machine virt \
> -accel kvm
> 
> When built with --enable-ubsan.
> 
> Signed-off-by: Joe Komlodi <komlodi@google.com>

queued, thanks.

-- 
Peter Xu