[Qemu-devel] [PATCH-for-4.1 v4 0/3] hw/ssi/xilinx_spips: Avoid NULL pointer deference

[Qemu-devel] [PATCH-for-4.1 v4 0/3] hw/ssi/xilinx_spips: Avoid NULL pointer deference

[Philippe Mathieu-Daudé]

v1: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01238.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01586.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01657.html
v4
- address Francisco comments from v3
- RFC avoid out-of-bound

Philippe Mathieu-Daudé (3):
  hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
  hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory
  hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

 hw/ssi/xilinx_spips.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v4 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

In the next commit we will implement the write_with_attrs()
handler. To avoid using different APIs, convert the read()
handler first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: Do not ignore lqspi_read() return value (Francisco)
---
 hw/ssi/xilinx_spips.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 8115bb6d46..b7c7275dbe 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1202,27 +1202,26 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
     }
 }
 
-static uint64_t
-lqspi_read(void *opaque, hwaddr addr, unsigned int size)
+static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
+                              unsigned size, MemTxAttrs attrs)
 {
-    XilinxQSPIPS *q = opaque;
-    uint32_t ret;
+    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
 
     if (addr >= q->lqspi_cached_addr &&
             addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
         uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
-        ret = cpu_to_le32(*(uint32_t *)retp);
-        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
-                   (unsigned)ret);
-        return ret;
-    } else {
-        lqspi_load_cache(opaque, addr);
-        return lqspi_read(opaque, addr, size);
+        *value = cpu_to_le32(*(uint32_t *)retp);
+        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
+                   addr, *value);
+        return MEMTX_OK;
     }
+
+    lqspi_load_cache(opaque, addr);
+    return lqspi_read(opaque, addr, value, size, attrs);
 }
 
 static const MemoryRegionOps lqspi_ops = {
-    .read = lqspi_read,
+    .read_with_attrs = lqspi_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v4 2/3] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

[Philippe Mathieu-Daudé]

Lei Sun found while auditing the code that a CPU write would
trigger a NULL pointer dereference.

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

Fix by implementing the write_with_attrs() handler.
Return MEMTX_ERROR when the region is accessed (this error maps
to an AXI slave error).

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

Reported-by: Lei Sun <slei.casper@gmail.com>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: Fix typos (Francisco)
---
 hw/ssi/xilinx_spips.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index b7c7275dbe..3c4e8365ee 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1220,8 +1220,24 @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
     return lqspi_read(opaque, addr, value, size, attrs);
 }
 
+static MemTxResult lqspi_write(void *opaque, hwaddr offset, uint64_t value,
+                               unsigned size, MemTxAttrs attrs)
+{
+    /*
+     * From UG1085, Chapter 24 (Quad-SPI controllers):
+     * - Writes are ignored
+     * - AXI writes generate an external AXI slave error (SLVERR)
+     */
+    qemu_log_mask(LOG_GUEST_ERROR, "%s Unexpected %u-bit access to 0x%" PRIx64
+                                   " (value: 0x%" PRIx64 "\n",
+                  __func__, size << 3, offset, value);
+
+    return MEMTX_ERROR;
+}
+
 static const MemoryRegionOps lqspi_ops = {
     .read_with_attrs = lqspi_read,
+    .write_with_attrs = lqspi_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.20.1

[Qemu-devel] [RFC PATCH-for-4.1 v4 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Philippe Mathieu-Daudé]

Both lqspi_read() and lqspi_load_cache() expect a 32-bit
aligned address.

Set MemoryRegionOps.impl values to force 32-bit accesses,
this way we are sure we do not access the lqspi_buf[] array
out of bound.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Late friday patch...

 hw/ssi/xilinx_spips.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 3c4e8365ee..8f705132a3 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1243,6 +1243,10 @@ static const MemoryRegionOps lqspi_ops = {
         .min_access_size = 1,
         .max_access_size = 4
     }
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    }
 };
 
 static void xilinx_spips_realize(DeviceState *dev, Error **errp)
-- 
2.20.1

Re: [Qemu-devel] [RFC PATCH-for-4.1 v4 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Francisco Iglesias]

Hi Philippe,

On [2019 Jul 05] Fri 22:25:00, Philippe Mathieu-Daudé wrote:
> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
> aligned address.
> 
> Set MemoryRegionOps.impl values to force 32-bit accesses,
> this way we are sure we do not access the lqspi_buf[] array
> out of bound.

