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

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.

Hi Mark,

On Mon, Jul 1, 2024 at 10:49 PM Mark Cave-Ayland <
mark.cave-ayland@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 <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?
>
>
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

On Tue, 2 Jul 2024 at 19:15, Zheyu Ma <zheyuma97@gmail.com> wrote:
>
> Hi Mark,
>
> On Mon, Jul 1, 2024 at 10:49 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 30/06/2024 14:04, Zheyu Ma wrote:
>> 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.

Yeah, I couldn't find a datasheet for this device either.
In the absence of any clear information, I think what we usually
do in QEMU is take a plausible guess at what it might do
and/or implement something that's straightforward for us to
implement. Chances are good that real guest code never exercises
the weird corners of the device behaviour anyway. Possible
options include:
 * just ignore the blit attempt entirely
 * clamp source and destination addr/length and do the parts
   that do fall within the vram
 * treat address calculations as always wrapping around within
   the vram (so if you address off the top of it you end up
   reading from the bottom of it)

I would suggest we just pick one, implement it (with a comment
saying we don't have a spec so we're guessing about the
behaviour in this case), and then test that whatever guest
code we have (e.g. the bios, linux, some BSD) doesn't
misbehave with the patch applied.

By the way, doesn't this problem also affect the other
TCX accelerator functions? Most obviously, tcg_rblit_writel()
is basically the same structure for computing address and length;
but also e.g. tcx_stip_writel() and tcx_rstip_writel() don't
do bounds checking before accessing s->vram[addr + i].

thanks
-- PMM

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.

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

Hi Xingtao,

On Mon, Jul 1, 2024 at 5:13 AM Xingtao Yao (Fujitsu) <yaoxt.fnst@fujitsu.com>
wrote:

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