diff mbox series

hw/ssi/npcm7xx_fiu: Fix handling of unsigned integer

Message ID 20200919132435.310527-1-f4bug@amsat.org
State New
Headers show
Series hw/ssi/npcm7xx_fiu: Fix handling of unsigned integer | expand

Commit Message

Philippe Mathieu-Daudé Sept. 19, 2020, 1:24 p.m. UTC 
Fix integer handling issues handling issue reported by Coverity:

  hw/ssi/npcm7xx_fiu.c: 162 in npcm7xx_fiu_flash_read()
  >>>     CID 1432730:  Integer handling issues  (NEGATIVE_RETURNS)
  >>>     "npcm7xx_fiu_cs_index(fiu, f)" is passed to a parameter that cannot be negative.
  162         npcm7xx_fiu_select(fiu, npcm7xx_fiu_cs_index(fiu, f));

  hw/ssi/npcm7xx_fiu.c: 221 in npcm7xx_fiu_flash_write()
  218         cs_id = npcm7xx_fiu_cs_index(fiu, f);
  219         trace_npcm7xx_fiu_flash_write(DEVICE(fiu)->canonical_path, cs_id, addr,
  220                                       size, v);
  >>>     CID 1432729:  Integer handling issues  (NEGATIVE_RETURNS)
  >>>     "cs_id" is passed to a parameter that cannot be negative.
  221         npcm7xx_fiu_select(fiu, cs_id);

Since the index of the flash can not be negative, return an
unsigned type.

Reported-by: Coverity (CID 1432729 & 1432730: NEGATIVE_RETURNS)
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ssi/npcm7xx_fiu.c | 12 ++++++------
 hw/ssi/trace-events  |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Havard Skinnemoen Sept. 19, 2020, 5:07 p.m. UTC | #1 
On Sat, Sep 19, 2020 at 6:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Fix integer handling issues handling issue reported by Coverity:
>
>   hw/ssi/npcm7xx_fiu.c: 162 in npcm7xx_fiu_flash_read()
>   >>>     CID 1432730:  Integer handling issues  (NEGATIVE_RETURNS)
>   >>>     "npcm7xx_fiu_cs_index(fiu, f)" is passed to a parameter that cannot be negative.
>   162         npcm7xx_fiu_select(fiu, npcm7xx_fiu_cs_index(fiu, f));
>
>   hw/ssi/npcm7xx_fiu.c: 221 in npcm7xx_fiu_flash_write()
>   218         cs_id = npcm7xx_fiu_cs_index(fiu, f);
>   219         trace_npcm7xx_fiu_flash_write(DEVICE(fiu)->canonical_path, cs_id, addr,
>   220                                       size, v);
>   >>>     CID 1432729:  Integer handling issues  (NEGATIVE_RETURNS)
>   >>>     "cs_id" is passed to a parameter that cannot be negative.
>   221         npcm7xx_fiu_select(fiu, cs_id);
>
> Since the index of the flash can not be negative, return an
> unsigned type.
>
> Reported-by: Coverity (CID 1432729 & 1432730: NEGATIVE_RETURNS)
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>

Thanks!

