[PATCH 0/7] block: Miscellaneous minor Coverity fixes

[PATCH 0/7] block: Miscellaneous minor Coverity fixes

[Peter Maydell]

This patchset is a collection of fixes for minor Coverity reported
issues. In all cases, there isn't a user-visible problem, but
the Coverity issue pointed up somewhere where we could clean
up the code or make it a bit more obvious to a human reader
what the intent was.

Only lightly tested (with "make check" and "make check-avocado").

thanks
-- PMM

Peter Maydell (7):
  block/vdi.c: Avoid potential overflow when calculating size of write
  block/gluster: Use g_autofree for string in qemu_gluster_parse_json()
  hw/block/pflash_cfi01: Don't decrement pfl->counter below 0
  hw/ide/atapi: Be explicit that assigning to s->lcyl truncates
  hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found
    something
  hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()
  block/ssh.c: Don't double-check that characters are hex digits

 block/gluster.c         |  6 +-----
 block/ssh.c             | 10 ++++++----
 block/vdi.c             |  2 +-
 hw/block/fdc-isa.c      |  2 ++
 hw/block/pflash_cfi01.c |  1 +
 hw/ide/atapi.c          |  2 +-
 hw/ide/pci.c            |  4 ----
 7 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/7] block/vdi.c: Avoid potential overflow when calculating size of write

[Peter Maydell]

In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
get the size to write in bytes. Coverity notes that this means that
we do the multiply as a 32x32->32 multiply before converting to
64 bits, which has the potential to overflow.

This is very unlikely to happen, since the block map has 4 bytes per
block and the maximum number of blocks in the image must fit into a
32-bit integer.  But we can keep Coverity happy by including a cast
so we do a 64-bit multiply here.

Resolves: Coverity CID 1508076
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 block/vdi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6363da08cee..27c60ba18d0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -728,7 +728,7 @@ nonallocating_write:
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
         ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
-                             n_sectors * SECTOR_SIZE, base, 0);
+                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
     }
 
     return ret;
-- 
2.34.1

Re: [PATCH 1/7] block/vdi.c: Avoid potential overflow when calculating size of write

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
> get the size to write in bytes. Coverity notes that this means that
> we do the multiply as a 32x32->32 multiply before converting to
> 64 bits, which has the potential to overflow.
> 
> This is very unlikely to happen, since the block map has 4 bytes per
> block and the maximum number of blocks in the image must fit into a
> 32-bit integer.  But we can keep Coverity happy by including a cast
> so we do a 64-bit multiply here.
> 
> Resolves: Coverity CID 1508076
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  block/vdi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 6363da08cee..27c60ba18d0 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -728,7 +728,7 @@ nonallocating_write:
>          logout("will write %u block map sectors starting from entry %u\n",
>                 n_sectors, bmap_first);
>          ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
> -                             n_sectors * SECTOR_SIZE, base, 0);
> +                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
>      }

I wonder if we shouldn't just make VDI's SECTOR_SIZE 64 bits like
BDRV_SECTOR_SIZE. It's easy to miss the cast in individual places.

Kevin

Re: [PATCH 1/7] block/vdi.c: Avoid potential overflow when calculating size of write

[Stefan Weil via]

Am 31.07.24 um 16:36 schrieb Peter Maydell:

> In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
> get the size to write in bytes. Coverity notes that this means that
> we do the multiply as a 32x32->32 multiply before converting to
> 64 bits, which has the potential to overflow.
>
> This is very unlikely to happen, since the block map has 4 bytes per
> block and the maximum number of blocks in the image must fit into a
> 32-bit integer.  But we can keep Coverity happy by including a cast
> so we do a 64-bit multiply here.
>
> Resolves: Coverity CID 1508076
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   block/vdi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 6363da08cee..27c60ba18d0 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -728,7 +728,7 @@ nonallocating_write:
>           logout("will write %u block map sectors starting from entry %u\n",
>                  n_sectors, bmap_first);
>           ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
> -                             n_sectors * SECTOR_SIZE, base, 0);
> +                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
>       }
>   
>       return ret;

