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

[Cédric Le Goater]

On 1/7/20 3:43 PM, Peter Maydell wrote:
> 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)

Yes. I will send fixes for both issues.

Thanks,

C. 

>> +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
>