[PATCH 00/12] coverity fixes

[PATCH 00/12] coverity fixes

[Vladimir Sementsov-Ogievskiy]

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

Vladimir Sementsov-Ogievskiy (12):
  hw/core/loader: load_at(): check size
  hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers
  util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow
  libvhost-user.c: add assertion to vu_message_read_default
  device_tree: qmp_dumpdtb(): stronger assertion
  mc146818rtc: rtc_set_time(): initialize tm to zeroes
  pcie_sriov: unregister_vfs(): fix error path
  block/nvme: nvme_process_completion() fix bound for cid
  kvm-all: introduce limits for name_size and num_desc
  hw/core/loader: gunzip(): initialize z_stream
  hw/core/loader: read_targphys(): add upper bound
  io/channel-socket: qio_channel_socket_flush(): improve msg validation

 accel/kvm/kvm-all.c                       | 15 +++++++++++
 block/nvme.c                              |  6 ++---
 hw/core/loader.c                          | 32 +++++++++++++++++++----
 hw/i386/intel_iommu.c                     | 11 +++++---
 hw/pci/pcie_sriov.c                       |  9 +++----
 hw/rtc/mc146818rtc.c                      |  2 +-
 include/hw/loader.h                       |  2 --
 io/channel-socket.c                       |  5 ++++
 softmmu/device_tree.c                     |  2 +-
 subprojects/libvhost-user/libvhost-user.c |  1 +
 util/filemonitor-inotify.c                | 21 +++++++++------
 11 files changed, 77 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH 01/12] hw/core/loader: load_at(): check size

[Vladimir Sementsov-Ogievskiy]

This @size parameter often comes from fd. We'd better check it before
doing read and allocation.

Chose 1G as high enough empiric bound.

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

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 4dd5a71fb7..4b67543046 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
 
 /* ELF loader */
 
+#define ELF_LOAD_MAX (1024 * 1024 * 1024)
+
 static void *load_at(int fd, off_t offset, size_t size)
 {
     void *ptr;
-    if (lseek(fd, offset, SEEK_SET) < 0)
+
+    /*
+     * We often come here with @size, which was previously read from file
+     * descriptor too. That's not good to read and allocate for unchecked
+     * number of bytes. Coverity also doesn't like it and generate problems.
+     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
+     */
+    if (size > ELF_LOAD_MAX) {
         return NULL;
+    }
+
+    if (lseek(fd, offset, SEEK_SET) < 0) {
+        return NULL;
+    }
+
     ptr = g_malloc(size);
     if (read(fd, ptr, size) != size) {
         g_free(ptr);
-- 
2.34.1

[PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

Add a constant and clear assertion. The assertion also tells Coverity
that we are not going to overflow the array.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..2233dbe13a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
  *     vtd_spte_rsvd 4k pages
  *     vtd_spte_rsvd_large large pages
  */
-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;
+
+    assert(level < VTD_SPTE_RSVD_LEN);
+
+    rsvd_mask = vtd_spte_rsvd[level];
 
     if ((level == VTD_SL_PD_LEVEL || level == VTD_SL_PDP_LEVEL) &&
         (slpte & VTD_SL_PT_PAGE_SIZE_MASK)) {
-- 
2.34.1

[PATCH 03/12] util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow

[Vladimir Sementsov-Ogievskiy]

Prefer clear assertions instead of possible array overflow.

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

diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
index 2c45f7f176..09ef240174 100644
--- a/util/filemonitor-inotify.c
+++ b/util/filemonitor-inotify.c
@@ -81,16 +81,21 @@ 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);
+
+        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

[PATCH 04/12] 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 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Vladimir Sementsov-Ogievskiy]

Coverity mark this size, got from the buffer as untrasted value, it's
not good to use it as length when writing to file. Make the assertion
more strict to also check upper bound.

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

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 30aa3aea9f..adc4236e21 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
 
     size = fdt_totalsize(current_machine->fdt);
 
-    g_assert(size > 0);
+    g_assert(size > 0 && size <= FDT_MAX_SIZE);
 
     if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
         error_setg(errp, "Error saving FDT to file %s: %s",
-- 
2.34.1

[PATCH 06/12] 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..b63e1aeaea 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 = {0};
     g_autofree const char *qom_path = object_get_canonical_path(OBJECT(s));
 
     rtc_get_time(s, &tm);
-- 
2.34.1

[PATCH 07/12] 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

[PATCH 08/12] block/nvme: nvme_process_completion() fix bound for cid

[Vladimir Sementsov-Ogievskiy]

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

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

diff --git a/block/nvme.c b/block/nvme.c
index b6e95f0b7e..7f11ce1d46 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -416,9 +416,9 @@ 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 is: 1..%u", cid, NVME_NUM_REQS);
             continue;
         }
         trace_nvme_complete_command(s, q->index, cid);
-- 
2.34.1

[PATCH 09/12] kvm-all: introduce limits for name_size and num_desc

[Vladimir Sementsov-Ogievskiy]

Coverity doesn't like when the value with unchecked bounds that comes
from fd is used as length for IO or allocation. And really, that's not
a good practice. Let's introduce at least an empirical limits for these
values.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 accel/kvm/kvm-all.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ff1578bb32..6d0ba7d900 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3988,6 +3988,9 @@ typedef struct StatsDescriptors {
 static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
     QTAILQ_HEAD_INITIALIZER(stats_descriptors);
 
+
+#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
+#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)
 /*
  * Return the descriptors for 'target', that either have already been read
  * or are retrieved from 'stats_fd'.
@@ -4021,6 +4024,18 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
         g_free(descriptors);
         return NULL;
     }
+    if (kvm_stats_header->name_size > KVM_STATS_QEMU_MAX_NAME_SIZE) {
+        error_setg(errp, "KVM stats: too large name_size: %" PRIu32,
+                   kvm_stats_header->name_size);
+        g_free(descriptors);
+        return NULL;
+    }
+    if (kvm_stats_header->num_desc > KVM_STATS_QEMU_MAX_NUM_DESC) {
+        error_setg(errp, "KVM stats: too large num_desc: %" PRIu32,
+                   kvm_stats_header->num_desc);
+        g_free(descriptors);
+        return NULL;
+    }
     size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
 
     /* Read stats descriptors */
-- 
2.34.1

[PATCH 10/12] 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 4b67543046..aa02b27089 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -573,7 +573,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 = {0};
     ssize_t dstbytes;
     int r, i, flags;
 
-- 
2.34.1

[PATCH 11/12] hw/core/loader: read_targphys(): add upper bound

[Vladimir Sementsov-Ogievskiy]

Coverity doesn't like using "untrusted" values, coming from buffers and
fd-s as length to do IO and allocations. And that's make sense. The
function is used three times with "untrusted" nbytes parameter. Let's
introduce at least empirical limit of 1G for it.

While being here make the function static, as it's used only here.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/core/loader.c    | 13 ++++++++++---
 include/hw/loader.h |  2 --
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index aa02b27089..48cff6f59e 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
     return actsize < 0 ? -1 : l;
 }
 
+#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
 /* read()-like version */
-ssize_t read_targphys(const char *name,
-                      int fd, hwaddr dst_addr, size_t nbytes)
+static ssize_t read_targphys(const char *name,
+                             int fd, hwaddr dst_addr, size_t nbytes)
 {
     uint8_t *buf;
     ssize_t did;
 
+    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
+        return -1;
+    }
+
     buf = g_malloc(nbytes);
     did = read(fd, buf, nbytes);
-    if (did > 0)
+    if (did > 0) {
         rom_add_blob_fixed("read", buf, did, dst_addr);
+    }
+
     g_free(buf);
     return did;
 }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index c4c14170ea..e29af233d2 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -264,8 +264,6 @@ ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
 
 ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
 
-ssize_t read_targphys(const char *name,
-                      int fd, hwaddr dst_addr, size_t nbytes);
 void pstrcpy_targphys(const char *name,
                       hwaddr dest, int buf_size,
                       const char *source);
-- 
2.34.1

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

[Vladimir Sementsov-Ogievskiy]

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 08/12] block/nvme: nvme_process_completion() fix bound for cid

