Re: [PATCH] hw/display/tcx: Fix out-of-bounds access in tcx_blit_writel

[Mark Cave-Ayland]

On 30/06/2024 14:04, Zheyu Ma wrote:

> 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) {
> +            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) {

What would happen if the source data plus length goes beyond the end of the 
framebuffer but the destination lies completely within it? Presumably the length of 
the data copy should be restricted to the length of the source data rather than the 
entire copy being ignored?


ATB,

Mark.