| Message ID | 20240630151414.2969772-1-zheyuma97@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully | expand |
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 >
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; > }
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
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; } }
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(-)