1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH-for-4.1 v5 0/3] hw/ssi/xilinx_spips: Avoid NULL dereference and out-of-bound access
  4. View patch

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

Philippe Mathieu-Daudé posted 3 patches 6 years, 7 months ago
Maintainers: Alistair Francis <alistair@alistair23.me>
[Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
Both lqspi_read() and lqspi_load_cache() expect a 32-bit
aligned address.

From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:

  Transfer Size Limitations

    Because of the 32-bit wide TX, RX, and generic FIFO, all
    APB/AXI transfers must be an integer multiple of 4-bytes.
    Shorter transfers are not possible.

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.

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
---
 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..b29e0a4a89 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
     .read_with_attrs = lqspi_read,
     .write_with_attrs = lqspi_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
     .valid = {
         .min_access_size = 1,
         .max_access_size = 4
-- 
2.20.1
Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Francisco Iglesias 6 years, 7 months ago 
Hi Philippe,

On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
> aligned address.
> 
> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:

s/22/24/

After above change:

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

(I tested all three patches so the Tested-by tag can also be appended on the
other two)

Best regards,
Francisco Iglesias

> 
>   Transfer Size Limitations
> 
>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>     APB/AXI transfers must be an integer multiple of 4-bytes.
>     Shorter transfers are not possible.
> 
> 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.
> 
> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
> ---
>  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..b29e0a4a89 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>      .read_with_attrs = lqspi_read,
>      .write_with_attrs = lqspi_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
>      .valid = {
>          .min_access_size = 1,
>          .max_access_size = 4
> -- 
> 2.20.1
>
Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
Hi Francisco,

On 7/8/19 4:26 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
>> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
>> aligned address.
>>
>> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
> 
> s/22/24/

I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
datasheet is not stable to refer to. Maybe I should simply use:

  From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':

> 
> After above change:
> 
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
> Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> (I tested all three patches so the Tested-by tag can also be appended on the
> other two)

Thanks!

> 
> Best regards,
> Francisco Iglesias
> 
>>
>>   Transfer Size Limitations
>>
>>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>>     APB/AXI transfers must be an integer multiple of 4-bytes.
>>     Shorter transfers are not possible.
>>
>> 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.
>>
>> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
>> ---
>>  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..b29e0a4a89 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>>      .read_with_attrs = lqspi_read,
>>      .write_with_attrs = lqspi_write,
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>>      .valid = {
>>          .min_access_size = 1,
>>          .max_access_size = 4
>> -- 
>> 2.20.1
>>
Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Francisco Iglesias 6 years, 7 months ago 
Hi Philippe,

On [2019 Jul 08] Mon 16:58:29, Philippe Mathieu-Daudé wrote:
> Hi Francisco,
> 
> On 7/8/19 4:26 PM, Francisco Iglesias wrote:
> > Hi Philippe,
> > 
> > On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
> >> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
> >> aligned address.
> >>
> >> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
> > 
> > s/22/24/
> 
> I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
> datasheet is not stable to refer to. Maybe I should simply use:
> 
>   From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':

I just clicked on the link and ended up into this version of the UG1085:

'UG1085 (v1.9) January 17, 2019'

But your right its probably better to refer to a specific version of the
UG1085 instead? Then both should be ok, either to write the way you
suggest above or refer to the chapter number in that UG1085 version (as it
was before).

Best regards,
Francisco

> 
> > 
> > After above change:
> > 
> > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
> > Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> > 
> > (I tested all three patches so the Tested-by tag can also be appended on the
> > other two)
> 
> Thanks!
> 
> > 
> > Best regards,
> > Francisco Iglesias
> > 
> >>
> >>   Transfer Size Limitations
> >>
> >>     Because of the 32-bit wide TX, RX, and generic FIFO, all
> >>     APB/AXI transfers must be an integer multiple of 4-bytes.
> >>     Shorter transfers are not possible.
> >>
> >> 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.
> >>
> >> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
> >> ---
> >>  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..b29e0a4a89 100644
> >> --- a/hw/ssi/xilinx_spips.c
> >> +++ b/hw/ssi/xilinx_spips.c
> >> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
> >>      .read_with_attrs = lqspi_read,
> >>      .write_with_attrs = lqspi_write,
> >>      .endianness = DEVICE_NATIVE_ENDIAN,
> >> +    .impl = {
> >> +        .min_access_size = 4,
> >> +        .max_access_size = 4,
> >> +    },
> >>      .valid = {
> >>          .min_access_size = 1,
> >>          .max_access_size = 4
> >> -- 
> >> 2.20.1
> >>
Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
Hi Peter,

On 7/8/19 6:03 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 08] Mon 16:58:29, Philippe Mathieu-Daudé wrote:
>> Hi Francisco,
>>
>> On 7/8/19 4:26 PM, Francisco Iglesias wrote:
>>> Hi Philippe,
>>>
>>> On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
>>>> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
>>>> aligned address.
>>>>
>>>> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
>>>
>>> s/22/24/
>>
>> I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
>> datasheet is not stable to refer to. Maybe I should simply use:
>>
>>   From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':
> 
> I just clicked on the link and ended up into this version of the UG1085:
> 
> 'UG1085 (v1.9) January 17, 2019'
> 
> But your right its probably better to refer to a specific version of the
> UG1085 instead? Then both should be ok, either to write the way you
> suggest above or refer to the chapter number in that UG1085 version (as it
> was before).
> 
> Best regards,
> Francisco
> 
>>
>>>
>>> After above change:
>>>
>>> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
>>> Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>>>
>>> (I tested all three patches so the Tested-by tag can also be appended on the
>>> other two)

Are you OK to take this series via your ARM tree?

If so, do you want me to respin fixing the comment and adding Francisco
tags?

Thanks,

Phil.

>>
>> Thanks!
>>
>>>
>>> Best regards,
>>> Francisco Iglesias
>>>
>>>>
>>>>   Transfer Size Limitations
>>>>
>>>>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>>>>     APB/AXI transfers must be an integer multiple of 4-bytes.
>>>>     Shorter transfers are not possible.
>>>>
>>>> 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.
>>>>
>>>> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
>>>> ---
>>>>  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..b29e0a4a89 100644
>>>> --- a/hw/ssi/xilinx_spips.c
>>>> +++ b/hw/ssi/xilinx_spips.c
>>>> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>>>>      .read_with_attrs = lqspi_read,
>>>>      .write_with_attrs = lqspi_write,
>>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>> +    .impl = {
>>>> +        .min_access_size = 4,
>>>> +        .max_access_size = 4,
>>>> +    },
>>>>      .valid = {
>>>>          .min_access_size = 1,
>>>>          .max_access_size = 4
>>>> -- 
>>>> 2.20.1
>>>>
Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]
Posted by Peter Maydell 6 years, 7 months ago 
On Tue, 9 Jul 2019 at 11:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Are you OK to take this series via your ARM tree?
>
> If so, do you want me to respin fixing the comment and adding Francisco
> tags?

Yes, and yes please.

thanks
-- PMM

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.