[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_
From: |
Qiang Liu |
Subject: |
Re: [PATCH v2] hw/display/xlnx_dp: fix an out-of-bounds read in xlnx_dp_read |
Date: |
Fri, 6 Aug 2021 15:00:36 +0800 |
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!