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 9:04 PM
> To: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Zheyu Ma <zheyuma97@gmail.com>; qemu-devel@nongnu.org
> Subject: [PATCH] hw/display/tcx: Fix out-of-bounds access in tcx_blit_writel
>
> This patch addresses a potential out-of-bounds memory access issue in the
> tcx_blit_writel function. It adds bounds checking to ensure that memory
> accesses do not exceed the allocated VRAM size. If an out-of-bounds access
> is detected, an error is logged using qemu_log_mask.
>
> ASAN log:
> ==2960379==ERROR: AddressSanitizer: SEGV on unknown address
> 0x7f524752fd01 (pc 0x7f525c2c4881 bp 0x7ffdaf87bfd0 sp 0x7ffdaf87b788 T0)
> ==2960379==The signal is caused by a READ memory access.
> #0 0x7f525c2c4881 in memcpy
> string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:222
> #1 0x55aa782bd5b1 in __asan_memcpy
> llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
> #2 0x55aa7854dedd in tcx_blit_writel hw/display/tcx.c:590:13
>
> Reproducer:
> cat << EOF | qemu-system-sparc -display none \
> -machine accel=qtest, -m 512M -machine LX -m 256 -qtest stdio
> writel 0x562e98c4 0x3d92fd01
> EOF
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
> hw/display/tcx.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 99507e7638..af43bea7f2 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -33,6 +33,7 @@
> #include "migration/vmstate.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> +#include "qemu/log.h"
> #include "qom/object.h"
>
> #define TCX_ROM_FILE "QEMU,tcx.bin"
> @@ -577,6 +578,14 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
> addr = (addr >> 3) & 0xfffff;
> adsr = val & 0xffffff;
> len = ((val >> 24) & 0x1f) + 1;
> +
> + if (addr + len > s->vram_size || adsr + len > s->vram_size) {
if adsr == 0xffffff, this condition may be always true, thus the branch “if (adsr == 0xffffff) {” will be never
reached.
You are right, I misunderstood the condition :(
Thanks,
Zheyu
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: VRAM access out of bounds. addr: 0x%lx, adsr:
> 0x%x, len: %u\n",
> + __func__, addr, adsr, len);
> + return;
> + }
> +
> if (adsr == 0xffffff) {
> memset(&s->vram[addr], s->tmpblit, len);
> if (s->depth == 24) {
> --
> 2.34.1
>