Lei Sun found while auditing the code than a CPU write would
trigger a NULL pointer deference.

From UG1085 datasheet [*] AXI writes in this region are ignored
and generates an External Slave Error (SLVERR).

Fix by checking the access is a READ before calling the region
callback.

[*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

Reported-by: Lei Sun <slei.casper@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v1: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01238.html
---
 hw/ssi/xilinx_spips.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 8115bb6d46..0c9ccd12bf 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1202,6 +1202,18 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
     }
 }
 
+static bool lqspi_accepts(void *opaque, hwaddr addr,
+                          unsigned size, bool is_write,
+                          MemTxAttrs attrs)
+{
+    /*
+     * From UG1085, Chapter 24 (Quad-SPI controllers):
+     * - Writes are ignored
+     * - AXI writes generate an external AXI slave error (SLVERR)
+     */
+    return !is_write;
+}
+
 static uint64_t
 lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 {
@@ -1225,6 +1237,7 @@ static const MemoryRegionOps lqspi_ops = {
     .read = lqspi_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
+        .accepts = lqspi_accepts,
         .min_access_size = 1,
         .max_access_size = 4
     }
-- 
2.20.1

Hi Philippe,

On [2019 Jul 05] Fri 12:42:55, Philippe Mathieu-Daudé wrote:
> Lei Sun found while auditing the code than a CPU write would
> trigger a NULL pointer deference.
> 
> From UG1085 datasheet [*] AXI writes in this region are ignored
> and generates an External Slave Error (SLVERR).
> 
> Fix by checking the access is a READ before calling the region
> callback.
> 
> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> 
> Reported-by: Lei Sun <slei.casper@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01238.html
> ---
>  hw/ssi/xilinx_spips.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..0c9ccd12bf 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,6 +1202,18 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>      }
>  }
>  
> +static bool lqspi_accepts(void *opaque, hwaddr addr,
> +                          unsigned size, bool is_write,
> +                          MemTxAttrs attrs)
> +{
> +    /*
> +     * From UG1085, Chapter 24 (Quad-SPI controllers):
> +     * - Writes are ignored
> +     * - AXI writes generate an external AXI slave error (SLVERR)
> +     */
> +    return !is_write;

What I can see above generates an AXI Decode error (MEMTX_DECODE_ERROR),
MEMTX_ERROR is the one that maps to AXI slave error. What about swapping to the
solution you proposed yesterday? (but changing return value of the write
function to be MEMTX_ERROR)

Best regards,
Francisco Iglesias

> +}
> +
>  static uint64_t
>  lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>  {
> @@ -1225,6 +1237,7 @@ static const MemoryRegionOps lqspi_ops = {
>      .read = lqspi_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
> +        .accepts = lqspi_accepts,
>          .min_access_size = 1,
>          .max_access_size = 4
>      }
> -- 
> 2.20.1
> 

+-- On Fri, 5 Jul 2019, Philippe Mathieu-Daudé wrote --+
| +static bool lqspi_accepts(void *opaque, hwaddr addr,
| +                          unsigned size, bool is_write,
| +                          MemTxAttrs attrs)
| +{
| +    /*
| +     * From UG1085, Chapter 24 (Quad-SPI controllers):
| +     * - Writes are ignored
| +     * - AXI writes generate an external AXI slave error (SLVERR)
| +     */
| +    return !is_write;
| +}
| +
|  static uint64_t
|  lqspi_read(void *opaque, hwaddr addr, unsigned int size)
|  {
| @@ -1225,6 +1237,7 @@ static const MemoryRegionOps lqspi_ops = {
|      .read = lqspi_read,
|      .endianness = DEVICE_NATIVE_ENDIAN,
|      .valid = {
| +        .accepts = lqspi_accepts,
|          .min_access_size = 1,
|          .max_access_size = 4
|      }

Looks okay.

To confirm,
  When lqspi_accepts() returns false, guest will see an error/exception?


Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F