On Tue, May 06, 2025 at 02:21:08PM +0100, Peter Maydell wrote: > On Mon, 5 May 2025 at 23:23, Joe Komlodi <koml...@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 <koml...@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.mayd...@linaro.org>
I'll adjust that when sending a PR. Thanks! -- Peter Xu
