[PATCH v2 0/8] coverity fixes

[PATCH v2 0/8] coverity fixes

[Vladimir Sementsov-Ogievskiy]

v2:
01: add explanations, new assert and avoid extra assignment
    add CIDs [thx to Paolo]
02: add explanation, improve wording
04,07: s/{0}/{}
06,08: improve wording

Hi! Here are some improvements to handle issues found by Coverity (not
public Coverity site, so there are no CIDs).

Vladimir Sementsov-Ogievskiy (8):
  hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): assert no overflow
  util/filemonitor-inotify: qemu_file_monitor_watch(): assert no
    overflow
  libvhost-user.c: add assertion to vu_message_read_default
  mc146818rtc: rtc_set_time(): initialize tm to zeroes
  pcie_sriov: unregister_vfs(): fix error path
  block/nvme: nvme_process_completion() fix bound for cid
  hw/core/loader: gunzip(): initialize z_stream
  io/channel-socket: qio_channel_socket_flush(): improve msg validation

 block/nvme.c                              |  7 ++++---
 hw/core/loader.c                          |  2 +-
 hw/i386/intel_iommu.c                     | 23 ++++++++++++++++++---
 hw/pci/pcie_sriov.c                       |  9 +++-----
 hw/rtc/mc146818rtc.c                      |  2 +-
 io/channel-socket.c                       |  5 +++++
 subprojects/libvhost-user/libvhost-user.c |  1 +
 util/filemonitor-inotify.c                | 25 +++++++++++++++--------
 8 files changed, 52 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 1/8] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): assert no overflow

[Vladimir Sementsov-Ogievskiy]

We support only 3- and 4-level page-tables, which is firstly checked in
vtd_decide_config(), then setup in vtd_init(). Than level fields are
checked by vtd_is_level_supported().

So here we can't have level out from 1..4 inclusive range. Let's assert
it. That also explains Coverity that we are not going to overflow the
array.

CID: 1487158, 1487186
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/i386/intel_iommu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..3b68183b78 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1027,18 +1027,35 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
  * Rsvd field masks for spte:
  *     vtd_spte_rsvd 4k pages
  *     vtd_spte_rsvd_large large pages
+ *
+ * We support only 3-level and 4-level page tables (see vtd_init() which
+ * sets only VTD_CAP_SAGAW_39bit and maybe VTD_CAP_SAGAW_48bit bits in s->cap).
  */
-static uint64_t vtd_spte_rsvd[5];
-static uint64_t vtd_spte_rsvd_large[5];
+#define VTD_SPTE_RSVD_LEN 5
+static uint64_t vtd_spte_rsvd[VTD_SPTE_RSVD_LEN];
+static uint64_t vtd_spte_rsvd_large[VTD_SPTE_RSVD_LEN];
 
 static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
 {
-    uint64_t rsvd_mask = vtd_spte_rsvd[level];
+    uint64_t rsvd_mask;
+
+    /*
+     * We should have caught a guest-mis-programmed level earlier,
+     * via vtd_is_level_supported.
+     */
+    assert(level < VTD_SPTE_RSVD_LEN);
+    /*
+     * Zero level doesn't exist. The smallest level is VTD_SL_PT_LEVEL=1 and
+     * checked by vtd_is_last_slpte().
+     */
+    assert(level);
 
     if ((level == VTD_SL_PD_LEVEL || level == VTD_SL_PDP_LEVEL) &&
         (slpte & VTD_SL_PT_PAGE_SIZE_MASK)) {
         /* large page */
         rsvd_mask = vtd_spte_rsvd_large[level];
+    } else {
+        rsvd_mask = vtd_spte_rsvd[level];
     }
 
     return slpte & rsvd_mask;
-- 
2.34.1

Re: [PATCH v2 1/8] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): assert no overflow

[Peter Maydell]

On Tue, 26 Sept 2023 at 21:15, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> We support only 3- and 4-level page-tables, which is firstly checked in
> vtd_decide_config(), then setup in vtd_init(). Than level fields are
> checked by vtd_is_level_supported().
>
> So here we can't have level out from 1..4 inclusive range. Let's assert
> it. That also explains Coverity that we are not going to overflow the
> array.
>
> CID: 1487158, 1487186
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/i386/intel_iommu.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 2/8] util/filemonitor-inotify: qemu_file_monitor_watch(): assert no overflow

