In sm501_system_config_write(), we update the local memory size index
based on the incoming value. However, there was no check to ensure
that the index is within the valid range, which could result in
a buffer overflow.

This commit adds a check to ensure that the local memory size index
is within the valid range before updating it. If the index is not
valid, an error is logged and the index is not updated.

ASAN log:
==3067247==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55c6586e4d3c at pc 0x55c655d4e0ac bp 0x7ffc9d5c6a10 sp 0x7ffc9d5c6a08
READ of size 4 at 0x55c6586e4d3c thread T0
    #0 0x55c655d4e0ab in sm501_2d_operation qemu/hw/display/sm501.c:729:21
    #1 0x55c655d4b8a1 in sm501_2d_engine_write qemu/hw/display/sm501.c:1551:13

Reproducer:
cat << EOF | qemu-system-x86_64  \
-display none -machine accel=qtest, -m 512M -machine q35 -nodefaults \
-device sm501 -qtest stdio
outl 0xcf8 0x80000814
outl 0xcfc 0xe4000000
outl 0xcf8 0x80000804
outw 0xcfc 0x02
writel 0xe4000010 0xe000
writel 0xe4100010 0x10000
writel 0xe4100008 0x10001
writel 0xe410000c 0x80000000
EOF

Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 hw/display/sm501.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 26dc8170d8..2cdfeaacab 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1020,11 +1020,19 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
         s->gpio_63_32_control = value & 0xFF80FFFF;
         break;
     case SM501_DRAM_CONTROL:
-        s->local_mem_size_index = (value >> 13) & 0x7;
-        /* TODO : check validity of size change */
+    {
+        int local_mem_size_index = (value >> 13) & 0x7;
+        if (local_mem_size_index < ARRAY_SIZE(sm501_mem_local_size)) {
+            s->local_mem_size_index = local_mem_size_index;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "sm501: Invalid local_mem_size_index value: %d\n",
+                          local_mem_size_index);
+        }
         s->dram_control &= 0x80000000;
         s->dram_control |= value & 0x7FFFFFC3;
         break;
+    }
     case SM501_ARBTRTN_CONTROL:
         s->arbitration_control = value & 0x37777777;
         break;
-- 
2.34.1

On Wed, 3 Jul 2024, Zheyu Ma wrote:
> In sm501_system_config_write(), we update the local memory size index
> based on the incoming value. However, there was no check to ensure
> that the index is within the valid range, which could result in
> a buffer overflow.
>
> This commit adds a check to ensure that the local memory size index
> is within the valid range before updating it. If the index is not
> valid, an error is logged and the index is not updated.
>
> ASAN log:
> ==3067247==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55c6586e4d3c at pc 0x55c655d4e0ac bp 0x7ffc9d5c6a10 sp 0x7ffc9d5c6a08
> READ of size 4 at 0x55c6586e4d3c thread T0
>    #0 0x55c655d4e0ab in sm501_2d_operation qemu/hw/display/sm501.c:729:21
>    #1 0x55c655d4b8a1 in sm501_2d_engine_write qemu/hw/display/sm501.c:1551:13
>
> Reproducer:
> cat << EOF | qemu-system-x86_64  \
> -display none -machine accel=qtest, -m 512M -machine q35 -nodefaults \
> -device sm501 -qtest stdio
> outl 0xcf8 0x80000814
> outl 0xcfc 0xe4000000
> outl 0xcf8 0x80000804
> outw 0xcfc 0x02
> writel 0xe4000010 0xe000
> writel 0xe4100010 0x10000
> writel 0xe4100008 0x10001
> writel 0xe410000c 0x80000000
> EOF
>
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
> hw/display/sm501.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 26dc8170d8..2cdfeaacab 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1020,11 +1020,19 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>         s->gpio_63_32_control = value & 0xFF80FFFF;
>         break;
>     case SM501_DRAM_CONTROL:
> -        s->local_mem_size_index = (value >> 13) & 0x7;
> -        /* TODO : check validity of size change */
> +    {
> +        int local_mem_size_index = (value >> 13) & 0x7;
> +        if (local_mem_size_index < ARRAY_SIZE(sm501_mem_local_size)) {
> +            s->local_mem_size_index = local_mem_size_index;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "sm501: Invalid local_mem_size_index value: %d\n",
> +                          local_mem_size_index);
> +        }

Thanks. This fixes the overflow in the size index array but we may need to 
also check if it would set it to larger than 
memory_region_size(&s->local_mem_region) otherwise 2d operations may 
overflow that.

Regards,
BALATON Zoltan

>         s->dram_control &= 0x80000000;
>         s->dram_control |= value & 0x7FFFFFC3;
>         break;
> +    }
>     case SM501_ARBTRTN_CONTROL:
>         s->arbitration_control = value & 0x37777777;
>         break;
>