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

[Peter Xu <peterx@redhat.com>]

On Tue, May 06, 2025 at 02:21:08PM +0100, Peter Maydell wrote:
> 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>

I'll adjust that when sending a PR.  Thanks!

-- 
Peter Xu