> ---
>  hw/ssi/npcm7xx_fiu.c | 12 ++++++------
>  hw/ssi/trace-events  |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
> index 104e8f2b963..5040132b074 100644
> --- a/hw/ssi/npcm7xx_fiu.c
> +++ b/hw/ssi/npcm7xx_fiu.c
> @@ -103,7 +103,8 @@ enum NPCM7xxFIURegister {
>   * Returns the index of flash in the fiu->flash array. This corresponds to the
>   * chip select ID of the flash.
>   */
> -static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
> +static unsigned npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu,
> +                                     NPCM7xxFIUFlash *flash)
>  {
>      int index = flash - fiu->flash;
>
> @@ -113,20 +114,19 @@ static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
>  }
>
>  /* Assert the chip select specified in the UMA Control/Status Register. */
> -static void npcm7xx_fiu_select(NPCM7xxFIUState *s, int cs_id)
> +static void npcm7xx_fiu_select(NPCM7xxFIUState *s, unsigned cs_id)
>  {
>      trace_npcm7xx_fiu_select(DEVICE(s)->canonical_path, cs_id);
>
>      if (cs_id < s->cs_count) {
>          qemu_irq_lower(s->cs_lines[cs_id]);
> +        s->active_cs = cs_id;
>      } else {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: UMA to CS%d; this module has only %d chip selects",
>                        DEVICE(s)->canonical_path, cs_id, s->cs_count);
> -        cs_id = -1;
> +        s->active_cs = -1;
>      }
> -
> -    s->active_cs = cs_id;
>  }
>
>  /* Deassert the currently active chip select. */
> @@ -206,7 +206,7 @@ static void npcm7xx_fiu_flash_write(void *opaque, hwaddr addr, uint64_t v,
>      NPCM7xxFIUFlash *f = opaque;
>      NPCM7xxFIUState *fiu = f->fiu;
>      uint32_t dwr_cfg;
> -    int cs_id;
> +    unsigned cs_id;
>      int i;
>
>      if (fiu->active_cs != -1) {
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 2f83ef833fb..612d3d6087a 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -19,4 +19,4 @@ npcm7xx_fiu_deselect(const char *id, int cs) "%s deselect CS%d"
>  npcm7xx_fiu_ctrl_read(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>  npcm7xx_fiu_ctrl_write(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>  npcm7xx_fiu_flash_read(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
> -npcm7xx_fiu_flash_write(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
> +npcm7xx_fiu_flash_write(const char *id, unsigned cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
> --
> 2.26.2
>
Philippe Mathieu-Daudé Oct. 5, 2020, 7:43 a.m. UTC | #2 
Hi Peter,

Can you take this patch via your qemu-arm tree?

On 9/19/20 3:24 PM, Philippe Mathieu-Daudé wrote:
> Fix integer handling issues handling issue reported by Coverity:
> 
>   hw/ssi/npcm7xx_fiu.c: 162 in npcm7xx_fiu_flash_read()
>   >>>     CID 1432730:  Integer handling issues  (NEGATIVE_RETURNS)
>   >>>     "npcm7xx_fiu_cs_index(fiu, f)" is passed to a parameter that cannot be negative.
>   162         npcm7xx_fiu_select(fiu, npcm7xx_fiu_cs_index(fiu, f));
> 
>   hw/ssi/npcm7xx_fiu.c: 221 in npcm7xx_fiu_flash_write()
>   218         cs_id = npcm7xx_fiu_cs_index(fiu, f);
>   219         trace_npcm7xx_fiu_flash_write(DEVICE(fiu)->canonical_path, cs_id, addr,
>   220                                       size, v);
>   >>>     CID 1432729:  Integer handling issues  (NEGATIVE_RETURNS)
>   >>>     "cs_id" is passed to a parameter that cannot be negative.
>   221         npcm7xx_fiu_select(fiu, cs_id);
> 
> Since the index of the flash can not be negative, return an
> unsigned type.
> 
> Reported-by: Coverity (CID 1432729 & 1432730: NEGATIVE_RETURNS)
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/ssi/npcm7xx_fiu.c | 12 ++++++------
>  hw/ssi/trace-events  |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
> index 104e8f2b963..5040132b074 100644
> --- a/hw/ssi/npcm7xx_fiu.c
> +++ b/hw/ssi/npcm7xx_fiu.c
> @@ -103,7 +103,8 @@ enum NPCM7xxFIURegister {
>   * Returns the index of flash in the fiu->flash array. This corresponds to the
>   * chip select ID of the flash.
>   */
> -static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
> +static unsigned npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu,
> +                                     NPCM7xxFIUFlash *flash)
>  {
>      int index = flash - fiu->flash;
>  
> @@ -113,20 +114,19 @@ static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
>  }
>  
>  /* Assert the chip select specified in the UMA Control/Status Register. */
> -static void npcm7xx_fiu_select(NPCM7xxFIUState *s, int cs_id)
> +static void npcm7xx_fiu_select(NPCM7xxFIUState *s, unsigned cs_id)
>  {
>      trace_npcm7xx_fiu_select(DEVICE(s)->canonical_path, cs_id);
>  
>      if (cs_id < s->cs_count) {
>          qemu_irq_lower(s->cs_lines[cs_id]);
> +        s->active_cs = cs_id;
>      } else {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: UMA to CS%d; this module has only %d chip selects",
>                        DEVICE(s)->canonical_path, cs_id, s->cs_count);
> -        cs_id = -1;
> +        s->active_cs = -1;
>      }
> -
> -    s->active_cs = cs_id;
>  }
>  
>  /* Deassert the currently active chip select. */
> @@ -206,7 +206,7 @@ static void npcm7xx_fiu_flash_write(void *opaque, hwaddr addr, uint64_t v,
>      NPCM7xxFIUFlash *f = opaque;
>      NPCM7xxFIUState *fiu = f->fiu;
>      uint32_t dwr_cfg;
> -    int cs_id;
> +    unsigned cs_id;
>      int i;
>  
>      if (fiu->active_cs != -1) {
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 2f83ef833fb..612d3d6087a 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -19,4 +19,4 @@ npcm7xx_fiu_deselect(const char *id, int cs) "%s deselect CS%d"
>  npcm7xx_fiu_ctrl_read(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>  npcm7xx_fiu_ctrl_write(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>  npcm7xx_fiu_flash_read(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
> -npcm7xx_fiu_flash_write(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
> +npcm7xx_fiu_flash_write(const char *id, unsigned cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
>
Peter Maydell Oct. 8, 2020, 10:45 a.m. UTC | #3 
On Mon, 5 Oct 2020 at 08:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> Can you take this patch via your qemu-arm tree?

Sure.

Applied to target-arm.next, thanks.

-- PMM
diff mbox series

Patch

diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
index 104e8f2b963..5040132b074 100644
--- a/hw/ssi/npcm7xx_fiu.c
+++ b/hw/ssi/npcm7xx_fiu.c
@@ -103,7 +103,8 @@  enum NPCM7xxFIURegister {
  * Returns the index of flash in the fiu->flash array. This corresponds to the
  * chip select ID of the flash.
  */
-static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
+static unsigned npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu,
+                                     NPCM7xxFIUFlash *flash)
 {
     int index = flash - fiu->flash;
 
@@ -113,20 +114,19 @@  static int npcm7xx_fiu_cs_index(NPCM7xxFIUState *fiu, NPCM7xxFIUFlash *flash)
 }
 
 /* Assert the chip select specified in the UMA Control/Status Register. */
-static void npcm7xx_fiu_select(NPCM7xxFIUState *s, int cs_id)
+static void npcm7xx_fiu_select(NPCM7xxFIUState *s, unsigned cs_id)
 {
     trace_npcm7xx_fiu_select(DEVICE(s)->canonical_path, cs_id);
 
     if (cs_id < s->cs_count) {
         qemu_irq_lower(s->cs_lines[cs_id]);
+        s->active_cs = cs_id;
     } else {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: UMA to CS%d; this module has only %d chip selects",
                       DEVICE(s)->canonical_path, cs_id, s->cs_count);
-        cs_id = -1;
+        s->active_cs = -1;
     }
-
-    s->active_cs = cs_id;
 }
 
 /* Deassert the currently active chip select. */
@@ -206,7 +206,7 @@  static void npcm7xx_fiu_flash_write(void *opaque, hwaddr addr, uint64_t v,
     NPCM7xxFIUFlash *f = opaque;
     NPCM7xxFIUState *fiu = f->fiu;
     uint32_t dwr_cfg;
-    int cs_id;
+    unsigned cs_id;
     int i;
 
     if (fiu->active_cs != -1) {
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index 2f83ef833fb..612d3d6087a 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -19,4 +19,4 @@  npcm7xx_fiu_deselect(const char *id, int cs) "%s deselect CS%d"
 npcm7xx_fiu_ctrl_read(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
 npcm7xx_fiu_ctrl_write(const char *id, uint64_t addr, uint32_t data) "%s offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
 npcm7xx_fiu_flash_read(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
-npcm7xx_fiu_flash_write(const char *id, int cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64
+npcm7xx_fiu_flash_write(const char *id, unsigned cs, uint64_t addr, unsigned int size, uint64_t value) "%s[%d] offset: 0x%08" PRIx64 " size: %u value: 0x%" PRIx64