[Michael Tokarev]

25.09.2023 22:40, Vladimir Sementsov-Ogievskiy wrote:
> NVMeQueuePair::reqs as length NVME_NUM_REQS, which less than
> NVME_QUEUE_SIZE by 1.

> +        if (cid == 0 || cid > NVME_NUM_REQS) {
> +            warn_report("NVMe: Unexpected CID in completion queue: %" PRIu32
> +                        ", should be within is: 1..%u", cid, NVME_NUM_REQS);

  - is: I guess :)

/mjt

Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound

[Michael Tokarev]

25.09.2023 22:40, Vladimir Sementsov-Ogievskiy wrote:
> Coverity doesn't like using "untrusted" values, coming from buffers and
> fd-s as length to do IO and allocations. And that's make sense. The

"And that makes sense".  Just a nitpick in commit comment.

> function is used three times with "untrusted" nbytes parameter. Let's
> introduce at least empirical limit of 1G for it.
> 
> While being here make the function static, as it's used only here.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/core/loader.c    | 13 ++++++++++---
>   include/hw/loader.h |  2 --
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa02b27089..48cff6f59e 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>       return actsize < 0 ? -1 : l;
>   }
>   
> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>   /* read()-like version */
> -ssize_t read_targphys(const char *name,
> -                      int fd, hwaddr dst_addr, size_t nbytes)
> +static ssize_t read_targphys(const char *name,
> +                             int fd, hwaddr dst_addr, size_t nbytes)
>   {
>       uint8_t *buf;
>       ssize_t did;
>   
> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
> +        return -1;

Right now this is not important, since the only user of this
function, load_aout(), ignores errno value and reports general
failure instead.  Original read_targphys() returned errno which
corresponds to failed read().

FWIW, at least load_aout() assumes we've read whole struct exec
from the file in question, which might not be the case.

/mjt

Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Alistair Francis]

On Tue, Sep 26, 2023 at 6:42 AM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity mark this size, got from the buffer as untrasted value, it's

s/untrasted/untrusted/g

