On 06/08/2021 09.00, Qiang Liu wrote:
On Wed, Aug 4, 2021 at 3:43 PM Thomas Huth <th...@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 <cyruscy...@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
