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

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

[Joe Komlodi]

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

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

[Peter Xu]

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

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

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

[Peter Xu]

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

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

[Peter Xu]

On Fri, May 09, 2025 at 11:22:10AM -0400, Peter Xu wrote:
> 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!

Unfortunately, this patch (with/without the touch up..) breaks ppc32 on the
boot-serial test..

Reproducer:

$ QTEST_QEMU_BINARY=./qemu-system-ppc ./tests/qtest/boot-serial-test -r /ppc/boot-serial/mac99

I think what happened is the ROM writer in that test can provide a "buf"
pointer over some range where memory_region_supports_direct_access() can
sometimes return true, but not always, in a WRITE_DATA request.

In that case, "buf" needs to be properly incremented in all cases, because
in that case only some part of "buf" is copied, and only copied if the
guest mem is directly accessible.  We'll need to increment "buf" properly
even if it's looping over some range of indirect memory.

I'll drop it as of now, please feel free to have a closer look..

Thanks,

-- 
Peter Xu