Thanks.

Reviewed-by: Stefan Weil <sw@weilnetz.de>

[PATCH 2/7] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Peter Maydell]

In the loop in qemu_gluster_parse_json() we do:

    char *str = NULL;
    for(...) {
        str = g_strdup_printf(...);
        ...
        if (various errors) {
            goto out;
        }
        ...
        g_free(str);
        str = NULL;
    }
    return 0;
out:
    various cleanups;
    g_free(str);
    ...
    return -errno;

Coverity correctly complains that the assignment "str = NULL" at the
end of the loop is unnecessary, because we will either go back to the
top of the loop and overwrite it, or else we will exit the loop and
then exit the function without ever reading str again. The assignment
is there as defensive coding to ensure that str is only non-NULL if
it's a live allocation, so this is intentional.

We can make Coverity happier and simplify the code here by using
g_autofree, since we never need 'str' outside the loop.

Resolves: Coverity CID 1527385
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 block/gluster.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index f8b415f3812..61ded95e660 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -514,7 +514,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     SocketAddressList **tail;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
-    char *str = NULL;
     const char *ptr;
     int i, type, num_servers;
 
@@ -547,7 +546,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     tail = &gconf->server;
 
     for (i = 0; i < num_servers; i++) {
-        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
+        g_autofree char *str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
         qdict_extract_subqdict(options, &backing_options, str);
 
         /* create opts info from runtime_type_opts list */
@@ -658,8 +657,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         qobject_unref(backing_options);
         backing_options = NULL;
-        g_free(str);
-        str = NULL;
     }
 
     return 0;
@@ -668,7 +665,6 @@ out:
     error_propagate(errp, local_err);
     qapi_free_SocketAddress(gsconf);
     qemu_opts_del(opts);
-    g_free(str);
     qobject_unref(backing_options);
     errno = EINVAL;
     return -errno;
-- 
2.34.1

Re: [PATCH 2/7] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In the loop in qemu_gluster_parse_json() we do:
> 
>     char *str = NULL;
>     for(...) {
>         str = g_strdup_printf(...);
>         ...
>         if (various errors) {
>             goto out;
>         }
>         ...
>         g_free(str);
>         str = NULL;
>     }
>     return 0;
> out:
>     various cleanups;
>     g_free(str);
>     ...
>     return -errno;
> 
> Coverity correctly complains that the assignment "str = NULL" at the
> end of the loop is unnecessary, because we will either go back to the
> top of the loop and overwrite it, or else we will exit the loop and
> then exit the function without ever reading str again. The assignment
> is there as defensive coding to ensure that str is only non-NULL if
> it's a live allocation, so this is intentional.
> 
> We can make Coverity happier and simplify the code here by using
> g_autofree, since we never need 'str' outside the loop.
> 
> Resolves: Coverity CID 1527385
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  block/gluster.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index f8b415f3812..61ded95e660 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -514,7 +514,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>      SocketAddressList **tail;
>      QDict *backing_options = NULL;
>      Error *local_err = NULL;
> -    char *str = NULL;
>      const char *ptr;
>      int i, type, num_servers;
>  
> @@ -547,7 +546,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
>      tail = &gconf->server;
>  
>      for (i = 0; i < num_servers; i++) {
> -        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
> +        g_autofree char *str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);

This line is too long now.

With this fixed:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 2/7] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Philippe Mathieu-Daudé]

On 31/7/24 16:36, Peter Maydell wrote:
> In the loop in qemu_gluster_parse_json() we do:
> 
>      char *str = NULL;
>      for(...) {
>          str = g_strdup_printf(...);
>          ...
>          if (various errors) {
>              goto out;
>          }
>          ...
>          g_free(str);
>          str = NULL;
>      }
>      return 0;
> out:
>      various cleanups;
>      g_free(str);
>      ...
>      return -errno;
> 
> Coverity correctly complains that the assignment "str = NULL" at the
> end of the loop is unnecessary, because we will either go back to the
> top of the loop and overwrite it, or else we will exit the loop and
> then exit the function without ever reading str again. The assignment
> is there as defensive coding to ensure that str is only non-NULL if
> it's a live allocation, so this is intentional.
> 
> We can make Coverity happier and simplify the code here by using
> g_autofree, since we never need 'str' outside the loop.
> 
> Resolves: Coverity CID 1527385
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   block/gluster.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/7] hw/block/pflash_cfi01: Don't decrement pfl->counter below 0

