[Qemu-devel] [PATCH-for-4.1 0/3] hw: Avoid crash when reading empty RX FIFO

[Qemu-devel] [PATCH-for-4.1 0/3] hw: Avoid crash when reading empty RX FIFO

[Philippe Mathieu-Daudé]

Fix a pair of crashes, one is an audit of first one.

Philippe Mathieu-Daudé (3):
  hw/ssi/mss-spi: Convert debug printf()s to trace events
  hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO
  hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO

 Makefile.objs        |  1 +
 hw/display/xlnx_dp.c | 15 +++++++++++----
 hw/ssi/mss-spi.c     | 31 +++++++++++++------------------
 hw/ssi/trace-events  |  6 ++++++
 4 files changed, 31 insertions(+), 22 deletions(-)
 create mode 100644 hw/ssi/trace-events

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] hw/ssi/mss-spi: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Useful while debugging, can be skipped for next dev cycle.

 Makefile.objs       |  1 +
 hw/ssi/mss-spi.c    | 23 ++++++-----------------
 hw/ssi/trace-events |  6 ++++++
 3 files changed, 13 insertions(+), 17 deletions(-)
 create mode 100644 hw/ssi/trace-events

diff --git a/Makefile.objs b/Makefile.objs
index 6a143dcd57..60ec443091 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -178,6 +178,7 @@ trace-events-subdirs += hw/scsi
 trace-events-subdirs += hw/sd
 trace-events-subdirs += hw/sparc
 trace-events-subdirs += hw/sparc64
+trace-events-subdirs += hw/ssi
 trace-events-subdirs += hw/timer
 trace-events-subdirs += hw/tpm
 trace-events-subdirs += hw/usb
diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
index 918b1f3e82..4878279482 100644
--- a/hw/ssi/mss-spi.c
+++ b/hw/ssi/mss-spi.c
@@ -27,18 +27,8 @@
 #include "hw/ssi/mss-spi.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
-#ifndef MSS_SPI_ERR_DEBUG
-#define MSS_SPI_ERR_DEBUG   0
-#endif
-
-#define DB_PRINT_L(lvl, fmt, args...) do { \
-    if (MSS_SPI_ERR_DEBUG >= lvl) { \
-        qemu_log("%s: " fmt "\n", __func__, ## args); \
-    } \
-} while (0)
-
-#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
 
 #define FIFO_CAPACITY         32
 
@@ -187,9 +177,9 @@ spi_read(void *opaque, hwaddr addr, unsigned int size)
         }
         break;
     }
-
-    DB_PRINT("addr=0x%" HWADDR_PRIx " = 0x%" PRIx32, addr * 4, ret);
+    trace_mss_spi_read(addr << 2, ret);
     spi_update_irq(s);
+
     return ret;
 }
 
@@ -225,9 +215,9 @@ static void spi_flush_txfifo(MSSSpiState *s)
         s->regs[R_SPI_STATUS] &= ~(S_TXDONE | S_RXRDY);
 
         tx = fifo32_pop(&s->tx_fifo);
-        DB_PRINT("data tx:0x%" PRIx32, tx);
+        trace_mss_spi_flush_fifo("tx", tx);
         rx = ssi_transfer(s->spi, tx);
