hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully

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

Posted by Zheyu Ma 1 year, 7 months ago

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
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();
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "bcm2835_thermal_read: invalid address 0x%"
+                      HWADDR_PRIx "\n", addr);
+        val = 0;
     }
     return val;
 }
@@ -72,8 +74,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr,
                        __func__, value, addr);
         break;
     default:
-        /* MemoryRegionOps are aligned, so this can not happen. */
-        g_assert_not_reached();
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "bcm2835_thermal_write: invalid address 0x%"
+                      HWADDR_PRIx "\n", addr);
+        break;
     }
 }
 
-- 
2.34.1

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

Posted by Philippe Mathieu-Daudé 1 year, 7 months ago

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;
>   }

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

Posted by Peter Maydell 1 year, 7 months ago

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

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

Posted by Xingtao Yao (Fujitsu) via 1 year, 7 months ago

Hi, zheyu

> -----Original Message-----
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of Zheyu
> Ma
> Sent: Sunday, June 30, 2024 11:14 PM
> To: Peter Maydell <peter.maydell@linaro.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>
> Cc: Zheyu Ma <zheyuma97@gmail.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Subject: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses
> gracefully
> 
> 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
> 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();
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "bcm2835_thermal_read: invalid address 0x%"
> +                      HWADDR_PRIx "\n", addr);
> +        val = 0;
>      }
>      return val;
>  }
> @@ -72,8 +74,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr
> addr,
>                         __func__, value, addr);
>          break;
>      default:
> -        /* MemoryRegionOps are aligned, so this can not happen. */
> -        g_assert_not_reached();
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "bcm2835_thermal_write: invalid address 0x%"
> +                      HWADDR_PRIx "\n", addr);
> +        break;
>      }
>  }

the default branch will never be reached in normal access, so I think this 
modification is not needed.

> 
> --
> 2.34.1
>