[Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference

[Qemu-devel] [PATCH v3 0/2] 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: Return MEMTX_ERROR instead of MEMTX_DECODE_ERROR

Philippe Mathieu-Daudé (2):
  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.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/2] 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>
---
 hw/ssi/xilinx_spips.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 8115bb6d46..e80619aece 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1202,27 +1202,27 @@ 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;
+        *value = cpu_to_le32(*(uint32_t *)retp);
+        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
+                   addr, *value);
     } else {
         lqspi_load_cache(opaque, addr);
-        return lqspi_read(opaque, addr, size);
+        lqspi_read(opaque, addr, value, size, attrs);
     }
+
+    return MEMTX_OK;
 }
 
 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 v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

[Philippe Mathieu-Daudé]

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 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>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 e80619aece..4c0b0aa3c9 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1221,8 +1221,24 @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
     return MEMTX_OK;
 }
 
+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

Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Francisco Iglesias]

Hi Philippe,

On [2019 Jul 05] Fri 17:08:49, 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>
> ---
>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..e80619aece 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,27 +1202,27 @@ 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;
> +        *value = cpu_to_le32(*(uint32_t *)retp);
> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
> +                   addr, *value);
>      } else {
>          lqspi_load_cache(opaque, addr);
> -        return lqspi_read(opaque, addr, size);
> +        lqspi_read(opaque, addr, value, size, attrs);

If you don't want to leave the return value floating you can always keep the
'return' (I'm unsure if coverity will complain about that).

Either way:

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


>      }
> +
> +    return MEMTX_OK;
>  }
>  
>  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 v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

[Francisco Iglesias]

Hi Philippe,

On [2019 Jul 05] Fri 17:08:50, Philippe Mathieu-Daudé wrote:
> Lei Sun found while auditing the code than a CPU write would

s/than/that/

> trigger a NULL pointer deference.

s/deference/dereference/

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

s/External/AXI/

> 
> Fix by implementing the write_with_attrs() handler.
> Return MEMTX_ERROR  when the region is accessed  (this error maps

There is an extra whitespace after 'MEMTX_ERROR' and also after
'accessed'.

Sorry for not mentioning above before, after correcting this cosmetica:

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

Best regards,
Francisco Iglesias

> 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>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  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 e80619aece..4c0b0aa3c9 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1221,8 +1221,24 @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
>      return MEMTX_OK;
>  }
>  
> +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
> 

Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

On 7/5/19 5:53 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 05] Fri 17:08:49, 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>
>> ---
>>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index 8115bb6d46..e80619aece 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1202,27 +1202,27 @@ 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;
>> +        *value = cpu_to_le32(*(uint32_t *)retp);
>> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
>> +                   addr, *value);
>>      } else {
>>          lqspi_load_cache(opaque, addr);
>> -        return lqspi_read(opaque, addr, size);
>> +        lqspi_read(opaque, addr, value, size, attrs);
> 
> If you don't want to leave the return value floating you can always keep the
> 'return' (I'm unsure if coverity will complain about that).

Ah, I missed that, I'll fix.

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

Thanks!

I'll wait some more time of other want to review, then I'll respin with
the typo you corrected in the 2nd patch fixed.

> 
>>      }
>> +
>> +    return MEMTX_OK;
>>  }
>>  
>>  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 v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

On 7/5/19 7:06 PM, Philippe Mathieu-Daudé wrote:
> On 7/5/19 5:53 PM, Francisco Iglesias wrote:
>> Hi Philippe,
>>
>> On [2019 Jul 05] Fri 17:08:49, 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>
>>> ---
>>>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>>> index 8115bb6d46..e80619aece 100644
>>> --- a/hw/ssi/xilinx_spips.c
>>> +++ b/hw/ssi/xilinx_spips.c
>>> @@ -1202,27 +1202,27 @@ 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) {

Looking at Frederic's commit 252b99baeb9, it seems this "4" has to be
replaced by "size", or cleaner, use .impl.min_access_size = 4.

Currently we have:

static const MemoryRegionOps lqspi_ops = {
    .valid = {
        .min_access_size = 1,
        .max_access_size = 4
    }
};

If we use:

- size = 1
- addr = LQSPI_CACHE_SIZE - 1

We have

'addr >= q->lqspi_cached_addr': true
'addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4': true

>>>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
>>> -        ret = cpu_to_le32(*(uint32_t *)retp);

Are we reading 3 extra bytes?

>>> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
>>> -                   (unsigned)ret);
>>> -        return ret;
>>> +        *value = cpu_to_le32(*(uint32_t *)retp);
>>> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
>>> +                   addr, *value);
>>>      } else {
>>>          lqspi_load_cache(opaque, addr);
>>> -        return lqspi_read(opaque, addr, size);
>>> +        lqspi_read(opaque, addr, value, size, attrs);
>>
>> If you don't want to leave the return value floating you can always keep the
>> 'return' (I'm unsure if coverity will complain about that).
> 
> Ah, I missed that, I'll fix.
> 
>>
>> Either way:
>>
>> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> Thanks!
> 
> I'll wait some more time of other want to review, then I'll respin with
> the typo you corrected in the 2nd patch fixed.
> 
>>
>>>      }
>>> +
>>> +    return MEMTX_OK;
>>>  }
>>>  
>>>  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
>>>