From: Thomas Huth <thuth@redhat.com>

If a guest triggers a multi-byte read/write at the very end of a memory
region, the code access_with_adjusted_size() still tries to access
all bytes of the transfer, even if the final bytes are already beyond
the memory region's size. If the device handler cannot cope with those
accesses, bad things can happen, for example:

 $ echo "writew 0x800064 0x4142" | \
   ./qemu-system-avr -M mega2560 -display none -qtest stdio -accel qtest
 [I 0.000001] OPENED
 [R +0.001750] writew 0x800064 0x4142
 qemu-system-avr: ../../devel/qemu/hw/misc/avr_power.c:58:
  avr_mask_write: Assertion `offset == 0' failed.
 Aborted                    (core dumped)

We really should not call MR handlers for bytes that are beyond the
MR's size, so let's add a check to limit the size in such cases.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3393
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 system/memory.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 56f3225b21a..2ff74c42e3f 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -531,6 +531,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     uint64_t access_mask;
     unsigned access_size;
     unsigned i;
+    unsigned int checked_size;
     MemTxResult r = MEMTX_OK;
     bool reentrancy_guard_applied = false;
 
@@ -557,13 +558,21 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
+
+    if (addr + size > mr->size) {
+        assert(addr < mr->size);
+        checked_size = mr->size - addr;
+    } else {
+        checked_size = size;
+    }
+
     if (devend_big_endian(mr->ops->endianness)) {
-        for (i = 0; i < size; i += access_size) {
+        for (i = 0; i < checked_size; i += access_size) {
             r |= access_fn(mr, addr + i, value, access_size,
                         (size - access_size - i) * 8, access_mask, attrs);
         }
     } else {
-        for (i = 0; i < size; i += access_size) {
+        for (i = 0; i < checked_size; i += access_size) {
             r |= access_fn(mr, addr + i, value, access_size, i * 8,
                         access_mask, attrs);
         }
-- 
2.53.0

On Mon, 20 Apr 2026 at 08:52, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> If a guest triggers a multi-byte read/write at the very end of a memory
> region, the code access_with_adjusted_size() still tries to access
> all bytes of the transfer, even if the final bytes are already beyond
> the memory region's size. If the device handler cannot cope with those
> accesses, bad things can happen, for example:
>
>  $ echo "writew 0x800064 0x4142" | \
>    ./qemu-system-avr -M mega2560 -display none -qtest stdio -accel qtest
>  [I 0.000001] OPENED
>  [R +0.001750] writew 0x800064 0x4142
>  qemu-system-avr: ../../devel/qemu/hw/misc/avr_power.c:58:
>   avr_mask_write: Assertion `offset == 0' failed.
>  Aborted                    (core dumped)
>
> We really should not call MR handlers for bytes that are beyond the
> MR's size, so let's add a check to limit the size in such cases.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3393
> Signed-off-by: Thomas Huth <thuth@redhat.com>

The MR in question sets .impl.max_access_size to 1 but leaves
.valid.max_access_size at its default (which we treat as 4),
and is then registered with an MR size of 1. Perhaps we should
also assert on MR registration that
  .valid.max_access_size <= the MR size ?
Though there might I suppose be cases where the MR window can be
guest-configured to various sizes so it might in some silly setups
be smaller than the largest permitted access size on startup.

We should also set .valid.max_access_size on this specific
device to 1, though the fix to the core memory system is
definitely something we should have (and that I was under
the impression we already enforced...)

> ---
>  system/memory.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 56f3225b21a..2ff74c42e3f 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -531,6 +531,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>      uint64_t access_mask;
>      unsigned access_size;
>      unsigned i;
> +    unsigned int checked_size;
>      MemTxResult r = MEMTX_OK;
>      bool reentrancy_guard_applied = false;
>
> @@ -557,13 +558,21 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>      /* FIXME: support unaligned access? */
>      access_size = MAX(MIN(size, access_size_max), access_size_min);
>      access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> +
> +    if (addr + size > mr->size) {

mr->size is an Int128 -- you can't portably do direct
arithmetic on it like this. (We still support configs where
there is no working __int128_t and Int128 is a struct.
I think --enable-tcg-interpreter is one way to get that.)

There are also I think cases where addr + size might overflow.
(addr is the within-MR offset, but we permit MR sizes up to
and including 2^64.)

> +        assert(addr < mr->size);
> +        checked_size = mr->size - addr;
> +    } else {
> +        checked_size = size;
> +    }

