Hi Mark,
On Mon, Jul 1, 2024 at 10:49 PM Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:
> 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 <zheyum...@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?
>
>
Thanks for your useful tips! However, I'm unfamiliar with the tcx device
and cannot find a specification/datasheet. I apologize for not proposing a
proper fix.
Regards,
Zheyu
