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

[Thomas Huth <thuth@redhat.com>]

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