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