[PATCH 0/2] ppc/pnv: PNOR fixes

[PATCH 0/2] ppc/pnv: PNOR fixes

[Cédric Le Goater]

Hello,

Coverity found a couple of issues in the PnvPNOR model. Here are the
fixes.

Thanks,

C.

Cédric Le Goater (2):
  ppc/pnv: check return value of blk_pwrite()
  ppc/pnv: fix check on return value of blk_getlength()

 include/hw/ppc/pnv_pnor.h |  2 +-
 hw/ppc/pnv_pnor.c         | 10 +++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.21.1

[PATCH 1/2] ppc/pnv: check return value of blk_pwrite()

[Cédric Le Goater]

When updating the PNOR file contents, we should check for a possible
failure of blk_pwrite().

Fixes Coverity issue CID 1412228.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv_pnor.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
index bfb1e92b0392..0e86ae2feae6 100644
--- a/hw/ppc/pnv_pnor.c
+++ b/hw/ppc/pnv_pnor.c
@@ -33,6 +33,7 @@ static uint64_t pnv_pnor_read(void *opaque, hwaddr addr, unsigned size)
 static void pnv_pnor_update(PnvPnor *s, int offset, int size)
 {
     int offset_end;
+    int ret;
 
     if (s->blk) {
         return;
@@ -42,8 +43,11 @@ static void pnv_pnor_update(PnvPnor *s, int offset, int 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);
+    ret = blk_pwrite(s->blk, offset, s->storage + offset,
+                     offset_end - offset, 0);
+    if (ret < 0) {
+        error_report("Could not update PNOR: %s", strerror(-ret));
+    }
 }
 
 static void pnv_pnor_write(void *opaque, hwaddr addr, uint64_t data,
-- 
2.21.1

[PATCH 2/2] ppc/pnv: fix check on return value of blk_getlength()

[Cédric Le Goater]

blk_getlength() returns an int64_t but the result is stored in a
uint32_t. Errors (negative values) won't be caught by the check in
pnv_pnor_realize() and blk_blockalign() will allocate a very large
buffer in such cases.

Fixes Coverity issue CID 1412226.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv_pnor.h | 2 +-
 hw/ppc/pnv_pnor.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/ppc/pnv_pnor.h b/include/hw/ppc/pnv_pnor.h
index c3dd28643cae..4f96abdfb402 100644
--- a/include/hw/ppc/pnv_pnor.h
+++ b/include/hw/ppc/pnv_pnor.h
@@ -23,7 +23,7 @@ typedef struct PnvPnor {
     BlockBackend   *blk;
 
     uint8_t        *storage;
-    uint32_t       size;
+    int64_t        size;
     MemoryRegion   mmio;
 } PnvPnor;
 
diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
index 0e86ae2feae6..b061106d1c0c 100644
--- a/hw/ppc/pnv_pnor.c
+++ b/hw/ppc/pnv_pnor.c
@@ -111,7 +111,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp)
 }
 
 static Property pnv_pnor_properties[] = {
-    DEFINE_PROP_UINT32("size", PnvPnor, size, 128 << 20),
+    DEFINE_PROP_INT64("size", PnvPnor, size, 128 << 20),
     DEFINE_PROP_DRIVE("drive", PnvPnor, blk),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.21.1

Re: [PATCH 1/2] ppc/pnv: check return value of blk_pwrite()

[Greg Kurz]

On Tue,  7 Jan 2020 18:18:08 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> When updating the PNOR file contents, we should check for a possible
> failure of blk_pwrite().
> 
> Fixes Coverity issue CID 1412228.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/pnv_pnor.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> index bfb1e92b0392..0e86ae2feae6 100644
> --- a/hw/ppc/pnv_pnor.c
> +++ b/hw/ppc/pnv_pnor.c
> @@ -33,6 +33,7 @@ static uint64_t pnv_pnor_read(void *opaque, hwaddr addr, unsigned size)
>  static void pnv_pnor_update(PnvPnor *s, int offset, int size)
>  {
>      int offset_end;
> +    int ret;
>  
>      if (s->blk) {
>          return;
> @@ -42,8 +43,11 @@ static void pnv_pnor_update(PnvPnor *s, int offset, int 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);
> +    ret = blk_pwrite(s->blk, offset, s->storage + offset,
> +                     offset_end - offset, 0);
> +    if (ret < 0) {
> +        error_report("Could not update PNOR: %s", strerror(-ret));
> +    }
>  }
>  
>  static void pnv_pnor_write(void *opaque, hwaddr addr, uint64_t data,

Re: [PATCH 2/2] ppc/pnv: fix check on return value of blk_getlength()

[Greg Kurz]

On Tue,  7 Jan 2020 18:18:09 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> blk_getlength() returns an int64_t but the result is stored in a
> uint32_t. Errors (negative values) won't be caught by the check in
> pnv_pnor_realize() and blk_blockalign() will allocate a very large
> buffer in such cases.
> 
> Fixes Coverity issue CID 1412226.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/ppc/pnv_pnor.h | 2 +-
>  hw/ppc/pnv_pnor.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_pnor.h b/include/hw/ppc/pnv_pnor.h
> index c3dd28643cae..4f96abdfb402 100644
> --- a/include/hw/ppc/pnv_pnor.h
> +++ b/include/hw/ppc/pnv_pnor.h
> @@ -23,7 +23,7 @@ typedef struct PnvPnor {
>      BlockBackend   *blk;
>  
>      uint8_t        *storage;
> -    uint32_t       size;
> +    int64_t        size;
>      MemoryRegion   mmio;
>  } PnvPnor;
>  
> diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> index 0e86ae2feae6..b061106d1c0c 100644
> --- a/hw/ppc/pnv_pnor.c
> +++ b/hw/ppc/pnv_pnor.c
> @@ -111,7 +111,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp)
>  }
>  
>  static Property pnv_pnor_properties[] = {
> -    DEFINE_PROP_UINT32("size", PnvPnor, size, 128 << 20),
> +    DEFINE_PROP_INT64("size", PnvPnor, size, 128 << 20),
>      DEFINE_PROP_DRIVE("drive", PnvPnor, blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Re: [PATCH 1/2] ppc/pnv: check return value of blk_pwrite()

[Philippe Mathieu-Daudé]

On 1/7/20 6:18 PM, Cédric Le Goater wrote:
> When updating the PNOR file contents, we should check for a possible
> failure of blk_pwrite().
> 
> Fixes Coverity issue CID 1412228.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/pnv_pnor.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> index bfb1e92b0392..0e86ae2feae6 100644
> --- a/hw/ppc/pnv_pnor.c
> +++ b/hw/ppc/pnv_pnor.c
> @@ -33,6 +33,7 @@ static uint64_t pnv_pnor_read(void *opaque, hwaddr addr, unsigned size)
>   static void pnv_pnor_update(PnvPnor *s, int offset, int size)
>   {
>       int offset_end;
> +    int ret;
>   
>       if (s->blk) {
>           return;
> @@ -42,8 +43,11 @@ static void pnv_pnor_update(PnvPnor *s, int offset, int 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);
> +    ret = blk_pwrite(s->blk, offset, s->storage + offset,
> +                     offset_end - offset, 0);
> +    if (ret < 0) {
> +        error_report("Could not update PNOR: %s", strerror(-ret));

Can you report the failed offset too please?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    }
>   }
>   
>   static void pnv_pnor_write(void *opaque, hwaddr addr, uint64_t data,
> 

Re: [PATCH 2/2] ppc/pnv: fix check on return value of blk_getlength()

[Philippe Mathieu-Daudé]

On 1/7/20 6:18 PM, Cédric Le Goater wrote:
> blk_getlength() returns an int64_t but the result is stored in a
> uint32_t. Errors (negative values) won't be caught by the check in
> pnv_pnor_realize() and blk_blockalign() will allocate a very large
> buffer in such cases.
> 
> Fixes Coverity issue CID 1412226.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/ppc/pnv_pnor.h | 2 +-
>   hw/ppc/pnv_pnor.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_pnor.h b/include/hw/ppc/pnv_pnor.h
> index c3dd28643cae..4f96abdfb402 100644
> --- a/include/hw/ppc/pnv_pnor.h
> +++ b/include/hw/ppc/pnv_pnor.h
> @@ -23,7 +23,7 @@ typedef struct PnvPnor {
>       BlockBackend   *blk;
>   
>       uint8_t        *storage;
> -    uint32_t       size;
> +    int64_t        size;
>       MemoryRegion   mmio;
>   } PnvPnor;
>   
> diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> index 0e86ae2feae6..b061106d1c0c 100644
> --- a/hw/ppc/pnv_pnor.c
> +++ b/hw/ppc/pnv_pnor.c
> @@ -111,7 +111,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp)
>   }
>   
>   static Property pnv_pnor_properties[] = {
> -    DEFINE_PROP_UINT32("size", PnvPnor, size, 128 << 20),
> +    DEFINE_PROP_INT64("size", PnvPnor, size, 128 << 20),

If you respin the series please consider using '128 * MiB' here.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       DEFINE_PROP_DRIVE("drive", PnvPnor, blk),
>       DEFINE_PROP_END_OF_LIST(),
>   };
> 

Re: [PATCH 1/2] ppc/pnv: check return value of blk_pwrite()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Tue, Jan 07, 2020 at 07:22:18PM +0100, Philippe Mathieu-Daudé wrote:
> On 1/7/20 6:18 PM, Cédric Le Goater wrote:
> > When updating the PNOR file contents, we should check for a possible
> > failure of blk_pwrite().
> > 
> > Fixes Coverity issue CID 1412228.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >   hw/ppc/pnv_pnor.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> > index bfb1e92b0392..0e86ae2feae6 100644
> > --- a/hw/ppc/pnv_pnor.c
> > +++ b/hw/ppc/pnv_pnor.c
> > @@ -33,6 +33,7 @@ static uint64_t pnv_pnor_read(void *opaque, hwaddr addr, unsigned size)
> >   static void pnv_pnor_update(PnvPnor *s, int offset, int size)
> >   {
> >       int offset_end;
> > +    int ret;
> >       if (s->blk) {
> >           return;
> > @@ -42,8 +43,11 @@ static void pnv_pnor_update(PnvPnor *s, int offset, int 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);
> > +    ret = blk_pwrite(s->blk, offset, s->storage + offset,
> > +                     offset_end - offset, 0);
> > +    if (ret < 0) {
> > +        error_report("Could not update PNOR: %s", strerror(-ret));
> 
> Can you report the failed offset too please?

Since this fixes a coverity warning, I'm applying now, and extending
the error message can be a followup.

> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> > +    }
> >   }
> >   static void pnv_pnor_write(void *opaque, hwaddr addr, uint64_t data,
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] ppc/pnv: fix check on return value of blk_getlength()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On Tue, Jan 07, 2020 at 06:18:09PM +0100, Cédric Le Goater wrote:
> blk_getlength() returns an int64_t but the result is stored in a
> uint32_t. Errors (negative values) won't be caught by the check in
> pnv_pnor_realize() and blk_blockalign() will allocate a very large
> buffer in such cases.
> 
> Fixes Coverity issue CID 1412226.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-5.0.

> ---
>  include/hw/ppc/pnv_pnor.h | 2 +-
>  hw/ppc/pnv_pnor.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_pnor.h b/include/hw/ppc/pnv_pnor.h
> index c3dd28643cae..4f96abdfb402 100644
> --- a/include/hw/ppc/pnv_pnor.h
> +++ b/include/hw/ppc/pnv_pnor.h
> @@ -23,7 +23,7 @@ typedef struct PnvPnor {
>      BlockBackend   *blk;
>  
>      uint8_t        *storage;
> -    uint32_t       size;
> +    int64_t        size;
>      MemoryRegion   mmio;
>  } PnvPnor;
>  
> diff --git a/hw/ppc/pnv_pnor.c b/hw/ppc/pnv_pnor.c
> index 0e86ae2feae6..b061106d1c0c 100644
> --- a/hw/ppc/pnv_pnor.c
> +++ b/hw/ppc/pnv_pnor.c
> @@ -111,7 +111,7 @@ static void pnv_pnor_realize(DeviceState *dev, Error **errp)
>  }
>  
>  static Property pnv_pnor_properties[] = {
> -    DEFINE_PROP_UINT32("size", PnvPnor, size, 128 << 20),
> +    DEFINE_PROP_INT64("size", PnvPnor, size, 128 << 20),
>      DEFINE_PROP_DRIVE("drive", PnvPnor, blk),
>      DEFINE_PROP_END_OF_LIST(),
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]