[Peter Maydell]

In pflash_write() Coverity points out that we can decrement the
unsigned pfl->counter below zero, which makes it wrap around.  In
fact this is harmless, because if pfl->counter is 0 at this point we
also increment pfl->wcycle to 3, and the wcycle == 3 handling doesn't
look at counter; the only way back into code which looks at the
counter value is via wcycle == 1, which will reinitialize the counter.
But it's arguably a little clearer to break early in the "counter ==
0" if(), to avoid the decrement-below-zero.

Resolves: Coverity CID 1547611
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/pflash_cfi01.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index c8f1cf5a872..2f3d1dd509c 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -614,6 +614,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
             if (!pfl->counter) {
                 trace_pflash_write(pfl->name, "block write finished");
                 pfl->wcycle++;
+                break;
             }
 
             pfl->counter--;
-- 
2.34.1

Re: [PATCH 3/7] hw/block/pflash_cfi01: Don't decrement pfl->counter below 0

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In pflash_write() Coverity points out that we can decrement the
> unsigned pfl->counter below zero, which makes it wrap around.  In
> fact this is harmless, because if pfl->counter is 0 at this point we
> also increment pfl->wcycle to 3, and the wcycle == 3 handling doesn't
> look at counter; the only way back into code which looks at the
> counter value is via wcycle == 1, which will reinitialize the counter.
> But it's arguably a little clearer to break early in the "counter ==
> 0" if(), to avoid the decrement-below-zero.
> 
> Resolves: Coverity CID 1547611
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 3/7] hw/block/pflash_cfi01: Don't decrement pfl->counter below 0

[Philippe Mathieu-Daudé]

On 31/7/24 16:36, Peter Maydell wrote:
> In pflash_write() Coverity points out that we can decrement the
> unsigned pfl->counter below zero, which makes it wrap around.  In
> fact this is harmless, because if pfl->counter is 0 at this point we
> also increment pfl->wcycle to 3, and the wcycle == 3 handling doesn't
> look at counter; the only way back into code which looks at the
> counter value is via wcycle == 1, which will reinitialize the counter.
> But it's arguably a little clearer to break early in the "counter ==
> 0" if(), to avoid the decrement-below-zero.
> 
> Resolves: Coverity CID 1547611
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/block/pflash_cfi01.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/7] hw/ide/atapi: Be explicit that assigning to s->lcyl truncates

[Peter Maydell]

In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then
assign its two halves to s->lcyl and s->hcyl like this:

           s->lcyl = size;
           s->hcyl = size >> 8;

Coverity warns that the first line here can overflow the
8-bit s->lcyl variable. This is true, and in this case we're
deliberately only after the low 8 bits of the value. The
code is clearer to both humans and Coverity if we're explicit
that we only wanted the low 8 bits, though.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ide/atapi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index fcb6cca1573..e82959dc2d3 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
                     byte_count_limit--;
                 size = byte_count_limit;
             }
-            s->lcyl = size;
+            s->lcyl = size & 0xff;
             s->hcyl = size >> 8;
             s->elementary_transfer_size = size;
             /* we cannot transmit more than one sector at a time */
-- 
2.34.1

Re: [PATCH 4/7] hw/ide/atapi: Be explicit that assigning to s->lcyl truncates

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then
> assign its two halves to s->lcyl and s->hcyl like this:
>
>            s->lcyl = size;
>            s->hcyl = size >> 8;
>
> Coverity warns that the first line here can overflow the
> 8-bit s->lcyl variable. This is true, and in this case we're
> deliberately only after the low 8 bits of the value. The
> code is clearer to both humans and Coverity if we're explicit
> that we only wanted the low 8 bits, though.
>

