Re: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully

[Peter Maydell]

On Mon, 1 Jul 2024 at 12:18, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Zheyu,
>
> On 30/6/24 17:14, Zheyu Ma wrote:
> > This commit handles invalid address accesses gracefully in both read and 
> > write
> > functions. Instead of asserting and aborting, it logs an error message and 
> > returns
> > a neutral value for read operations and does nothing for write operations.
> >
> > Error log:
> > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be 
> > reached
> > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code 
> > should not be reached
> > Aborted
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio
> > readw 0x3f212003
>
> Thanks for this very interesting bug report (and reproducer).
>
> > EOF
> >
> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> > ---
> >   hw/misc/bcm2835_thermal.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c
> > index ee7816b8a5..5c2a429d58 100644
> > --- a/hw/misc/bcm2835_thermal.c
> > +++ b/hw/misc/bcm2835_thermal.c
> > @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque, 
> > hwaddr addr, unsigned size)
> >           val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true);
> >           break;
> >       default:
> > -        /* MemoryRegionOps are aligned, so this can not happen. */
> > -        g_assert_not_reached();
>
> Like Xingtao Yao mentioned, I believe the current code is correct
> and shouldn't be reached.
>
> Why is it reached? You might have uncovered a core memory bug.

I think we get here because the bcm2835_thermal_ops MemoryRegionOps
sets impl.max_access_size and valid.max_access_size to 4, but it leaves
impl.min_access_size and valid.min_access_size unset, which means
"default to 1". So the memory system is presented with an access
of size 2 at offset 3, and it tries to synthesize it as a pair
of byte accesses at offsets 3 and 4; but the offset 3 trips
our assert above. So I think the fix is:

@@ -80,8 +80,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps bcm2835_thermal_ops = {
     .read = bcm2835_thermal_read,
     .write = bcm2835_thermal_write,
+    .impl.min_access_size = 4,
     .impl.max_access_size = 4,
     .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };

The .valid.max_access_size change is because otherwise we
fall over in a different way: the memory system tries to
synthesize a read using a 4-byte access at offset 3 even
though the device doesn't say it permits unaligned accesses.
This part does seem like a bug in the access_with_adjusted_size
code. If we want to look at that we should probably start with
this patch series from Tomoyuki Hirose from earlier this year:
https://lore.kernel.org/qemu-devel/20240201081313.1339788-1-tomoyuki.hirose@igel.co.jp/
which attempted to improve that code.

thanks
-- PMM