Re: [PATCH] system/physmem: Fix UBSan finding in address_space_write_rom_internal

[Peter Maydell]

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