I wonder if this is the right place to enforce this.
Another option would be to have memory_region_access_valid()
do the check and return false for accesses off the end.
The splitting of writes that we do here is only supposed to be
handling the case of "the implementation allows smaller chunks
than the device permits", not the "access overlaps between two
different MRs" case (which would need to be handled much further
up the callstack before we have identified the specific MR we
are accessing).

> +
>      if (devend_big_endian(mr->ops->endianness)) {
> -        for (i = 0; i < size; i += access_size) {
> +        for (i = 0; i < checked_size; i += access_size) {
>              r |= access_fn(mr, addr + i, value, access_size,
>                          (size - access_size - i) * 8, access_mask, attrs);
>          }
>      } else {
> -        for (i = 0; i < size; i += access_size) {
> +        for (i = 0; i < checked_size; i += access_size) {
>              r |= access_fn(mr, addr + i, value, access_size, i * 8,
>                          access_mask, attrs);
>          }

thanks
-- PMM

On 20/04/2026 11.51, Peter Maydell wrote:
> On Mon, 20 Apr 2026 at 08:52, Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> If a guest triggers a multi-byte read/write at the very end of a memory
>> region, the code access_with_adjusted_size() still tries to access
>> all bytes of the transfer, even if the final bytes are already beyond
>> the memory region's size. If the device handler cannot cope with those
>> accesses, bad things can happen, for example:
>>
>>   $ echo "writew 0x800064 0x4142" | \
>>     ./qemu-system-avr -M mega2560 -display none -qtest stdio -accel qtest
>>   [I 0.000001] OPENED
>>   [R +0.001750] writew 0x800064 0x4142
>>   qemu-system-avr: ../../devel/qemu/hw/misc/avr_power.c:58:
>>    avr_mask_write: Assertion `offset == 0' failed.
>>   Aborted                    (core dumped)
>>
>> We really should not call MR handlers for bytes that are beyond the
>> MR's size, so let's add a check to limit the size in such cases.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3393
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> The MR in question sets .impl.max_access_size to 1 but leaves
> .valid.max_access_size at its default (which we treat as 4),
> and is then registered with an MR size of 1. Perhaps we should
> also assert on MR registration that
>    .valid.max_access_size <= the MR size ?

Sounds like a good idea, but after a doing a quick check, it seems to me 
that we've got this pattern all over the place (see e.g. the "pcspk" or 
"kvmvapic" devices), so this would require a lot of clean-up patches first...

> Though there might I suppose be cases where the MR window can be
> guest-configured to various sizes so it might in some silly setups
> be smaller than the largest permitted access size on startup.
> 
> We should also set .valid.max_access_size on this specific
> device to 1, though the fix to the core memory system is
> definitely something we should have (and that I was under
> the impression we already enforced...)

Agreed, it makes sense to fix the .valid.max_access_size in that avr device. 
I'll send a separate patch for it.

>> ---
>>   system/memory.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 56f3225b21a..2ff74c42e3f 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -531,6 +531,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>>       uint64_t access_mask;
>>       unsigned access_size;
>>       unsigned i;
>> +    unsigned int checked_size;
>>       MemTxResult r = MEMTX_OK;
>>       bool reentrancy_guard_applied = false;
>>
>> @@ -557,13 +558,21 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>>       /* FIXME: support unaligned access? */
>>       access_size = MAX(MIN(size, access_size_max), access_size_min);
>>       access_mask = MAKE_64BIT_MASK(0, access_size * 8);
>> +
>> +    if (addr + size > mr->size) {
> 
> mr->size is an Int128 -- you can't portably do direct
> arithmetic on it like this. (We still support configs where
> there is no working __int128_t and Int128 is a struct.
> I think --enable-tcg-interpreter is one way to get that.)
> 
> There are also I think cases where addr + size might overflow.
> (addr is the within-MR offset, but we permit MR sizes up to
> and including 2^64.)

Ah, right ... I'll ponder about how to do this best (and your comment below) 
and will try to come up with a better solution.

  Thanks,
   Thomas


>> +        assert(addr < mr->size);
>> +        checked_size = mr->size - addr;
>> +    } else {
>> +        checked_size = size;
>> +    }
> 
> I wonder if this is the right place to enforce this.
> Another option would be to have memory_region_access_valid()
> do the check and return false for accesses off the end.
> The splitting of writes that we do here is only supposed to be
> handling the case of "the implementation allows smaller chunks
> than the device permits", not the "access overlaps between two
> different MRs" case (which would need to be handled much further
> up the callstack before we have identified the specific MR we
> are accessing).
> 
>> +
>>       if (devend_big_endian(mr->ops->endianness)) {
>> -        for (i = 0; i < size; i += access_size) {
>> +        for (i = 0; i < checked_size; i += access_size) {
>>               r |= access_fn(mr, addr + i, value, access_size,
>>                           (size - access_size - i) * 8, access_mask, attrs);
>>           }
>>       } else {
>> -        for (i = 0; i < size; i += access_size) {
>> +        for (i = 0; i < checked_size; i += access_size) {
>>               r |= access_fn(mr, addr + i, value, access_size, i * 8,
>>                           access_mask, attrs);
>>           }
> 
> thanks
> -- PMM
>

On Tue, 21 Apr 2026 at 09:20, Thomas Huth <thuth@redhat.com> wrote:
>
> On 20/04/2026 11.51, Peter Maydell wrote:
> > On Mon, 20 Apr 2026 at 08:52, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> From: Thomas Huth <thuth@redhat.com>
> >>
> >> If a guest triggers a multi-byte read/write at the very end of a memory
> >> region, the code access_with_adjusted_size() still tries to access
> >> all bytes of the transfer, even if the final bytes are already beyond
> >> the memory region's size. If the device handler cannot cope with those
> >> accesses, bad things can happen, for example:
> >>
> >>   $ echo "writew 0x800064 0x4142" | \
> >>     ./qemu-system-avr -M mega2560 -display none -qtest stdio -accel qtest
> >>   [I 0.000001] OPENED
> >>   [R +0.001750] writew 0x800064 0x4142
> >>   qemu-system-avr: ../../devel/qemu/hw/misc/avr_power.c:58:
> >>    avr_mask_write: Assertion `offset == 0' failed.
> >>   Aborted                    (core dumped)
> >>
> >> We really should not call MR handlers for bytes that are beyond the
> >> MR's size, so let's add a check to limit the size in such cases.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3393
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >
> > The MR in question sets .impl.max_access_size to 1 but leaves
> > .valid.max_access_size at its default (which we treat as 4),
> > and is then registered with an MR size of 1. Perhaps we should
> > also assert on MR registration that
> >    .valid.max_access_size <= the MR size ?
>
> Sounds like a good idea, but after a doing a quick check, it seems to me
> that we've got this pattern all over the place (see e.g. the "pcspk" or
> "kvmvapic" devices), so this would require a lot of clean-up patches first...

I realized yesterday while looking at something else that for
the IO case (i.e. x86 in/out insns, not memory-mapped) the
access size really can be bigger than the mapped size. There's
a comment in address_space_translate_internal() that mentions this:

    /* MMIO registers can be expected to perform full-width accesses based only
     * on their address, without considering adjacent registers that could
     * decode to completely different MemoryRegions.  When such registers
     * exist (e.g. I/O ports 0xcf8 and 0xcf9 on most PC chipsets), MMIO
     * regions overlap wildly.  For this reason we cannot clamp the accesses
     * here.

Port 0xcf8 is for PCI probing and is 32-bits wide; 0xcf9 is to
do with triggering reset.

thanks
-- PMM