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