Re: [PATCH] hw/display: refine upper limit for offset value in assert check

[Richard Henderson]

On 12/12/24 05:45, gerben@altlinux.org wrote:
> From: Denis Rastyogin <gerben@altlinux.org>
>
> Accessing an element of the s->core_registers array
> with a size of 236 (0x3AC) may lead to a buffer overflow,
> as the index 'offset' can exceed the valid range and reach values
> up to 5139 (0x504C >> 2). This change addresses
> a potential vulnerability when writing data.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Reported-by: David Meliksetyan <d.meliksetyan@fobos-nt.ru>
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
> ---
>   hw/display/xlnx_dp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 6ab2335499..69ccc7ccc2 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -743,6 +743,7 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, 
> uint64_t value,
>       DPRINTF("core write @%" PRIx64 " = 0x%8.8" PRIX64 "\n", offset, value);
>   
>       offset = offset >> 2;
> +    assert(offset <= (0x3AC >> 2));
>   
>       switch (offset) {
>       /*
> @@ -896,7 +897,6 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, 
> uint64_t value,
>           xlnx_dp_update_irq(s);
>           break;
>       default:
> -        assert(offset <= (0x504C >> 2));
>           s->core_registers[offset] = value;
>           break;
>       }

Why are you moving the assert?  The switch statement takes care of sorting non-default 
values of offset.

More correct would be to use DP_CORE_REG_ARRAY_SIZE in the assert, along with a comment 
that the io region has been sized exactly to fit core_registers[].


r~