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

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

gerben@altlinux.org posted 1 patch 1 year, 1 month ago

Diff against v1 v2 v3
Download mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20241212114554.517379-1-gerben@altlinux.org

Maintainers: Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Peter Maydell <peter.maydell@linaro.org>

There is a newer version of this series

hw/display/xlnx_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Expand all Fold all

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

Posted by gerben@altlinux.org 1 year, 1 month ago

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

Posted by Richard Henderson 1 year, 1 month ago

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~