Re: [PATCH] hw/display/sm501: Validate local memory size index in sm501_system_config_write

[BALATON Zoltan]

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;