Resolves?

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/ide/atapi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index fcb6cca1573..e82959dc2d3 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
               /* a new transfer is needed */
               s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
               ide_bus_set_irq(s->bus);
               byte_count_limit = atapi_byte_count_limit(s);
               trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
               size = s->packet_transfer_size;
               if (size > byte_count_limit) {
                   /* byte count limit must be even if this case */
                   if (byte_count_limit & 1)
>                      byte_count_limit--;
>                  size = byte_count_limit;
>              }
> -            s->lcyl = size;
> +            s->lcyl = size & 0xff;
>              s->hcyl = size >> 8;
>              s->elementary_transfer_size = size;
>              /* we cannot transmit more than one sector at a time */

@size is int.  I wonder why it's fine with size >> 8...  ah,
atapi_byte_count_limit() returns uint16_t, and Coverity is smart enough
to data-flow this via @byte_count_limit into @size.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 4/7] hw/ide/atapi: Be explicit that assigning to s->lcyl truncates

[Peter Maydell]

On Wed, 31 Jul 2024 at 15:47, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then
> > assign its two halves to s->lcyl and s->hcyl like this:
> >
> >            s->lcyl = size;
> >            s->hcyl = size >> 8;
> >
> > Coverity warns that the first line here can overflow the
> > 8-bit s->lcyl variable. This is true, and in this case we're
> > deliberately only after the low 8 bits of the value. The
> > code is clearer to both humans and Coverity if we're explicit
> > that we only wanted the low 8 bits, though.
> >
>
> Resolves?

Whoops.

Resolves: Coverity CID 1547621

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/ide/atapi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> > index fcb6cca1573..e82959dc2d3 100644
> > --- a/hw/ide/atapi.c
> > +++ b/hw/ide/atapi.c
> > @@ -265,7 +265,7 @@ void ide_atapi_cmd_reply_end(IDEState *s)
>                /* a new transfer is needed */
>                s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
>                ide_bus_set_irq(s->bus);
>                byte_count_limit = atapi_byte_count_limit(s);
>                trace_ide_atapi_cmd_reply_end_bcl(s, byte_count_limit);
>                size = s->packet_transfer_size;
>                if (size > byte_count_limit) {
>                    /* byte count limit must be even if this case */
>                    if (byte_count_limit & 1)
> >                      byte_count_limit--;
> >                  size = byte_count_limit;
> >              }
> > -            s->lcyl = size;
> > +            s->lcyl = size & 0xff;
> >              s->hcyl = size >> 8;
> >              s->elementary_transfer_size = size;
> >              /* we cannot transmit more than one sector at a time */
>
> @size is int.  I wonder why it's fine with size >> 8...  ah,
> atapi_byte_count_limit() returns uint16_t, and Coverity is smart enough
> to data-flow this via @byte_count_limit into @size.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

thanks
-- PMM

Re: [PATCH 4/7] hw/ide/atapi: Be explicit that assigning to s->lcyl truncates

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In ide_atapi_cmd_reply_end() we calculate a 16-bit size, and then
> assign its two halves to s->lcyl and s->hcyl like this:
> 
>            s->lcyl = size;
>            s->hcyl = size >> 8;
> 
> Coverity warns that the first line here can overflow the
> 8-bit s->lcyl variable. This is true, and in this case we're
> deliberately only after the low 8 bits of the value. The
> code is clearer to both humans and Coverity if we're explicit
> that we only wanted the low 8 bits, though.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH 5/7] hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found something

[Peter Maydell]

Coverity complains about an overflow in isa_fdc_get_drive_max_chs()
that can happen if the loop over fd_formats never finds a match,
because we initialize *maxc to 0 and then at the end of the
function decrement it.