-        DB_PRINT("data rx:0x%" PRIx32, rx);
+        trace_mss_spi_flush_fifo("rx", rx);
 
         if (fifo32_num_used(&s->rx_fifo) == s->fifo_depth) {
             s->regs[R_SPI_STATUS] |= S_RXCHOVRF;
@@ -262,9 +252,8 @@ static void spi_write(void *opaque, hwaddr addr,
     MSSSpiState *s = opaque;
     uint32_t value = val64;
 
-    DB_PRINT("addr=0x%" HWADDR_PRIx " =0x%" PRIx32, addr, value);
+    trace_mss_spi_write(addr, value);
     addr >>= 2;
-
     switch (addr) {
     case R_SPI_TX:
         /* adding to already full FIFO */
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
new file mode 100644
index 0000000000..6e494b913c
--- /dev/null
+++ b/hw/ssi/trace-events
@@ -0,0 +1,6 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# mss-spi.c
+mss_spi_read(uint32_t addr, uint32_t value) "read addr:0x%02x value:0x%08x"
+mss_spi_write(uint32_t addr, uint32_t value) "write addr:0x%02x value:0x%08x"
+mss_spi_flush_fifo(const char *name, uint32_t value) "flush fifo:%s value:0x%08x"
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/3] hw/ssi/mss-spi: Convert debug printf()s to trace events

[Alistair Francis]

On Tue, Jul 9, 2019 at 4:40 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Useful while debugging, can be skipped for next dev cycle.
>
>  Makefile.objs       |  1 +
>  hw/ssi/mss-spi.c    | 23 ++++++-----------------
>  hw/ssi/trace-events |  6 ++++++
>  3 files changed, 13 insertions(+), 17 deletions(-)
>  create mode 100644 hw/ssi/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 6a143dcd57..60ec443091 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -178,6 +178,7 @@ trace-events-subdirs += hw/scsi
>  trace-events-subdirs += hw/sd
>  trace-events-subdirs += hw/sparc
>  trace-events-subdirs += hw/sparc64
> +trace-events-subdirs += hw/ssi
>  trace-events-subdirs += hw/timer
>  trace-events-subdirs += hw/tpm
>  trace-events-subdirs += hw/usb
> diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
> index 918b1f3e82..4878279482 100644
> --- a/hw/ssi/mss-spi.c
> +++ b/hw/ssi/mss-spi.c
> @@ -27,18 +27,8 @@
>  #include "hw/ssi/mss-spi.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "trace.h"
>
> -#ifndef MSS_SPI_ERR_DEBUG
> -#define MSS_SPI_ERR_DEBUG   0
> -#endif
> -
> -#define DB_PRINT_L(lvl, fmt, args...) do { \
> -    if (MSS_SPI_ERR_DEBUG >= lvl) { \
> -        qemu_log("%s: " fmt "\n", __func__, ## args); \
> -    } \
> -} while (0)
> -
> -#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>
>  #define FIFO_CAPACITY         32
>
> @@ -187,9 +177,9 @@ spi_read(void *opaque, hwaddr addr, unsigned int size)
>          }
>          break;
>      }
> -
> -    DB_PRINT("addr=0x%" HWADDR_PRIx " = 0x%" PRIx32, addr * 4, ret);
> +    trace_mss_spi_read(addr << 2, ret);
>      spi_update_irq(s);
> +
>      return ret;
>  }
>
> @@ -225,9 +215,9 @@ static void spi_flush_txfifo(MSSSpiState *s)
>          s->regs[R_SPI_STATUS] &= ~(S_TXDONE | S_RXRDY);
>
>          tx = fifo32_pop(&s->tx_fifo);
> -        DB_PRINT("data tx:0x%" PRIx32, tx);
> +        trace_mss_spi_flush_fifo("tx", tx);
>          rx = ssi_transfer(s->spi, tx);
> -        DB_PRINT("data rx:0x%" PRIx32, rx);
> +        trace_mss_spi_flush_fifo("rx", rx);
>
>          if (fifo32_num_used(&s->rx_fifo) == s->fifo_depth) {
>              s->regs[R_SPI_STATUS] |= S_RXCHOVRF;
> @@ -262,9 +252,8 @@ static void spi_write(void *opaque, hwaddr addr,
>      MSSSpiState *s = opaque;
>      uint32_t value = val64;
>
> -    DB_PRINT("addr=0x%" HWADDR_PRIx " =0x%" PRIx32, addr, value);
> +    trace_mss_spi_write(addr, value);
>      addr >>= 2;
> -
>      switch (addr) {
>      case R_SPI_TX:
>          /* adding to already full FIFO */
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> new file mode 100644
> index 0000000000..6e494b913c
> --- /dev/null
> +++ b/hw/ssi/trace-events
> @@ -0,0 +1,6 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# mss-spi.c
> +mss_spi_read(uint32_t addr, uint32_t value) "read addr:0x%02x value:0x%08x"
> +mss_spi_write(uint32_t addr, uint32_t value) "write addr:0x%02x value:0x%08x"
> +mss_spi_flush_fifo(const char *name, uint32_t value) "flush fifo:%s value:0x%08x"
> --
> 2.20.1
>
>

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

[Philippe Mathieu-Daudé]

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é <philmd@redhat.com>
---
 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

Re: [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é <philmd@redhat.com> 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é <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

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
>
>

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

[Philippe Mathieu-Daudé]

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é <philmd@redhat.com>
---
 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

Re: [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é <philmd@redhat.com> 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é <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

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
>
>

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

[Peter Maydell]

On Tue, 9 Jul 2019 at 12:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Fix a pair of crashes, one is an audit of first one.
>
> Philippe Mathieu-Daudé (3):
>   hw/ssi/mss-spi: Convert debug printf()s to trace events
>   hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO
>   hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO
>

Applied patches 2 and 3 to target-arm.next, thanks.
For future series, could you avoid including both
for-4.1 and not-for-4.1 patches in the same series, please?
It's a bit of a pain to have to separate them out again,
and there's always the risk that I don't notice.

(You'll probably want to hold onto patch 1 and send it
out again after we reopen for 4.2, or I'll forget about its
existence :-))

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 7/11/19 4:51 PM, Peter Maydell wrote:
> On Tue, 9 Jul 2019 at 12:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Fix a pair of crashes, one is an audit of first one.
>>
>> Philippe Mathieu-Daudé (3):
>>   hw/ssi/mss-spi: Convert debug printf()s to trace events
>>   hw/ssi/mss-spi: Avoid crash when reading empty RX FIFO
>>   hw/display/xlnx_dp: Avoid crash when reading empty RX FIFO
>>
> 
> Applied patches 2 and 3 to target-arm.next, thanks.
> For future series, could you avoid including both
> for-4.1 and not-for-4.1 patches in the same series, please?
> It's a bit of a pain to have to separate them out again,
> and there's always the risk that I don't notice.

Sure, duly noted.

> (You'll probably want to hold onto patch 1 and send it
> out again after we reopen for 4.2, or I'll forget about its
> existence :-))

No problem!

Regards,

Phil.