| Message ID | 20190705202500.18853-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ssi/xilinx_spips: Avoid NULL pointer deference | expand |
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 >
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 >>
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)
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(+)