This can't ever actually happen because fd_formats has at least
one entry for each FloppyDriveType, so we must at least once
find a match and update *maxc, *maxh and *maxs. Assert that we
did find a match, which should keep Coverity happy and will also
detect possible bugs in the data in fd_formats.

Resolves: Coverity CID 1547663
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/fdc-isa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
index e43dc532af8..796835f57b3 100644
--- a/hw/block/fdc-isa.c
+++ b/hw/block/fdc-isa.c
@@ -147,6 +147,8 @@ static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
             *maxs = fdf->last_sect;
         }
     }
+    /* fd_formats must contain at least one entry per FloppyDriveType */
+    assert(*maxc);
     (*maxc)--;
 }
 
-- 
2.34.1

Re: [PATCH 5/7] hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found something

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Coverity complains about an overflow in isa_fdc_get_drive_max_chs()
> that can happen if the loop over fd_formats never finds a match,
> because we initialize *maxc to 0 and then at the end of the
> function decrement it.
>
> This can't ever actually happen because fd_formats has at least
> one entry for each FloppyDriveType, so we must at least once
> find a match and update *maxc, *maxh and *maxs. Assert that we
> did find a match, which should keep Coverity happy and will also
> detect possible bugs in the data in fd_formats.
>
> Resolves: Coverity CID 1547663
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/block/fdc-isa.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/block/fdc-isa.c b/hw/block/fdc-isa.c
> index e43dc532af8..796835f57b3 100644
> --- a/hw/block/fdc-isa.c
> +++ b/hw/block/fdc-isa.c
> @@ -147,6 +147,8 @@ static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
>              *maxs = fdf->last_sect;
>          }
>      }
> +    /* fd_formats must contain at least one entry per FloppyDriveType */
> +    assert(*maxc);
>      (*maxc)--;
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 5/7] hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found something

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> Coverity complains about an overflow in isa_fdc_get_drive_max_chs()
> that can happen if the loop over fd_formats never finds a match,
> because we initialize *maxc to 0 and then at the end of the
> function decrement it.
> 
> This can't ever actually happen because fd_formats has at least
> one entry for each FloppyDriveType, so we must at least once
> find a match and update *maxc, *maxh and *maxs. Assert that we
> did find a match, which should keep Coverity happy and will also
> detect possible bugs in the data in fd_formats.
> 
> Resolves: Coverity CID 1547663
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 5/7] hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found something

[Philippe Mathieu-Daudé]

On 31/7/24 16:36, Peter Maydell wrote:
> Coverity complains about an overflow in isa_fdc_get_drive_max_chs()
> that can happen if the loop over fd_formats never finds a match,
> because we initialize *maxc to 0 and then at the end of the
> function decrement it.
> 
> This can't ever actually happen because fd_formats has at least
> one entry for each FloppyDriveType, so we must at least once
> find a match and update *maxc, *maxh and *maxs. Assert that we
> did find a match, which should keep Coverity happy and will also
> detect possible bugs in the data in fd_formats.
> 
> Resolves: Coverity CID 1547663
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/block/fdc-isa.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 6/7] hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()

[Peter Maydell]

Coverity notes that the code at the end of the loop in
bmdma_prepare_buf() is unreachable.  This is because in commit
9fbf0fa81fca8f527 ("ide: remove hardcoded 2GiB transactional limit")
we removed the only codepath in the loop which could "break" out of
it, but didn't notice that this meant we should also remove the code
at the end of the loop.

Remove the dead code.

Resolves: Coverity CID 1547772
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ide/pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4675d079a17..f2cb500a94f 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -266,10 +266,6 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, int32_t limit)
             s->io_buffer_size += l;
         }
     }
-
-    qemu_sglist_destroy(&s->sg);
-    s->io_buffer_size = 0;
-    return -1;
 }
 
 /* return 0 if buffer completed */
-- 
2.34.1