[Vladimir Sementsov-Ogievskiy]

Prefer clear assertions instead of [im]possible array overflow.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 util/filemonitor-inotify.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
index 2c45f7f176..2121111f38 100644
--- a/util/filemonitor-inotify.c
+++ b/util/filemonitor-inotify.c
@@ -81,16 +81,25 @@ static void qemu_file_monitor_watch(void *arg)
 
     /* Loop over all events in the buffer */
     while (used < len) {
-        struct inotify_event *ev =
-            (struct inotify_event *)(buf + used);
-        const char *name = ev->len ? ev->name : "";
-        QFileMonitorDir *dir = g_hash_table_lookup(mon->idmap,
-                                                   GINT_TO_POINTER(ev->wd));
-        uint32_t iev = ev->mask &
-            (IN_CREATE | IN_MODIFY | IN_DELETE | IN_IGNORED |
-             IN_MOVED_TO | IN_MOVED_FROM | IN_ATTRIB);
+        const char *name;
+        QFileMonitorDir *dir;
+        uint32_t iev;
         int qev;
         gsize i;
+        struct inotify_event *ev = (struct inotify_event *)(buf + used);
+
+        /*
+         * We trust the kenel to provide valid buffer with complete event
+         * records.
+         */
+        assert(len - used >= sizeof(struct inotify_event));
+        assert(len - used - sizeof(struct inotify_event) >= ev->len);
+
+        name = ev->len ? ev->name : "";
+        dir = g_hash_table_lookup(mon->idmap, GINT_TO_POINTER(ev->wd));
+        iev = ev->mask &
+            (IN_CREATE | IN_MODIFY | IN_DELETE | IN_IGNORED |
+             IN_MOVED_TO | IN_MOVED_FROM | IN_ATTRIB);
 
         used += sizeof(struct inotify_event) + ev->len;
 
-- 
2.34.1

Re: [PATCH v2 2/8] util/filemonitor-inotify: qemu_file_monitor_watch(): assert no overflow

[Peter Maydell]

On Tue, 26 Sept 2023 at 21:15, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Prefer clear assertions instead of [im]possible array overflow.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 3/8] libvhost-user.c: add assertion to vu_message_read_default

[Vladimir Sementsov-Ogievskiy]

Explain Coverity that we are not going to overflow vmsg->fds.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 subprojects/libvhost-user/libvhost-user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 0469a50101..49b57c7ef4 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -322,6 +322,7 @@ vu_message_read_default(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
         if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS) {
             fd_size = cmsg->cmsg_len - CMSG_LEN(0);
             vmsg->fd_num = fd_size / sizeof(int);
+            assert(fd_size < VHOST_MEMORY_BASELINE_NREGIONS);
             memcpy(vmsg->fds, CMSG_DATA(cmsg), fd_size);
             break;
         }
-- 
2.34.1

[PATCH v2 4/8] mc146818rtc: rtc_set_time(): initialize tm to zeroes

[Vladimir Sementsov-Ogievskiy]

set_time() function doesn't set all the fields, so it's better to
initialize tm structure. And Coverity will be happier about it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/rtc/mc146818rtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index c27c362db9..2d391a8396 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -599,7 +599,7 @@ static void rtc_get_time(MC146818RtcState *s, struct tm *tm)
 
 static void rtc_set_time(MC146818RtcState *s)
 {
-    struct tm tm;
+    struct tm tm = {};
     g_autofree const char *qom_path = object_get_canonical_path(OBJECT(s));
 
     rtc_get_time(s, &tm);
-- 
2.34.1

Re: [PATCH v2 4/8] mc146818rtc: rtc_set_time(): initialize tm to zeroes

[Peter Maydell]

On Tue, 26 Sept 2023 at 21:15, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> set_time() function doesn't set all the fields, so it's better to
> initialize tm structure. And Coverity will be happier about it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/rtc/mc146818rtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 5/8] pcie_sriov: unregister_vfs(): fix error path

[Vladimir Sementsov-Ogievskiy]

