Re: [Qemu-arm] [Qemu-devel] [PATCH-for-4.1 2/3] hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO

[Alistair Francis]

On Tue, Jul 9, 2019 at 4:42 AM Philippe Mathieu-Daudé <address@hidden> wrote:
>
> Reading the RX_DATA register when the RX_FIFO is empty triggers
> an abort. This can be easily reproduced:
>
>   $ qemu-system-arm -M emcraft-sf2 -monitor stdio -S
>   QEMU 4.0.50 monitor - type 'help' for more information
>   (qemu) x 0x40001010
>   Aborted (core dumped)
>
>   (gdb) bt
>   #1  0x00007f035874f895 in abort () at /lib64/libc.so.6
>   #2  0x00005628686591ff in fifo8_pop (fifo=0x56286a9a4c68) at util/fifo8.c:66
>   #3  0x00005628683e0b8e in fifo32_pop (fifo=0x56286a9a4c68) at 
> include/qemu/fifo32.h:137
>   #4  0x00005628683e0efb in spi_read (opaque=0x56286a9a4850, addr=4, size=4) 
> at hw/ssi/mss-spi.c:168
>   #5  0x0000562867f96801 in memory_region_read_accessor (mr=0x56286a9a4b60, 
> addr=16, value=0x7ffeecb0c5c8, size=4, shift=0, mask=4294967295, attrs=...) 
> at memory.c:439
>   #6  0x0000562867f96cdb in access_with_adjusted_size (addr=16, 
> value=0x7ffeecb0c5c8, size=4, access_size_min=1, access_size_max=4, 
> access_fn=0x562867f967c3 <memory_region_read_accessor>, mr=0x56286a9a4b60, 
> attrs=...) at memory.c:569
>   #7  0x0000562867f99940 in memory_region_dispatch_read1 (mr=0x56286a9a4b60, 
> addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1420
>   #8  0x0000562867f99a08 in memory_region_dispatch_read (mr=0x56286a9a4b60, 
> addr=16, pval=0x7ffeecb0c5c8, size=4, attrs=...) at memory.c:1447
>   #9  0x0000562867f38721 in flatview_read_continue (fv=0x56286aec6360, 
> addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, 
> addr1=16, l=4, mr=0x56286a9a4b60) at exec.c:3385
>   #10 0x0000562867f38874 in flatview_read (fv=0x56286aec6360, 
> addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at 
> exec.c:3423
>   #11 0x0000562867f388ea in address_space_read_full (as=0x56286aa3e890, 
> addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4) at 
> exec.c:3436
>   #12 0x0000562867f389c5 in address_space_rw (as=0x56286aa3e890, 
> addr=1073745936, attrs=..., buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, 
> is_write=false) at exec.c:3466
>   #13 0x0000562867f3bdd7 in cpu_memory_rw_debug (cpu=0x56286aa19d00, 
> addr=1073745936, buf=0x7ffeecb0c7c0 "\340ǰ\354\376\177", len=4, is_write=0) 
> at exec.c:3976
>   #14 0x000056286811ed51 in memory_dump (mon=0x56286a8c32d0, count=1, 
> format=120, wsize=4, addr=1073745936, is_physical=0) at monitor/misc.c:730
>   #15 0x000056286811eff1 in hmp_memory_dump (mon=0x56286a8c32d0, 
> qdict=0x56286b15c400) at monitor/misc.c:785
>   #16 0x00005628684740ee in handle_hmp_command (mon=0x56286a8c32d0, 
> cmdline=0x56286a8caeb2 "0x40001010") at monitor/hmp.c:1082
>
> From the datasheet "Actel SmartFusion Microcontroller Subsystem
> User's Guide" Rev.1, Table 13-3 "SPI Register Summary", this
> register has a reset value of 0.
>
> Check the FIFO is not empty before accessing it, else log an
> error message.
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> ---
>  hw/ssi/mss-spi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
> index 4878279482..3201a577a1 100644
> --- a/hw/ssi/mss-spi.c
> +++ b/hw/ssi/mss-spi.c
> @@ -155,7 +155,13 @@ spi_read(void *opaque, hwaddr addr, unsigned int size)
>      case R_SPI_RX:
>          s->regs[R_SPI_STATUS] &= ~S_RXFIFOFUL;
>          s->regs[R_SPI_STATUS] &= ~S_RXCHOVRF;
> -        ret = fifo32_pop(&s->rx_fifo);
> +        if (fifo32_is_empty(&s->rx_fifo)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Reading empty RX_FIFO\n",
> +                          __func__);
> +        } else {
> +            ret = fifo32_pop(&s->rx_fifo);
> +        }
>          if (fifo32_is_empty(&s->rx_fifo)) {
>              s->regs[R_SPI_STATUS] |= S_RXFIFOEMP;
>          }
> --
> 2.20.1
>
>