Re: [PATCH 6/7] hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> Coverity notes that the code at the end of the loop in
> bmdma_prepare_buf() is unreachable.  This is because in commit
> 9fbf0fa81fca8f527 ("ide: remove hardcoded 2GiB transactional limit")
> we removed the only codepath in the loop which could "break" out of
> it, but didn't notice that this meant we should also remove the code
> at the end of the loop.
> 
> Remove the dead code.
> 
> Resolves: Coverity CID 1547772
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/ide/pci.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 4675d079a17..f2cb500a94f 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -266,10 +266,6 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, int32_t limit)
>              s->io_buffer_size += l;
>          }
>      }
> -
> -    qemu_sglist_destroy(&s->sg);
> -    s->io_buffer_size = 0;
> -    return -1;
>  }

Should we put a g_assert_not_reached() here instead to make it easier
for the reader to understand how this function works?

Either way:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 6/7] hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()

[Philippe Mathieu-Daudé]

On 31/7/24 17:13, Kevin Wolf wrote:
> Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
>> Coverity notes that the code at the end of the loop in
>> bmdma_prepare_buf() is unreachable.  This is because in commit
>> 9fbf0fa81fca8f527 ("ide: remove hardcoded 2GiB transactional limit")
>> we removed the only codepath in the loop which could "break" out of
>> it, but didn't notice that this meant we should also remove the code
>> at the end of the loop.
>>
>> Remove the dead code.
>>
>> Resolves: Coverity CID 1547772
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/ide/pci.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 4675d079a17..f2cb500a94f 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -266,10 +266,6 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, int32_t limit)
>>               s->io_buffer_size += l;
>>           }
>>       }
>> -
>> -    qemu_sglist_destroy(&s->sg);
>> -    s->io_buffer_size = 0;
>> -    return -1;
>>   }
> 
> Should we put a g_assert_not_reached() here instead to make it easier
> for the reader to understand how this function works?

Or break and keep returning at EOF:

-- >8 --
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4675d079a1..8386c4fe42 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -237,7 +237,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, 
int32_t limit)
              /* end of table (with a fail safe of one page) */
              if (bm->cur_prd_last ||
                  (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
-                return s->sg.size;
+                break;
              }
              pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
              bm->cur_addr += 8;
@@ -267,9 +267,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma, 
int32_t limit)
          }
      }

-    qemu_sglist_destroy(&s->sg);
-    s->io_buffer_size = 0;
-    return -1;
+    return s->sg.size;
  }

---

> 
> Either way:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 7/7] block/ssh.c: Don't double-check that characters are hex digits

[Peter Maydell]

In compare_fingerprint() we effectively check whether the characters
in the fingerprint are valid hex digits twice: first we do so with
qemu_isxdigit(), but then the hex2decimal() function also has a code
path where it effectively detects an invalid digit and returns -1.
This causes Coverity to complain because it thinks that we might use
that -1 value in an expression where it would be an integer overflow.

