Re: [PULL 01/88] ppc/pnv: Add a PNOR model

[Peter Maydell]

On Tue, 17 Dec 2019 at 04:43, David Gibson <address@hidden> wrote:
>
> From: Cédric Le Goater <address@hidden>
>
> On a POWERPC PowerNV system, the host firmware is stored in a PNOR
> flash chip which contents is mapped on the LPC bus. This model adds a
> simple dummy device to map the contents of a block device in the host
> address space.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/Makefile.objs      |   4 +-
>  hw/ppc/pnv.c              |  14 ++++
>  hw/ppc/pnv_pnor.c         | 135 ++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h      |   3 +
>  include/hw/ppc/pnv_pnor.h |  25 +++++++
>  5 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/pnv_pnor.c
>  create mode 100644 include/hw/ppc/pnv_pnor.h

Hi; Coverity finds some issues in this patch:

> +static void pnv_pnor_update(PnvPnor *s, int offset, int size)
> +{
> +    int offset_end;
> +
> +    if (s->blk) {
> +        return;
> +    }
> +
> +    offset_end = offset + size;
> +    offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
> +    offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> +
> +    blk_pwrite(s->blk, offset, s->storage + offset,
> +               offset_end - offset, 0);

Here we call blk_pwrite() but don't check whether it
succeeded or failed. (CID 1412228)

> +static void pnv_pnor_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvPnor *s = PNV_PNOR(dev);
> +    int ret;
> +
> +    if (s->blk) {
> +        uint64_t perm = BLK_PERM_CONSISTENT_READ |
> +                        (blk_is_read_only(s->blk) ? 0 : BLK_PERM_WRITE);
> +        ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
> +        if (ret < 0) {
> +            return;
> +        }
> +
> +        s->size = blk_getlength(s->blk);
> +        if (s->size <= 0) {

blk_getlength() returns an int64_t, but s->size is a uint32_t.
This means that this attempt to check for <= 0 doesn't
actually catch the negative values which are errors...

> +            error_setg(errp, "failed to get flash size");
> +            return;
> +        }
> +
> +        s->storage = blk_blockalign(s->blk, s->size);

...so we'll pass a very large positive number to
blk_blockalign() (since it takse a size_t argument), which
Coverity correctly identifies as doing the wrong thing.
(CID 1412226)

Side note: the blk functions here seem a bit inconsistent:
blk_getlength() returns int64_t
blk_blockalign() takes size_t
blk_pread() takes int

> +
> +        if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) {
> +            error_setg(errp, "failed to read the initial flash content");
> +            return;
> +        }
> +    } else {
> +        s->storage = blk_blockalign(NULL, s->size);
> +        memset(s->storage, 0xFF, s->size);
> +    }
> +
> +    memory_region_init_io(&s->mmio, OBJECT(s), &pnv_pnor_ops, s,
> +                          TYPE_PNV_PNOR, s->size);
> +}

thanks
-- PMM