Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read

[Thomas Huth <thuth@redhat.com>]

On 04/08/2021 08.51, Qiang Liu wrote:
> xlnx_dp_read allows an out-of-bounds read at its default branch because
> of an improper index.
> 
> According to
> https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> (DP Module), registers 0x3A4/0x3A4/0x3AC are allowed.
> 
> DP_INT_MASK     0x000003A4      32      mixed   0xFFFFF03F      Interrupt Mask Register for intrN.
> DP_INT_EN       0x000003A8      32      mixed   0x00000000      Interrupt Enable Register.
> DP_INT_DS       0x000003AC      32      mixed   0x00000000      Interrupt Disable Register.
> 
> In xlnx_dp_write, when the offset is 0x3A8 and 0x3AC, the virtual device
> will write s->core_registers[0x3A4
>>> 2]. That is to say, the maxize of s->core_registers could be ((0x3A4
>>> 2) + 1). However, the current size of s->core_registers is (0x3AF >>
>>> 2), that is ((0x3A4 >> 2) + 2), which is out of the range.
> In xlxn_dp_read, the access to offset 0x3A8 or 0x3AC will be directed to
> the offset 0x3A8 (incorrect functionality) or 0x3AC (out-of-bounds read)
> rather than 0x3A4.
> 
> This patch enforces the read access to offset 0x3A8 and 0x3AC to 0x3A4,
> but does not adjust the size of s->core_registers to avoid breaking
> migration.
> 
> Fixes: 58ac482a66de ("introduce xlnx-dp")
> Signed-off-by: Qiang Liu <cyruscyliu@gmail.com>
> ---
> v2:
>    - not change DP_CORE_REG_ARRAY_SIZE
>    - add a qtest reproducer
>    - update the code style
> 
> I have a question about the QTest reproducer. Before patching xlnx-dp,
> (0x3ac >> 2) will exceed the right bound of s->core_registers.  However,
> this is allowed by the assertion. There is no warning and this
> reproducer will pass. Is the reprodocer OK?
> 
>   hw/display/xlnx_dp.c            |  6 +++++-
>   tests/qtest/fuzz-xlnx-dp-test.c | 33 +++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build         |  1 +
>   3 files changed, 39 insertions(+), 1 deletion(-)
>   create mode 100644 tests/qtest/fuzz-xlnx-dp-test.c
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 7bcbb13..747df6e 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -714,7 +714,11 @@ static uint64_t xlnx_dp_read(void *opaque, hwaddr offset, unsigned size)
>           break;
>       default:
>           assert(offset <= (0x3AC >> 2));
> -        ret = s->core_registers[offset];
> +        if (offset == (0x3A8 >> 2) || offset == (0x3AC >> 2)) {
> +            ret = s->core_registers[DP_INT_MASK];
> +        } else {
> +            ret = s->core_registers[offset];
> +        }
>           break;
>       }
>   
> diff --git a/tests/qtest/fuzz-xlnx-dp-test.c b/tests/qtest/fuzz-xlnx-dp-test.c
> new file mode 100644
> index 0000000..69eb6c0
> --- /dev/null
> +++ b/tests/qtest/fuzz-xlnx-dp-test.c

Would it make sense to call the file xlnx-zcu102.c instead, in case we want 
to add other tests related to this machine later?

> @@ -0,0 +1,33 @@
> +/*
> + * QTest fuzzer-generated testcase for xlnx-dp display device
> + *
> + * Copyright (c) 2021 Qiang Liu <cyruscyliu@gmail.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqos/libqtest.h"
> +
> +/*
> + * This used to trigger the out-of-bounds read in xlnx_dp_read
> + */
> +static void test_fuzz_xlnx_dp_0x3ac(void)
> +{
> +    QTestState *s = qtest_init("-M xlnx-zcu102 -display none ");

You don't need "-display none", it's added by default in the qtest framework 
(see tests/qtest/libqtest.c).

> +    qtest_readl(s, 0xfd4a03ac);
> +    qtest_quit(s);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +   if (strcmp(arch, "aarch64") == 0) {

You likely don't need the architecture check, since it's only added for 
aarch64 in the meson.build file anyway.

> +        qtest_add_func("fuzz/test_fuzz_xlnx_dp/3ac", test_fuzz_xlnx_dp_0x3ac);
> +   }
> +
> +   return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 83ad237..6fd6b0e 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -185,6 +185,7 @@ qtests_aarch64 = \
>      'numa-test',
>      'boot-serial-test',
>      'xlnx-can-test',
> +   'fuzz-xlnx-dp-test',
>      'migration-test']
>   
>   qtests_s390x = \

With at least the "-display none" removed:
Acked-by: Thomas Huth <thuth@redhat.com>