Avoid the double-check of hex digit validity by testing the return
values from hex2decimal() rather than doing separate calls to
qemu_isxdigit().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Could alternatively have put a g_assert_non_reached() in
hex2decimal(), but this seemed better to me.
---
 block/ssh.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 27d582e0e3d..510dd208aba 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -376,13 +376,15 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
     unsigned c;
 
     while (len > 0) {
+        unsigned c0, c1;
         while (*host_key_check == ':')
             host_key_check++;
-        if (!qemu_isxdigit(host_key_check[0]) ||
-            !qemu_isxdigit(host_key_check[1]))
+        c0 = hex2decimal(host_key_check[0]);
+        c1 = hex2decimal(host_key_check[1]);
+        if (c0 > 0xf || c1 > 0xf) {
             return 1;
-        c = hex2decimal(host_key_check[0]) * 16 +
-            hex2decimal(host_key_check[1]);
+        }
+        c = c0 * 16 + c1;
         if (c - *fingerprint != 0)
             return c - *fingerprint;
         fingerprint++;
-- 
2.34.1

Re: [PATCH 7/7] block/ssh.c: Don't double-check that characters are hex digits

[Kevin Wolf]

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In compare_fingerprint() we effectively check whether the characters
> in the fingerprint are valid hex digits twice: first we do so with
> qemu_isxdigit(), but then the hex2decimal() function also has a code
> path where it effectively detects an invalid digit and returns -1.
> This causes Coverity to complain because it thinks that we might use
> that -1 value in an expression where it would be an integer overflow.

If it's a Coverity issue, I think you want to mention the CID, too.

> Avoid the double-check of hex digit validity by testing the return
> values from hex2decimal() rather than doing separate calls to
> qemu_isxdigit().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Could alternatively have put a g_assert_non_reached() in
> hex2decimal(), but this seemed better to me.

I don't like that hex2decimal() returns -1 when its result is unsigned,
which is why you had to write the check as > 0xf rather than < 0. So in
this sense, g_assert_non_reached() would look better to me. But we could
also just change it to return UINT_MAX instead, which should be the
same, just written in a less confusing way.

>  block/ssh.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 27d582e0e3d..510dd208aba 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -376,13 +376,15 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
>      unsigned c;
>  
>      while (len > 0) {
> +        unsigned c0, c1;
>          while (*host_key_check == ':')
>              host_key_check++;
> -        if (!qemu_isxdigit(host_key_check[0]) ||
> -            !qemu_isxdigit(host_key_check[1]))
> +        c0 = hex2decimal(host_key_check[0]);
> +        c1 = hex2decimal(host_key_check[1]);
> +        if (c0 > 0xf || c1 > 0xf) {
>              return 1;
> -        c = hex2decimal(host_key_check[0]) * 16 +
> -            hex2decimal(host_key_check[1]);
> +        }
> +        c = c0 * 16 + c1;
>          if (c - *fingerprint != 0)
>              return c - *fingerprint;
>          fingerprint++;

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 7/7] block/ssh.c: Don't double-check that characters are hex digits

[Peter Maydell]

On Wed, 31 Jul 2024 at 16:21, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> > In compare_fingerprint() we effectively check whether the characters
> > in the fingerprint are valid hex digits twice: first we do so with
> > qemu_isxdigit(), but then the hex2decimal() function also has a code
> > path where it effectively detects an invalid digit and returns -1.
> > This causes Coverity to complain because it thinks that we might use
> > that -1 value in an expression where it would be an integer overflow.
>
> If it's a Coverity issue, I think you want to mention the CID, too.

Yes;

Resolves: Coverity CID 1547813

> > Avoid the double-check of hex digit validity by testing the return
> > values from hex2decimal() rather than doing separate calls to
> > qemu_isxdigit().
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Could alternatively have put a g_assert_non_reached() in
> > hex2decimal(), but this seemed better to me.
>
> I don't like that hex2decimal() returns -1 when its result is unsigned,
> which is why you had to write the check as > 0xf rather than < 0. So in
> this sense, g_assert_non_reached() would look better to me. But we could
> also just change it to return UINT_MAX instead, which should be the
> same, just written in a less confusing way.

I was not super happy with the -1 either. Happy to change that
to 'return UINT_MAX'.

-- PMM

Re: [PATCH 0/7] block: Miscellaneous minor Coverity fixes

[Philippe Mathieu-Daudé]

On 31/7/24 16:36, Peter Maydell wrote:

> Peter Maydell (7):
>    hw/block/pflash_cfi01: Don't decrement pfl->counter below 0
>    hw/ide/atapi: Be explicit that assigning to s->lcyl truncates
>    hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found
>      something
>    hw/ide/pci.c: Remove dead code from bmdma_prepare_buf()

Patches 3-5 queued, thanks.

Re: [PATCH 0/7] block: Miscellaneous minor Coverity fixes

[Philippe Mathieu-Daudé]

On 31/7/24 16:36, Peter Maydell wrote:

>    hw/block/pflash_cfi01: Don't decrement pfl->counter below 0
>    hw/ide/atapi: Be explicit that assigning to s->lcyl truncates
>    hw/block/fdc-isa: Assert that isa_fdc_get_drive_max_chs() found
>      something

Patches 3-5 queued, thanks.