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

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

[gerben]

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;
     }
-- 
2.42.2

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~

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

[gerben]

From: Denis Rastyogin <gerben@altlinux.org>

Accessing s->core_registers (size 236) could overflow
if the offset goes beyond the valid range.

Since the memory region matches core_registers size exactly,
guest cannot write out-of-bounds.

Therefore, the debug assert has been refined to ensure the offset
remains within DP_CORE_REG_ARRAY_SIZE, preventing internal errors.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 7c980ee642..b35ee2f869 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -896,7 +896,11 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
         xlnx_dp_update_irq(s);
         break;
     default:
-        assert(offset <= (0x504C >> 2));
+        /*
+         * Check to ensure the offset is within the bounds of
+         * the core_registers[] array.
+         */
+        assert(offset < DP_CORE_REG_ARRAY_SIZE);
         s->core_registers[offset] = value;
         break;
     }
-- 
2.42.2