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