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

[Qiang Liu <cyruscyliu@gmail.com>]

On Wed, Aug 4, 2021 at 3:43 PM Thomas Huth <thuth@redhat.com> wrote:
>
> 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?
It seems that each file in tests/qtest is called by the name of a
single virtual device. I follow this pattern. Additionally, maybe if,
in the future, xlnx-dp is used by another machine, then it is not
proper to call the file xlnx-zcu102. What do you think about it?

> > @@ -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)
Got it.

> > +    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.
Got it.

> > +        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>
Thank you!