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

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

[Qiang Liu]

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
@@ -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 ");
+    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) {
+        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 = \
-- 
2.7.4

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

[Thomas Huth]

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>

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

[Qiang Liu]

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!

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

[Thomas Huth]

On 06/08/2021 09.00, Qiang Liu wrote:
> 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?

Ok, fair points, then let's keep the name as you suggested.

  Thomas

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

[Alexander Bulekov]

On 210804 1451, 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>

Acked-by: Alexander Bulekov <alxndr@bu.edu>

If there is somehow a regression, the test won't fail in a fatal way, so
maybe it makes sense to throw in a setenv(UBSAN_OPTIONS=halt_on_error=1)

As a side note(not strictly related to this fix) should we continue
joining reproducer patches with the fixes? In order to test the
reproducer, you need to cleave the fix off the patch. At the same time
we don't want to mess up bisection, so does it make sense to have the
reproducer patch be separate but come last in the series?

Thanks

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

[Gerd Hoffmann]

  Hi,

> As a side note(not strictly related to this fix) should we continue
> joining reproducer patches with the fixes? In order to test the
> reproducer, you need to cleave the fix off the patch. At the same time
> we don't want to mess up bisection, so does it make sense to have the
> reproducer patch be separate but come last in the series?

Yes, I think it makes sense to send the testcase as separate patch, and
the ordering (fix first, testcase second) makes sense too.  If they are
separated it is easy enough to create a local test branch with a
different order, or to just temporarily revert the fix to test the
testcase.

take care,
  Gerd

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

[Peter Maydell]

On Fri, 6 Aug 2021 at 15:43, Alexander Bulekov <alxndr@bu.edu> wrote:
> As a side note(not strictly related to this fix) should we continue
> joining reproducer patches with the fixes? In order to test the
> reproducer, you need to cleave the fix off the patch. At the same time
> we don't want to mess up bisection, so does it make sense to have the
> reproducer patch be separate but come last in the series?

My preference is for the test case as a separate patch, last
in the series. For this kind of minor easy-to-review fix it
matters less, but sometimes the right fix for a problem might
be larger or more complicated, and then having the test case
in the same patch makes that patch awkwardly large.

Also the person able to review the code change and the person
able to review the test case might not be the same...

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 8/6/21 4:42 PM, Alexander Bulekov wrote:
> On 210804 1451, 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>
> 
> Acked-by: Alexander Bulekov <alxndr@bu.edu>
> 
> If there is somehow a regression, the test won't fail in a fatal way, so
> maybe it makes sense to throw in a setenv(UBSAN_OPTIONS=halt_on_error=1)

Where? Main meson? qtests meson? setenv() in the test (but would
override preset variable)?

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

[Qiang Liu]

Thank you for all the insightful comments about the separated patches.
This would be my first time to format a serial of patches. Does it
look like below?
[PATCH v3 00/2] title
     [PATCH v3 01/2] fix
     [PATCH v3 02/2] test

Best,
Qiang

On Mon, Aug 9, 2021 at 11:24 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 6 Aug 2021 at 15:43, Alexander Bulekov <alxndr@bu.edu> wrote:
> > As a side note(not strictly related to this fix) should we continue
> > joining reproducer patches with the fixes? In order to test the
> > reproducer, you need to cleave the fix off the patch. At the same time
> > we don't want to mess up bisection, so does it make sense to have the
> > reproducer patch be separate but come last in the series?
>
> My preference is for the test case as a separate patch, last
> in the series. For this kind of minor easy-to-review fix it
> matters less, but sometimes the right fix for a problem might
> be larger or more complicated, and then having the test case
> in the same patch makes that patch awkwardly large.
>
> Also the person able to review the code change and the person
> able to review the test case might not be the same...
>
> thanks
> -- PMM

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

[Alexander Bulekov]

On 210809 1131, Philippe Mathieu-Daudé wrote:
> On 8/6/21 4:42 PM, Alexander Bulekov wrote:
> > On 210804 1451, 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>
> > 
> > Acked-by: Alexander Bulekov <alxndr@bu.edu>
> > 
> > If there is somehow a regression, the test won't fail in a fatal way, so
> > maybe it makes sense to throw in a setenv(UBSAN_OPTIONS=halt_on_error=1)
> 
> Where? Main meson? qtests meson? setenv() in the test (but would
> override preset variable)?
> 

Probably in the test, with overwrite = 0 ? Without halt_on_error the
test will succeed even if the problem returns..
-Alex

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

[Philippe Mathieu-Daudé]

On 8/9/21 11:33 AM, Qiang Liu wrote:
> Thank you for all the insightful comments about the separated patches.
> This would be my first time to format a serial of patches. Does it
> look like below?

> [PATCH v3 00/2] title
>      [PATCH v3 01/2] fix
>      [PATCH v3 02/2] test

Exactly. Otherwise, adding the test before the fix would
make an incremental build to fail.