* [Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference
@ 2019-07-05 15:08 Philippe Mathieu-Daudé
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs Philippe Mathieu-Daudé
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory Philippe Mathieu-Daudé
0 siblings, 2 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-05 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Francisco Iglesias, Alistair Francis,
Prasad J Pandit, Lei Sun, qemu-arm, Edgar E. Iglesias,
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
2019-07-05 15:08 [Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference Philippe Mathieu-Daudé
@ 2019-07-05 15:08 ` Philippe Mathieu-Daudé
2019-07-05 15:53 ` Francisco Iglesias
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory Philippe Mathieu-Daudé
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-05 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Francisco Iglesias, Alistair Francis,
Prasad J Pandit, Lei Sun, qemu-arm, Edgar E. Iglesias,
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory
2019-07-05 15:08 [Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference Philippe Mathieu-Daudé
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs Philippe Mathieu-Daudé
@ 2019-07-05 15:08 ` Philippe Mathieu-Daudé
2019-07-05 16:10 ` Francisco Iglesias
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-05 15:08 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Francisco Iglesias, Alistair Francis,
Prasad J Pandit, Lei Sun, qemu-arm, Edgar E. Iglesias,
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs Philippe Mathieu-Daudé
@ 2019-07-05 15:53 ` Francisco Iglesias
2019-07-05 17:06 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Francisco Iglesias @ 2019-07-05 15:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Alistair Francis, qemu-devel, Prasad J Pandit,
Lei Sun, qemu-arm, Edgar E. 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory Philippe Mathieu-Daudé
@ 2019-07-05 16:10 ` Francisco Iglesias
0 siblings, 0 replies; 7+ messages in thread
From: Francisco Iglesias @ 2019-07-05 16:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Alistair Francis, qemu-devel, Prasad J Pandit,
Lei Sun, qemu-arm, Edgar E. 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
2019-07-05 15:53 ` Francisco Iglesias
@ 2019-07-05 17:06 ` Philippe Mathieu-Daudé
2019-07-05 18:19 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-05 17:06 UTC (permalink / raw)
To: Francisco Iglesias
Cc: Peter Maydell, Alistair Francis, qemu-devel, Prasad J Pandit,
Lei Sun, qemu-arm, Edgar E. Iglesias
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
2019-07-05 17:06 ` Philippe Mathieu-Daudé
@ 2019-07-05 18:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-05 18:19 UTC (permalink / raw)
To: Francisco Iglesias
Cc: Peter Maydell, Alistair Francis, qemu-devel, Prasad J Pandit,
Lei Sun, KONRAD Frederic, qemu-arm, Edgar E. Iglesias
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
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-05 18:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05 15:08 [Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference Philippe Mathieu-Daudé
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs Philippe Mathieu-Daudé
2019-07-05 15:53 ` Francisco Iglesias
2019-07-05 17:06 ` Philippe Mathieu-Daudé
2019-07-05 18:19 ` Philippe Mathieu-Daudé
2019-07-05 15:08 ` [Qemu-devel] [PATCH v3 2/2] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory Philippe Mathieu-Daudé
2019-07-05 16:10 ` Francisco Iglesias
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.