Re: [Qemu-arm] [Qemu-devel] [PATCH-for-4.1 3/3] hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO

[Alistair Francis]

On Tue, Jul 9, 2019 at 4:38 AM Philippe Mathieu-Daudé <address@hidden> wrote:
>
> In the previous commit we fixed a crash when the guest read a
> register that pop from an empty FIFO.
> By auditing the repository, we found another similar use with
> an easy way to reproduce:
>
>   $ qemu-system-aarch64 -M xlnx-zcu102 -monitor stdio -S
>   QEMU 4.0.50 monitor - type 'help' for more information
>   (qemu) xp/b 0xfd4a0134
>   Aborted (core dumped)
>
>   (gdb) bt
>   #0  0x00007f6936dea57f in raise () at /lib64/libc.so.6
>   #1  0x00007f6936dd4895 in abort () at /lib64/libc.so.6
>   #2  0x0000561ad32975ec in xlnx_dp_aux_pop_rx_fifo (s=0x7f692babee70) at 
> hw/display/xlnx_dp.c:431
>   #3  0x0000561ad3297dc0 in xlnx_dp_read (opaque=0x7f692babee70, offset=77, 
> size=4) at hw/display/xlnx_dp.c:667
>   #4  0x0000561ad321b896 in memory_region_read_accessor (mr=0x7f692babf620, 
> addr=308, value=0x7ffe05c1db88, size=4, shift=0, mask=4294967295, attrs=...) 
> at memory.c:439
>   #5  0x0000561ad321bd70 in access_with_adjusted_size (addr=308, 
> value=0x7ffe05c1db88, size=1, access_size_min=4, access_size_max=4, 
> access_fn=0x561ad321b858 <memory_region_read_accessor>, mr=0x7f692babf620, 
> attrs=...) at memory.c:569
>   #6  0x0000561ad321e9d5 in memory_region_dispatch_read1 (mr=0x7f692babf620, 
> addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1420
>   #7  0x0000561ad321ea9d in memory_region_dispatch_read (mr=0x7f692babf620, 
> addr=308, pval=0x7ffe05c1db88, size=1, attrs=...) at memory.c:1447
>   #8  0x0000561ad31bd742 in flatview_read_continue (fv=0x561ad69c04f0, 
> addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", 
> len=1, addr1=308, l=1, mr=0x7f692babf620) at exec.c:3385
>   #9  0x0000561ad31bd895 in flatview_read (fv=0x561ad69c04f0, 
> addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", 
> len=1) at exec.c:3423
>   #10 0x0000561ad31bd90b in address_space_read_full (as=0x561ad5bb3020, 
> addr=4249485620, attrs=..., buf=0x7ffe05c1dcf0 "\020\335\301\005\376\177", 
> len=1) at exec.c:3436
>   #11 0x0000561ad33b1c42 in address_space_read (len=1, buf=0x7ffe05c1dcf0 
> "\020\335\301\005\376\177", attrs=..., addr=4249485620, as=0x561ad5bb3020) at 
> include/exec/memory.h:2131
>   #12 0x0000561ad33b1c42 in memory_dump (mon=0x561ad59c4530, count=1, 
> format=120, wsize=1, addr=4249485620, is_physical=1) at monitor/misc.c:723
>   #13 0x0000561ad33b1fc1 in hmp_physical_memory_dump (mon=0x561ad59c4530, 
> qdict=0x561ad6c6fd00) at monitor/misc.c:795
>   #14 0x0000561ad37b4a9f in handle_hmp_command (mon=0x561ad59c4530, 
> cmdline=0x561ad59d0f22 "/b 0x00000000fd4a0134") at monitor/hmp.c:1082
>
> Fix by checking the FIFO is not empty before popping from it.
>
> The datasheet is not clear about the reset value of this register,
> we choose to return '0'.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> ---
>  hw/display/xlnx_dp.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index cfd4c700b7..cc5b650df0 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -427,11 +427,18 @@ static uint8_t xlnx_dp_aux_pop_rx_fifo(XlnxDPState *s)
>      uint8_t ret;
>
>      if (fifo8_is_empty(&s->rx_fifo)) {
> -        DPRINTF("rx_fifo underflow..\n");
> -        abort();
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Reading empty RX_FIFO\n",
> +                      __func__);
> +        /*
> +         * The datasheet is not clear about the reset value, it seems
> +         * to be unspecified. We choose to return '0'.
> +         */
> +        ret = 0;
> +    } else {
> +        ret = fifo8_pop(&s->rx_fifo);
> +        DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret);
>      }
> -    ret = fifo8_pop(&s->rx_fifo);
> -    DPRINTF("pop 0x%" PRIX8 " from rx_fifo.\n", ret);
>      return ret;
>  }
>
> --
> 2.20.1
>
>