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

[Philippe Mathieu-Daudé]

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.

Likely around access_with_adjusted_size() in system/memory.c.

I'll keep investigating, but so far it reminds me a previous
patch from Andrew, but it isn't the fix:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20170630030058.28943-1-andrew@aj.id.au/

> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "bcm2835_thermal_read: invalid address 0x%"
> +                      HWADDR_PRIx "\n", addr);
> +        val = 0;
>       }
>       return val;
>   }