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