The patch and correction follows chapter 24 aswell (UG1085) so I suggest
dropping 'RFC' in the subject.

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Late friday patch...
> 
>  hw/ssi/xilinx_spips.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 3c4e8365ee..8f705132a3 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1243,6 +1243,10 @@ static const MemoryRegionOps lqspi_ops = {
>          .min_access_size = 1,
>          .max_access_size = 4
>      }

A comma is missing on the line above (else the patch doesn't compile).

Best regards,
Francisco

> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    }
>  };
>  
>  static void xilinx_spips_realize(DeviceState *dev, Error **errp)
> -- 
> 2.20.1
> 

Re: [Qemu-devel] [PATCH-for-4.1 v4 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Francisco Iglesias]

On [2019 Jul 05] Fri 22:24:58, Philippe Mathieu-Daudé wrote:
> In the next commit we will implement the write_with_attrs()
> handler. To avoid using different APIs, convert the read()
> handler first.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
> v4: Do not ignore lqspi_read() return value (Francisco)
> ---
>  hw/ssi/xilinx_spips.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..b7c7275dbe 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,27 +1202,26 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>      }
>  }
>  
> -static uint64_t
> -lqspi_read(void *opaque, hwaddr addr, unsigned int size)
> +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
> +                              unsigned size, MemTxAttrs attrs)
>  {
> -    XilinxQSPIPS *q = opaque;
> -    uint32_t ret;
> +    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
>  
>      if (addr >= q->lqspi_cached_addr &&
>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
> -        ret = cpu_to_le32(*(uint32_t *)retp);
> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
> -                   (unsigned)ret);
> -        return ret;
> -    } else {
> -        lqspi_load_cache(opaque, addr);
> -        return lqspi_read(opaque, addr, size);
> +        *value = cpu_to_le32(*(uint32_t *)retp);
> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
> +                   addr, *value);
> +        return MEMTX_OK;
>      }
> +
> +    lqspi_load_cache(opaque, addr);
> +    return lqspi_read(opaque, addr, value, size, attrs);
>  }
>  
>  static const MemoryRegionOps lqspi_ops = {
> -    .read = lqspi_read,
> +    .read_with_attrs = lqspi_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> -- 
> 2.20.1
> 

Re: [Qemu-devel] [PATCH-for-4.1 v4 0/3] hw/ssi/xilinx_spips: Avoid NULL pointer deference

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190705202500.18853-1-philmd@redhat.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

  CC      hw/ssi/aspeed_smc.o
  CC      hw/ssi/stm32f2xx_spi.o
  CC      hw/ssi/mss-spi.o
/var/tmp/patchew-tester-tmp-ghjjv86b/src/hw/ssi/xilinx_spips.c:1246:5: error: expected ‘}’ before ‘.’ token
 1246 |     .impl = {
      |     ^
/var/tmp/patchew-tester-tmp-ghjjv86b/src/hw/ssi/xilinx_spips.c:1238:42: note: to match this ‘{’


The full log is available at
http://patchew.org/logs/20190705202500.18853-1-philmd@redhat.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [RFC PATCH-for-4.1 v4 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Philippe Mathieu-Daudé]

On 7/6/19 12:24 AM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 05] Fri 22:25:00, Philippe Mathieu-Daudé wrote:
>> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
>> aligned address.
>>
>> Set MemoryRegionOps.impl values to force 32-bit accesses,
>> this way we are sure we do not access the lqspi_buf[] array
>> out of bound.
> 
> The patch and correction follows chapter 24 aswell (UG1085) so I suggest
> dropping 'RFC' in the subject.

OK.

>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Late friday patch...
>>
>>  hw/ssi/xilinx_spips.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index 3c4e8365ee..8f705132a3 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1243,6 +1243,10 @@ static const MemoryRegionOps lqspi_ops = {
>>          .min_access_size = 1,
>>          .max_access_size = 4
>>      }
> 
> A comma is missing on the line above (else the patch doesn't compile).

Sorry, I first added this block before '.valid' and tested it, then
moved it and thought the trailing comma was not useful and forgot to
test for the previous '.valid' field :/

> 
> Best regards,
> Francisco
> 
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    }
>>  };
>>  
>>  static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>> -- 
>> 2.20.1
>>