> not good to use it as length when writing to file. Make the assertion
> more strict to also check upper bound.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  softmmu/device_tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 30aa3aea9f..adc4236e21 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>
>      size = fdt_totalsize(current_machine->fdt);
>
> -    g_assert(size > 0);
> +    g_assert(size > 0 && size <= FDT_MAX_SIZE);
>
>      if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>          error_setg(errp, "Error saving FDT to file %s: %s",
> --
> 2.34.1
>
>

Re: [PATCH 12/12] io/channel-socket: qio_channel_socket_flush(): improve msg validation

[Maksim Davydov]

Could you add a comment into the commit message why ee_data must be
bigger than ee_info?

On 9/25/23 22:40, Vladimir Sementsov-Ogievskiy wrote:
> 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;

-- 
Best regards,
Maksim Davydov

Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 04:26, Alistair Francis wrote:
> On Tue, Sep 26, 2023 at 6:42 AM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Coverity mark this size, got from the buffer as untrasted value, it's
> 
> s/untrasted/untrusted/g

will fix.

> 
>> not good to use it as length when writing to file. Make the assertion
>> more strict to also check upper bound.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 

Thanks!

> 
>> ---
>>   softmmu/device_tree.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index 30aa3aea9f..adc4236e21 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>>
>>       size = fdt_totalsize(current_machine->fdt);
>>
>> -    g_assert(size > 0);
>> +    g_assert(size > 0 && size <= FDT_MAX_SIZE);
>>
>>       if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>>           error_setg(errp, "Error saving FDT to file %s: %s",
>> --
>> 2.34.1
>>
>>

-- 
Best regards,
Vladimir

Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound

[Vladimir Sementsov-Ogievskiy]

On 25.09.23 23:12, Michael Tokarev wrote:
> 25.09.2023 22:40, Vladimir Sementsov-Ogievskiy wrote:
>> Coverity doesn't like using "untrusted" values, coming from buffers and
>> fd-s as length to do IO and allocations. And that's make sense. The
> 
> "And that makes sense".  Just a nitpick in commit comment.
> 
>> function is used three times with "untrusted" nbytes parameter. Let's
>> introduce at least empirical limit of 1G for it.
>>
>> While being here make the function static, as it's used only here.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/core/loader.c    | 13 ++++++++++---
>>   include/hw/loader.h |  2 --
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index aa02b27089..48cff6f59e 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>>       return actsize < 0 ? -1 : l;
>>   }
>> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>>   /* read()-like version */
>> -ssize_t read_targphys(const char *name,
>> -                      int fd, hwaddr dst_addr, size_t nbytes)
>> +static ssize_t read_targphys(const char *name,
>> +                             int fd, hwaddr dst_addr, size_t nbytes)
>>   {
>>       uint8_t *buf;
>>       ssize_t did;
>> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
>> +        return -1;
> 
> Right now this is not important, since the only user of this
> function, load_aout(), ignores errno value and reports general
> failure instead.  Original read_targphys() returned errno which
> corresponds to failed read().

Agree, will fix to -EINVAL

> 
> FWIW, at least load_aout() assumes we've read whole struct exec
> from the file in question, which might not be the case.
> 

Hmm, right. Will fix too.

Thanks for reviewing!

-- 
Best regards,
Vladimir

Re: [PATCH 12/12] io/channel-socket: qio_channel_socket_flush(): improve msg validation

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 12:04, Maksim Davydov wrote:
> Could you add a comment into the commit message why ee_data must be
> bigger than ee_info?

As I understand, in this case ee_data is lower bound and ee_info is upper bound of notification:

https://docs.kernel.org/networking/msg_zerocopy.html#notification-parsing

and the next line "sioc->zero_copy_sent += serr->ee_data - serr->ee_info + 1;" actually depends on it.

So, I'll add:

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.

> 
> On 9/25/23 22:40, Vladimir Sementsov-Ogievskiy wrote:
>> 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;
> 

-- 
Best regards,
Vladimir

Re: [PATCH 01/12] hw/core/loader: load_at(): check size

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> This @size parameter often comes from fd. We'd better check it before
> doing read and allocation.
>
> Chose 1G as high enough empiric bound.

Empirical for who?

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/core/loader.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4dd5a71fb7..4b67543046 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>
>  /* ELF loader */
>
> +#define ELF_LOAD_MAX (1024 * 1024 * 1024)
> +
>  static void *load_at(int fd, off_t offset, size_t size)
>  {
>      void *ptr;
> -    if (lseek(fd, offset, SEEK_SET) < 0)
> +
> +    /*
> +     * We often come here with @size, which was previously read from file
> +     * descriptor too. That's not good to read and allocate for unchecked
> +     * number of bytes. Coverity also doesn't like it and generate problems.
> +     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
> +     */
> +    if (size > ELF_LOAD_MAX) {
>          return NULL;
> +    }
> +
> +    if (lseek(fd, offset, SEEK_SET) < 0) {
> +        return NULL;
> +    }
> +
>      ptr = g_malloc(size);
>      if (read(fd, ptr, size) != size) {
>          g_free(ptr);

This doesn't really help anything:
 (1) if the value is really big, it doesn't cause any terrible
consequences -- QEMU will just exit because the allocation
fails, which is fine because this will be at QEMU startup
and only happens if the user running QEMU gives us a silly file
 (2) we do a lot of other "allocate and abort on failure"
elsewhere in the ELF loader, for instance the allocations of
the symbol table and relocs in the load_symbols and
elf_reloc functions, and then on a bigger scale when we
work with the actual data in the ELF file

thanks
-- PMM

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Add a constant and clear assertion. The assertion also tells Coverity
> that we are not going to overflow the array.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/i386/intel_iommu.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..2233dbe13a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
>   *     vtd_spte_rsvd 4k pages
>   *     vtd_spte_rsvd_large large pages
>   */
> -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;
> +
> +    assert(level < VTD_SPTE_RSVD_LEN);
> +
> +    rsvd_mask = vtd_spte_rsvd[level];


Looking at the code it is not clear to me why this assertion is
valid. It looks like we are picking up fields from guest-set
configuration (probably in-memory data structures). So we can't
assert() here -- we need to do whatever the real hardware does
if these fields are set to an incorrect value, or at least something
sensible that doesn't crash QEMU.

thanks
-- PMM

Re: [PATCH 03/12] util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Prefer clear assertions instead of possible array overflow.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  util/filemonitor-inotify.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/util/filemonitor-inotify.c b/util/filemonitor-inotify.c
> index 2c45f7f176..09ef240174 100644
> --- a/util/filemonitor-inotify.c
> +++ b/util/filemonitor-inotify.c
> @@ -81,16 +81,21 @@ 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);
> +
> +        assert(len - used >= sizeof(struct inotify_event));
> +        assert(len - used - sizeof(struct inotify_event) >= ev->len);

So this is something we can assert because we trust the kernel
(which is what's on the other end of the inotify fd) to only
give us a valid buffer with complete event records in it, not
partial pieces, right? If so, then we should say so in a comment.

(FWIW in the online Coverity Scan we just marked this one as a
false-positive.)

thanks
-- PMM

Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:42, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity mark this size, got from the buffer as untrasted value, it's
> not good to use it as length when writing to file. Make the assertion
> more strict to also check upper bound.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  softmmu/device_tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 30aa3aea9f..adc4236e21 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>
>      size = fdt_totalsize(current_machine->fdt);
>
> -    g_assert(size > 0);
> +    g_assert(size > 0 && size <= FDT_MAX_SIZE);

FDT_MAX_SIZE is not "this is as big as an FDT can ever be". It's
only the internal sizing of device trees that we create ourselves
in the machine models (and which we will bump up if for some
reason we ever find ourselves needing to create bigger device
trees). So it's not really a suitable upper bound.

>      if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>          error_setg(errp, "Error saving FDT to file %s: %s",

Nothing bad happens if we pass g_file_set_contents() a very
large size -- we'll just create a large file. The user already
has lots of ways to fill up their disk if they want to, and
we don't have any idea how much disk space they might or might
not have.

I would just mark this as a false positive.

thanks
-- PMM

Re: [PATCH 01/12] hw/core/loader: load_at(): check size

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:33, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> This @size parameter often comes from fd. We'd better check it before
>> doing read and allocation.
>>
>> Chose 1G as high enough empiric bound.
> 
> Empirical for who?
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/core/loader.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 4dd5a71fb7..4b67543046 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>>
>>   /* ELF loader */
>>
>> +#define ELF_LOAD_MAX (1024 * 1024 * 1024)
>> +
>>   static void *load_at(int fd, off_t offset, size_t size)
>>   {
>>       void *ptr;
>> -    if (lseek(fd, offset, SEEK_SET) < 0)
>> +
>> +    /*
>> +     * We often come here with @size, which was previously read from file
>> +     * descriptor too. That's not good to read and allocate for unchecked
>> +     * number of bytes. Coverity also doesn't like it and generate problems.
>> +     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
>> +     */
>> +    if (size > ELF_LOAD_MAX) {
>>           return NULL;
>> +    }
>> +
>> +    if (lseek(fd, offset, SEEK_SET) < 0) {
>> +        return NULL;
>> +    }
>> +
>>       ptr = g_malloc(size);
>>       if (read(fd, ptr, size) != size) {
>>           g_free(ptr);
> 
> This doesn't really help anything:
>   (1) if the value is really big, it doesn't cause any terrible
> consequences -- QEMU will just exit because the allocation
> fails, which is fine because this will be at QEMU startup
> and only happens if the user running QEMU gives us a silly file
>   (2) we do a lot of other "allocate and abort on failure"
> elsewhere in the ELF loader, for instance the allocations of
> the symbol table and relocs in the load_symbols and
> elf_reloc functions, and then on a bigger scale when we
> work with the actual data in the ELF file

Reasonable..

Don't you have an idea, how to somehow mark the value "trusted" for Coverity?

-- 
Best regards,
Vladimir

Re: [PATCH 01/12] hw/core/loader: load_at(): check size

[Peter Maydell]

On Tue, 26 Sept 2023 at 11:51, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:33, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> This @size parameter often comes from fd. We'd better check it before
> >> doing read and allocation.
> >>
> >> Chose 1G as high enough empiric bound.
> >
> > Empirical for who?
> >
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >>   hw/core/loader.c | 17 ++++++++++++++++-
> >>   1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >> index 4dd5a71fb7..4b67543046 100644
> >> --- a/hw/core/loader.c
> >> +++ b/hw/core/loader.c
> >> @@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> >>
> >>   /* ELF loader */
> >>
> >> +#define ELF_LOAD_MAX (1024 * 1024 * 1024)
> >> +
> >>   static void *load_at(int fd, off_t offset, size_t size)
> >>   {
> >>       void *ptr;
> >> -    if (lseek(fd, offset, SEEK_SET) < 0)
> >> +
> >> +    /*
> >> +     * We often come here with @size, which was previously read from file
> >> +     * descriptor too. That's not good to read and allocate for unchecked
> >> +     * number of bytes. Coverity also doesn't like it and generate problems.
> >> +     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
> >> +     */
> >> +    if (size > ELF_LOAD_MAX) {
> >>           return NULL;
> >> +    }
> >> +
> >> +    if (lseek(fd, offset, SEEK_SET) < 0) {
> >> +        return NULL;
> >> +    }
> >> +
> >>       ptr = g_malloc(size);
> >>       if (read(fd, ptr, size) != size) {
> >>           g_free(ptr);
> >
> > This doesn't really help anything:
> >   (1) if the value is really big, it doesn't cause any terrible
> > consequences -- QEMU will just exit because the allocation
> > fails, which is fine because this will be at QEMU startup
> > and only happens if the user running QEMU gives us a silly file
> >   (2) we do a lot of other "allocate and abort on failure"
> > elsewhere in the ELF loader, for instance the allocations of
> > the symbol table and relocs in the load_symbols and
> > elf_reloc functions, and then on a bigger scale when we
> > work with the actual data in the ELF file
>
> Reasonable..
>
> Don't you have an idea, how to somehow mark the value "trusted" for Coverity?

In the web UI, I just mark it "false positive" in the dropdown, and
move on. Coverity has an absolute ton of false positives, and you
really can't work with it unless you have a workflow for ignoring them.

thanks
-- PMM

Re: [PATCH 06/12] mc146818rtc: rtc_set_time(): initialize tm to zeroes

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:42, 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(-)
>
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index c27c362db9..b63e1aeaea 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 = {0};
>      g_autofree const char *qom_path = object_get_canonical_path(OBJECT(s));
>
>      rtc_get_time(s, &tm);

This is probably a false positive, but initializing the struct is
easier to reason about for humans too.

Our "zero initialize a struct" syntax is "= {}" without the 0, though.
(The version with the 0 is the standards-blessed one, but in practice
some compiler versions have not been happy with it in all situations
and produce spurious warnings.)

thanks
-- PMM

Re: [PATCH 08/12] block/nvme: nvme_process_completion() fix bound for cid

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:42, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> NVMeQueuePair::reqs as length NVME_NUM_REQS, which less than
> NVME_QUEUE_SIZE by 1.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  block/nvme.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index b6e95f0b7e..7f11ce1d46 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -416,9 +416,9 @@ 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 is: 1..%u", cid, NVME_NUM_REQS);
>              continue;
>          }
>          trace_nvme_complete_command(s, q->index, cid);

A slightly different patch for this one was sent to the list back in 2020 but
apparently fell through the cracks:

https://patchew.org/QEMU/20201208144452.91172-1-alex.chen@huawei.com/

-- PMM

Re: [PATCH 09/12] kvm-all: introduce limits for name_size and num_desc

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity doesn't like when the value with unchecked bounds that comes
> from fd is used as length for IO or allocation. And really, that's not
> a good practice. Let's introduce at least an empirical limits for these
> values.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  accel/kvm/kvm-all.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ff1578bb32..6d0ba7d900 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3988,6 +3988,9 @@ typedef struct StatsDescriptors {
>  static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
>      QTAILQ_HEAD_INITIALIZER(stats_descriptors);
>
> +
> +#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
> +#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)

These seem arbitrary. Why these values in particular?
Does the kernel have any limitation on the values it passes us?
Do we have any particular limit on what we can handle?

thanks
-- PMM

Re: [PATCH 10/12] hw/core/loader: gunzip(): initialize z_stream

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:41, 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>
> ---
>  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 4b67543046..aa02b27089 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -573,7 +573,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 = {0};
>      ssize_t dstbytes;
>      int r, i, flags;

Same remark about using "= {}".

thanks
-- PMM

Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound

[Peter Maydell]

On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity doesn't like using "untrusted" values, coming from buffers and
> fd-s as length to do IO and allocations. And that's make sense. The
> function is used three times with "untrusted" nbytes parameter. Let's
> introduce at least empirical limit of 1G for it.
>
> While being here make the function static, as it's used only here.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/core/loader.c    | 13 ++++++++++---
>  include/hw/loader.h |  2 --
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa02b27089..48cff6f59e 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>      return actsize < 0 ? -1 : l;
>  }
>
> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>  /* read()-like version */
> -ssize_t read_targphys(const char *name,
> -                      int fd, hwaddr dst_addr, size_t nbytes)
> +static ssize_t read_targphys(const char *name,
> +                             int fd, hwaddr dst_addr, size_t nbytes)
>  {
>      uint8_t *buf;
>      ssize_t did;
>
> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
> +        return -1;
> +    }
> +
>      buf = g_malloc(nbytes);
>      did = read(fd, buf, nbytes);
> -    if (did > 0)
> +    if (did > 0) {
>          rom_add_blob_fixed("read", buf, did, dst_addr);
> +    }
> +
>      g_free(buf);
>      return did;
>  }

This is called only from load_aout(). load_aout() is
passed a max size that it can write. So we shouldn't
be imposing a different maximum size in this utility function,
in the same way that the standard library read() does not
say "I'm going to impose a 1GB limit on how much you can read".
If the limit checks in load_aout() are wrong we should fix them.

(In any case load_aout() is used only for loading the initial
kernel on PPC and SPARC, so it's not very important.)

thanks
-- PMM

Re: [PATCH 01/12] hw/core/loader: load_at(): check size

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:54, Peter Maydell wrote:
> On Tue, 26 Sept 2023 at 11:51, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> On 26.09.23 13:33, Peter Maydell wrote:
>>> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
>>> <vsementsov@yandex-team.ru> wrote:
>>>>
>>>> This @size parameter often comes from fd. We'd better check it before
>>>> doing read and allocation.
>>>>
>>>> Chose 1G as high enough empiric bound.
>>>
>>> Empirical for who?
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>>    hw/core/loader.c | 17 ++++++++++++++++-
>>>>    1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>>> index 4dd5a71fb7..4b67543046 100644
>>>> --- a/hw/core/loader.c
>>>> +++ b/hw/core/loader.c
>>>> @@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
>>>>
>>>>    /* ELF loader */
>>>>
>>>> +#define ELF_LOAD_MAX (1024 * 1024 * 1024)
>>>> +
>>>>    static void *load_at(int fd, off_t offset, size_t size)
>>>>    {
>>>>        void *ptr;
>>>> -    if (lseek(fd, offset, SEEK_SET) < 0)
>>>> +
>>>> +    /*
>>>> +     * We often come here with @size, which was previously read from file
>>>> +     * descriptor too. That's not good to read and allocate for unchecked
>>>> +     * number of bytes. Coverity also doesn't like it and generate problems.
>>>> +     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
>>>> +     */
>>>> +    if (size > ELF_LOAD_MAX) {
>>>>            return NULL;
>>>> +    }
>>>> +
>>>> +    if (lseek(fd, offset, SEEK_SET) < 0) {
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>>        ptr = g_malloc(size);
>>>>        if (read(fd, ptr, size) != size) {
>>>>            g_free(ptr);
>>>
>>> This doesn't really help anything:
>>>    (1) if the value is really big, it doesn't cause any terrible
>>> consequences -- QEMU will just exit because the allocation
>>> fails, which is fine because this will be at QEMU startup
>>> and only happens if the user running QEMU gives us a silly file
>>>    (2) we do a lot of other "allocate and abort on failure"
>>> elsewhere in the ELF loader, for instance the allocations of
>>> the symbol table and relocs in the load_symbols and
>>> elf_reloc functions, and then on a bigger scale when we
>>> work with the actual data in the ELF file
>>
>> Reasonable..
>>
>> Don't you have an idea, how to somehow mark the value "trusted" for Coverity?
> 
> In the web UI, I just mark it "false positive" in the dropdown, and
> move on. Coverity has an absolute ton of false positives, and you
> really can't work with it unless you have a workflow for ignoring them.
> 

Yes, I have the possibility to mark "false positives", but tried to fix some things in the code which seemed reasonable to me. Thanks a lot for reviewing and explaining!

-- 
Best regards,
Vladimir

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:37, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Add a constant and clear assertion. The assertion also tells Coverity
>> that we are not going to overflow the array.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/i386/intel_iommu.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index c0ce896668..2233dbe13a 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
>>    *     vtd_spte_rsvd 4k pages
>>    *     vtd_spte_rsvd_large large pages
>>    */
>> -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;
>> +
>> +    assert(level < VTD_SPTE_RSVD_LEN);
>> +
>> +    rsvd_mask = vtd_spte_rsvd[level];
> 
> 
> Looking at the code it is not clear to me why this assertion is
> valid. It looks like we are picking up fields from guest-set
> configuration (probably in-memory data structures). So we can't
> assert() here -- we need to do whatever the real hardware does
> if these fields are set to an incorrect value, or at least something
> sensible that doesn't crash QEMU.

But touching vtd_spte_rsvd with level>=5 is even worse than assertion, I think. That's overflows the array.

I don't know what the real hardware should do in this case. So, this assertion just stresses that the code is incomplete and makes it easier to find a bug if it comes.


-- 
Best regards,
Vladimir

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Peter Maydell]

On Tue, 26 Sept 2023 at 15:12, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:37, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> Add a constant and clear assertion. The assertion also tells Coverity
> >> that we are not going to overflow the array.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >>   hw/i386/intel_iommu.c | 11 ++++++++---
> >>   1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index c0ce896668..2233dbe13a 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
> >>    *     vtd_spte_rsvd 4k pages
> >>    *     vtd_spte_rsvd_large large pages
> >>    */
> >> -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;
> >> +
> >> +    assert(level < VTD_SPTE_RSVD_LEN);
> >> +
> >> +    rsvd_mask = vtd_spte_rsvd[level];
> >
> >
> > Looking at the code it is not clear to me why this assertion is
> > valid. It looks like we are picking up fields from guest-set
> > configuration (probably in-memory data structures). So we can't
> > assert() here -- we need to do whatever the real hardware does
> > if these fields are set to an incorrect value, or at least something
> > sensible that doesn't crash QEMU.
>
> But touching vtd_spte_rsvd with level>=5 is even worse than
> assertion, I think. That's overflows the array.

Correct. We shouldn't do that. But we also should not just
assert().

> I don't know what the real hardware should do in this case.

Then we should find out... Hopefully the specs will say.
If they don't then we can do whatever is a reasonable
behaviour (eg treat like some other valid value).

thanks
-- PMM

Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:51, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:42, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Coverity mark this size, got from the buffer as untrasted value, it's
>> not good to use it as length when writing to file. Make the assertion
>> more strict to also check upper bound.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   softmmu/device_tree.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
>> index 30aa3aea9f..adc4236e21 100644
>> --- a/softmmu/device_tree.c
>> +++ b/softmmu/device_tree.c
>> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
>>
>>       size = fdt_totalsize(current_machine->fdt);
>>
>> -    g_assert(size > 0);
>> +    g_assert(size > 0 && size <= FDT_MAX_SIZE);
> 
> FDT_MAX_SIZE is not "this is as big as an FDT can ever be". It's
> only the internal sizing of device trees that we create ourselves
> in the machine models (and which we will bump up if for some
> reason we ever find ourselves needing to create bigger device
> trees). So it's not really a suitable upper bound.
> 
>>       if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>>           error_setg(errp, "Error saving FDT to file %s: %s",
> 
> Nothing bad happens if we pass g_file_set_contents() a very

but it will also try to read beyond the allocated fdt. In my thought clear crash on assertion is better than such memory access.

> large size -- we'll just create a large file. The user already
> has lots of ways to fill up their disk if they want to, and
> we don't have any idea how much disk space they might or might
> not have.
> 
> I would just mark this as a false positive.
> 
> thanks
> -- PMM

-- 
Best regards,
Vladimir

Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion

[Peter Maydell]

On Tue, 26 Sept 2023 at 15:20, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:51, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:42, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> Coverity mark this size, got from the buffer as untrasted value, it's
> >> not good to use it as length when writing to file. Make the assertion
> >> more strict to also check upper bound.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >>   softmmu/device_tree.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> >> index 30aa3aea9f..adc4236e21 100644
> >> --- a/softmmu/device_tree.c
> >> +++ b/softmmu/device_tree.c
> >> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
> >>
> >>       size = fdt_totalsize(current_machine->fdt);
> >>
> >> -    g_assert(size > 0);
> >> +    g_assert(size > 0 && size <= FDT_MAX_SIZE);
> >
> > FDT_MAX_SIZE is not "this is as big as an FDT can ever be". It's
> > only the internal sizing of device trees that we create ourselves
> > in the machine models (and which we will bump up if for some
> > reason we ever find ourselves needing to create bigger device
> > trees). So it's not really a suitable upper bound.
> >
> >>       if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
> >>           error_setg(errp, "Error saving FDT to file %s: %s",
> >
> > Nothing bad happens if we pass g_file_set_contents() a very
>
> but it will also try to read beyond the allocated fdt.

No, it won't, because we can assume that current_machine->fdt points
to a valid device tree blob, and so size is by definition correct.

The libfdt code keeps track of the size of the memory we allocated
for it to use -- when we call fdt_open_into() we pass that size.
fdt_totalsize() simply returns that passed in size value. So
the amount of memory we can access is exactly "size".

(The fdt may not have come from create_device_tree(), so
it's possible both that the size as returned by fdt_totalsize()
can validly be larger than FDT_MAX_SIZE, and also that the fdt
blob can be smaller than FDT_MAX_SIZE. So that's the wrong thing
to try to check against either way.)

thanks
-- PMM

Re: [PATCH 09/12] kvm-all: introduce limits for name_size and num_desc

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 14:05, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:43, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Coverity doesn't like when the value with unchecked bounds that comes
>> from fd is used as length for IO or allocation. And really, that's not
>> a good practice. Let's introduce at least an empirical limits for these
>> values.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   accel/kvm/kvm-all.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index ff1578bb32..6d0ba7d900 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3988,6 +3988,9 @@ typedef struct StatsDescriptors {
>>   static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
>>       QTAILQ_HEAD_INITIALIZER(stats_descriptors);
>>
>> +
>> +#define KVM_STATS_QEMU_MAX_NAME_SIZE (1024 * 1024)
>> +#define KVM_STATS_QEMU_MAX_NUM_DESC (1024)
> 
> These seem arbitrary. Why these values in particular?
> Does the kernel have any limitation on the values it passes us?

Documentation doesn't say about limits

> Do we have any particular limit on what we can handle?
> 

Hmm. At least, we don't arithmetic operations with these values to overflow. But in this case g_malloc0_n should crash anyway.

So we may rely on g_malloc0_n as on assertion that the values are good enough and further doubts are false-positives. Will drop this patch.

-- 
Best regards,
Vladimir

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:37, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Add a constant and clear assertion. The assertion also tells Coverity
>> that we are not going to overflow the array.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/i386/intel_iommu.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index c0ce896668..2233dbe13a 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
>>    *     vtd_spte_rsvd 4k pages
>>    *     vtd_spte_rsvd_large large pages
>>    */
>> -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;
>> +
>> +    assert(level < VTD_SPTE_RSVD_LEN);
>> +
>> +    rsvd_mask = vtd_spte_rsvd[level];
> 
> 
> Looking at the code it is not clear to me why this assertion is
> valid. It looks like we are picking up fields from guest-set
> configuration (probably in-memory data structures). So we can't
> assert() here -- we need to do whatever the real hardware does
> if these fields are set to an incorrect value, or at least something
> sensible that doesn't crash QEMU.
> 

Finally, seems that assertion is valid. We do check the guest-set configuration:

1. in vtd_decide_config(), we check that s->aw_bits is exactly one of VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.

2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be VTD_CAP_SAGAW_48bit (bit 2),  but never bit 3 (which would allow 5-level page-table) or any other bit (i.e. bits 0 and 4 which are reserved).

3. then, as I could follow, both context entry and pasid entry should go through vtd_is_level_supported(), which checks that level is allowed in s->cap.

So in the code we should work only with levels 3 and 4.



-- 
Best regards,
Vladimir

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 21:36, Vladimir Sementsov-Ogievskiy wrote:
> On 26.09.23 13:37, Peter Maydell wrote:
>> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
>> <vsementsov@yandex-team.ru> wrote:
>>>
>>> Add a constant and clear assertion. The assertion also tells Coverity
>>> that we are not going to overflow the array.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   hw/i386/intel_iommu.c | 11 ++++++++---
>>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index c0ce896668..2233dbe13a 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
>>>    *     vtd_spte_rsvd 4k pages
>>>    *     vtd_spte_rsvd_large large pages
>>>    */
>>> -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;
>>> +
>>> +    assert(level < VTD_SPTE_RSVD_LEN);
>>> +
>>> +    rsvd_mask = vtd_spte_rsvd[level];
>>
>>
>> Looking at the code it is not clear to me why this assertion is
>> valid. It looks like we are picking up fields from guest-set
>> configuration (probably in-memory data structures). So we can't
>> assert() here -- we need to do whatever the real hardware does
>> if these fields are set to an incorrect value, or at least something
>> sensible that doesn't crash QEMU.
>>
> 
> Finally, seems that assertion is valid. We do check the guest-set configuration:
> 
> 1. in vtd_decide_config(), we check that s->aw_bits is exactly one of VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.
> 
> 2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be VTD_CAP_SAGAW_48bit (bit 2),  but never bit 3 (which would allow 5-level page-table) or any other bit (i.e. bits 0 and 4 which are reserved).
> 
> 3. then, as I could follow, both context entry and pasid entry should go through vtd_is_level_supported(), which checks that level is allowed in s->cap.
> 
> So in the code we should work only with levels 3 and 4.
> 

(i.e. maximum levels, of course we have page-tables of levels 1..3 and 1..4 correspondingly)


-- 
Best regards,
Vladimir

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Peter Maydell]

On Tue, 26 Sept 2023 at 19:36, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:37, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> Add a constant and clear assertion. The assertion also tells Coverity
> >> that we are not going to overflow the array.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >>   hw/i386/intel_iommu.c | 11 ++++++++---
> >>   1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index c0ce896668..2233dbe13a 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
> >>    *     vtd_spte_rsvd 4k pages
> >>    *     vtd_spte_rsvd_large large pages
> >>    */
> >> -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;
> >> +
> >> +    assert(level < VTD_SPTE_RSVD_LEN);
> >> +
> >> +    rsvd_mask = vtd_spte_rsvd[level];
> >
> >
> > Looking at the code it is not clear to me why this assertion is
> > valid. It looks like we are picking up fields from guest-set
> > configuration (probably in-memory data structures). So we can't
> > assert() here -- we need to do whatever the real hardware does
> > if these fields are set to an incorrect value, or at least something
> > sensible that doesn't crash QEMU.
> >
>
> Finally, seems that assertion is valid. We do check the guest-set configuration:
>
> 1. in vtd_decide_config(), we check that s->aw_bits is exactly one of VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.
>
> 2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be VTD_CAP_SAGAW_48bit (bit 2),  but never bit 3 (which would allow 5-level page-table) or any other bit (i.e. bits 0 and 4 which are reserved).
>
> 3. then, as I could follow, both context entry and pasid entry should go through vtd_is_level_supported(), which checks that level is allowed in s->cap.
>
> So in the code we should work only with levels 3 and 4.

Thanks for working through that. I'm not completely sure if we always
do the level validity check (eg in vtd_dev_to_context_entry() we skip
it if s->root_scalable is true), but clearly the intention of the code
is to validate the level early. So asserting in this function is fine,
and if the assert ever fires we know we got the validity check wrong
earlier.

A comment something like

  /*
   * We should have caught a guest-mis-programmed level earlier,
   * via vtd_is_level_supported
   */

might help somebody in future if the assert ever does fire.

Could you also add "CID: 1487158, 1487186" to the commit message?
I've just noticed this issue is in our online coverity scan db too
as unresolved.

With those changes,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 21:59, Peter Maydell wrote:
> On Tue, 26 Sept 2023 at 19:36, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> On 26.09.23 13:37, Peter Maydell wrote:
>>> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
>>> <vsementsov@yandex-team.ru> wrote:
>>>>
>>>> Add a constant and clear assertion. The assertion also tells Coverity
>>>> that we are not going to overflow the array.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>>    hw/i386/intel_iommu.c | 11 ++++++++---
>>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index c0ce896668..2233dbe13a 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -1028,12 +1028,17 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
>>>>     *     vtd_spte_rsvd 4k pages
>>>>     *     vtd_spte_rsvd_large large pages
>>>>     */
>>>> -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;
>>>> +
>>>> +    assert(level < VTD_SPTE_RSVD_LEN);
>>>> +
>>>> +    rsvd_mask = vtd_spte_rsvd[level];
>>>
>>>
>>> Looking at the code it is not clear to me why this assertion is
>>> valid. It looks like we are picking up fields from guest-set
>>> configuration (probably in-memory data structures). So we can't
>>> assert() here -- we need to do whatever the real hardware does
>>> if these fields are set to an incorrect value, or at least something
>>> sensible that doesn't crash QEMU.
>>>
>>
>> Finally, seems that assertion is valid. We do check the guest-set configuration:
>>
>> 1. in vtd_decide_config(), we check that s->aw_bits is exactly one of VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.
>>
>> 2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be VTD_CAP_SAGAW_48bit (bit 2),  but never bit 3 (which would allow 5-level page-table) or any other bit (i.e. bits 0 and 4 which are reserved).
>>
>> 3. then, as I could follow, both context entry and pasid entry should go through vtd_is_level_supported(), which checks that level is allowed in s->cap.
>>
>> So in the code we should work only with levels 3 and 4.
> 
> Thanks for working through that. I'm not completely sure if we always
> do the level validity check (eg in vtd_dev_to_context_entry() we skip
> it if s->root_scalable is true),

when root_scalable is true, level comes from pasid entry, see vtd_get_iova_level():

   static uint32_t vtd_get_iova_level(IntelIOMMUState *s,
                                      VTDContextEntry *ce,
                                      uint32_t pasid)
   {
       VTDPASIDEntry pe;
                                                                                    
       if (s->root_scalable) {
           vtd_ce_get_rid2pasid_entry(s, ce, &pe, pasid);
           return VTD_PE_GET_LEVEL(&pe);
       }
                                                                                    
       return vtd_ce_get_level(ce);
   }


and in case of root_scalable=true, in vtd_dev_to_context_entry(), we do check it by vtd_ce_rid2pasid_check() -> .... -> vtd_get_pe_in_pasid_leaf_table() -> vtd_is_level_supported()


[but yes, I'm not sure that we correctly check it on _all_ the paths]


> but clearly the intention of the code
> is to validate the level early. So asserting in this function is fine,
> and if the assert ever fires we know we got the validity check wrong
> earlier.
> 
> A comment something like
> 
>    /*
>     * We should have caught a guest-mis-programmed level earlier,
>     * via vtd_is_level_supported
>     */
> 
> might help somebody in future if the assert ever does fire.
> 
> Could you also add "CID: 1487158, 1487186" to the commit message?
> I've just noticed this issue is in our online coverity scan db too
> as unresolved.

OK, thanks for checking this.

> 
> With those changes,
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 

Thanks!

-- 
Best regards,
Vladimir