local_err must be NULL before calling object_property_set_bool(), so we
must clear it on each iteration. Let's also use more convenient
error_reportf_err().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pcie_sriov.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
index 76a3b6917e..5ef8950940 100644
--- a/hw/pci/pcie_sriov.c
+++ b/hw/pci/pcie_sriov.c
@@ -196,19 +196,16 @@ static void register_vfs(PCIDevice *dev)
 
 static void unregister_vfs(PCIDevice *dev)
 {
-    Error *local_err = NULL;
     uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
     uint16_t i;
 
     trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
                                PCI_FUNC(dev->devfn), num_vfs);
     for (i = 0; i < num_vfs; i++) {
+        Error *err = NULL;
         PCIDevice *vf = dev->exp.sriov_pf.vf[i];
-        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
-        if (local_err) {
-            fprintf(stderr, "Failed to unplug: %s\n",
-                    error_get_pretty(local_err));
-            error_free(local_err);
+        if (!object_property_set_bool(OBJECT(vf), "realized", false, &err)) {
+            error_reportf_err(err, "Failed to unplug: ");
         }
         object_unparent(OBJECT(vf));
         object_unref(OBJECT(vf));
-- 
2.34.1

Re: [PATCH v2 5/8] pcie_sriov: unregister_vfs(): fix error path

[Vladimir Sementsov-Ogievskiy]

[add Markus]

On 26.09.23 23:15, Vladimir Sementsov-Ogievskiy wrote:
> local_err must be NULL before calling object_property_set_bool(), so we
> must clear it on each iteration. Let's also use more convenient
> error_reportf_err().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/pci/pcie_sriov.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci/pcie_sriov.c b/hw/pci/pcie_sriov.c
> index 76a3b6917e..5ef8950940 100644
> --- a/hw/pci/pcie_sriov.c
> +++ b/hw/pci/pcie_sriov.c
> @@ -196,19 +196,16 @@ static void register_vfs(PCIDevice *dev)
>   
>   static void unregister_vfs(PCIDevice *dev)
>   {
> -    Error *local_err = NULL;
>       uint16_t num_vfs = dev->exp.sriov_pf.num_vfs;
>       uint16_t i;
>   
>       trace_sriov_unregister_vfs(dev->name, PCI_SLOT(dev->devfn),
>                                  PCI_FUNC(dev->devfn), num_vfs);
>       for (i = 0; i < num_vfs; i++) {
> +        Error *err = NULL;
>           PCIDevice *vf = dev->exp.sriov_pf.vf[i];
> -        object_property_set_bool(OBJECT(vf), "realized", false, &local_err);
> -        if (local_err) {
> -            fprintf(stderr, "Failed to unplug: %s\n",
> -                    error_get_pretty(local_err));
> -            error_free(local_err);
> +        if (!object_property_set_bool(OBJECT(vf), "realized", false, &err)) {
> +            error_reportf_err(err, "Failed to unplug: ");
>           }
>           object_unparent(OBJECT(vf));
>           object_unref(OBJECT(vf));

-- 
Best regards,
Vladimir

[PATCH v2 6/8] block/nvme: nvme_process_completion() fix bound for cid

[Vladimir Sementsov-Ogievskiy]

NVMeQueuePair::reqs has length NVME_NUM_REQS, which less than
NVME_QUEUE_SIZE by 1.

Fixes: 1086e95da17050 ("block/nvme: switch to a NVMeRequest freelist")
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

Cc: stefanha@redhat.com
Cc: alex.chen@huawei.com
Cc: euler.robot@huawei.com

Note, that there was similar patch in the past:
 https://patchew.org/QEMU/20201208144452.91172-1-alex.chen@huawei.com/
I still think, that using NVME_NUM_REQS is better here.

 block/nvme.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index b6e95f0b7e..0faedf3072 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -416,9 +416,10 @@ static bool nvme_process_completion(NVMeQueuePair *q)
             q->cq_phase = !q->cq_phase;
         }
         cid = le16_to_cpu(c->cid);
-        if (cid == 0 || cid > NVME_QUEUE_SIZE) {
-            warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", "
-                        "queue size: %u", cid, NVME_QUEUE_SIZE);
+        if (cid == 0 || cid > NVME_NUM_REQS) {
+            warn_report("NVMe: Unexpected CID in completion queue: %" PRIu32
+                        ", should be within: 1..%u inclusively", cid,
+                        NVME_NUM_REQS);
             continue;
         }
         trace_nvme_complete_command(s, q->index, cid);
-- 
2.34.1

Re: [PATCH v2 6/8] block/nvme: nvme_process_completion() fix bound for cid

[Stefan Hajnoczi]

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

On Tue, Sep 26, 2023 at 11:15:30PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> NVMeQueuePair::reqs has length NVME_NUM_REQS, which less than
> NVME_QUEUE_SIZE by 1.
> 
> Fixes: 1086e95da17050 ("block/nvme: switch to a NVMeRequest freelist")
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> Cc: stefanha@redhat.com
> Cc: alex.chen@huawei.com
> Cc: euler.robot@huawei.com
> 
> Note, that there was similar patch in the past:
>  https://patchew.org/QEMU/20201208144452.91172-1-alex.chen@huawei.com/
> I still think, that using NVME_NUM_REQS is better here.
> 
>  block/nvme.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH v2 7/8] hw/core/loader: gunzip(): initialize z_stream

[Vladimir Sementsov-Ogievskiy]

Coverity signals that variable as being used uninitialized. And really,
when work with external APIs that's better to zero out the structure,
where we set some fields by hand.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/core/loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 4dd5a71fb7..b7bb44b7f7 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -558,7 +558,7 @@ static void zfree(void *x, void *addr)
 
 ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen)
 {
-    z_stream s;
+    z_stream s = {};
     ssize_t dstbytes;
     int r, i, flags;
 
-- 
2.34.1

Re: [PATCH v2 7/8] hw/core/loader: gunzip(): initialize z_stream

[Peter Maydell]

On Tue, 26 Sept 2023 at 21:16, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity signals that variable as being used uninitialized. And really,
> when work with external APIs that's better to zero out the structure,
> where we set some fields by hand.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 8/8] io/channel-socket: qio_channel_socket_flush(): improve msg validation

[Vladimir Sementsov-Ogievskiy]

For SO_EE_ORIGIN_ZEROCOPY the 32-bit notification range is encoded
as [ee_info, ee_data] inclusively, so ee_info should be less or
equal to ee_data.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 io/channel-socket.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 02ffb51e99..3a899b0608 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -782,6 +782,11 @@ static int qio_channel_socket_flush(QIOChannel *ioc,
                              "Error not from zero copy");
             return -1;
         }
+        if (serr->ee_data < serr->ee_info) {
+            error_setg_errno(errp, serr->ee_origin,
+                             "Wrong notification bounds");
+            return -1;
+        }
 
         /* No errors, count successfully finished sendmsg()*/
         sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;
-- 
2.34.1

Re: [PATCH v2 0/8] coverity fixes

[Maksim Davydov]

On 9/26/23 23:15, Vladimir Sementsov-Ogievskiy wrote:
> v2:
> 01: add explanations, new assert and avoid extra assignment
>      add CIDs [thx to Paolo]
> 02: add explanation, improve wording
> 04,07: s/{0}/{}
> 06,08: improve wording
>
> Hi! Here are some improvements to handle issues found by Coverity (not
> public Coverity site, so there are no CIDs).
>
> Vladimir Sementsov-Ogievskiy (8):
>    hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): assert no overflow
>    util/filemonitor-inotify: qemu_file_monitor_watch(): assert no
>      overflow
>    libvhost-user.c: add assertion to vu_message_read_default
>    mc146818rtc: rtc_set_time(): initialize tm to zeroes
>    pcie_sriov: unregister_vfs(): fix error path
>    block/nvme: nvme_process_completion() fix bound for cid
>    hw/core/loader: gunzip(): initialize z_stream
>    io/channel-socket: qio_channel_socket_flush(): improve msg validation
>
>   block/nvme.c                              |  7 ++++---
>   hw/core/loader.c                          |  2 +-
>   hw/i386/intel_iommu.c                     | 23 ++++++++++++++++++---
>   hw/pci/pcie_sriov.c                       |  9 +++-----
>   hw/rtc/mc146818rtc.c                      |  2 +-
>   io/channel-socket.c                       |  5 +++++
>   subprojects/libvhost-user/libvhost-user.c |  1 +
>   util/filemonitor-inotify.c                | 25 +++++++++++++++--------
>   8 files changed, 52 insertions(+), 22 deletions(-)
>
all patches:
Reviewed-by: Maksim Davydov <davydov-max@yandex-team.ru>