Re: [PATCH v3 24/28] tests/qtest: Replace g_memdup() by g_memdup2()
On 03/09/2021 19.45, Philippe Mathieu-Daudé wrote:
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/libqos/ahci.c | 6 +++---
tests/qtest/libqos/qgraph.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index fba3e7a954e..eaa2096512e 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -639,8 +639,8 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
AHCIOpts *opts;
uint64_t buffer_in;
-opts = g_memdup((opts_in == NULL ? _opts : opts_in),
-sizeof(AHCIOpts));
+opts = g_memdup2((opts_in == NULL ? _opts : opts_in),
+ sizeof(AHCIOpts));
buffer_in = opts->buffer;
@@ -860,7 +860,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
g_assert(!props->ncq || props->lba48);
/* Defaults and book-keeping */
-cmd->props = g_memdup(props, sizeof(AHCICommandProp));
+cmd->props = g_memdup2(props, sizeof(AHCICommandProp));
cmd->name = command_name;
cmd->xbytes = props->size;
cmd->prd_size = 4096;
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index d1dc4919305..109ff04e1e8 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -93,7 +93,7 @@ static void add_edge(const char *source, const char *dest,
edge->type = type;
edge->dest = g_strdup(dest);
edge->edge_name = g_strdup(opts->edge_name ?: dest);
-edge->arg = g_memdup(opts->arg, opts->size_arg);
+edge->arg = g_memdup2(opts->arg, opts->size_arg);
edge->before_cmd_line =
opts->before_cmd_line ? g_strconcat(" ", opts->before_cmd_line, NULL)
: NULL;
Reviewed-by: Thomas Huth
Re: [PATCH v3 26/28] target/ppc: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:45:08PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson > --- > target/ppc/mmu-hash64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index 19832c4b46f..bc6f8748acb 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -1122,7 +1122,7 @@ void ppc_hash64_init(PowerPCCPU *cpu) > return; > } > > -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); > +cpu->hash64_opts = g_memdup2(pcc->hash64_opts, > sizeof(*cpu->hash64_opts)); > } > > void ppc_hash64_finalize(PowerPCCPU *cpu) -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v3 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:44:58PM +0200, Philippe Mathieu-Daudé wrote:
> Per
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> The old API took the size of the memory to duplicate as a guint,
> whereas most memory functions take memory sizes as a gsize. This
> made it easy to accidentally pass a gsize to g_memdup(). For large
> values, that would lead to a silent truncation of the size from 64
> to 32 bits, and result in a heap area being returned which is
> significantly smaller than what the caller expects. This can likely
> be exploited in various modules to cause a heap buffer overflow.
>
> Replace g_memdup() by the safer g_memdup2() wrapper.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/spapr_pci.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7430bd63142..8e36cffab79 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2201,10 +2201,9 @@ static int spapr_pci_post_load(void *opaque, int
> version_id)
> int i;
>
> for (i = 0; i < sphb->msi_devs_num; ++i) {
> -key = g_memdup(>msi_devs[i].key,
> - sizeof(sphb->msi_devs[i].key));
> -value = g_memdup(>msi_devs[i].value,
> - sizeof(sphb->msi_devs[i].value));
> +key = g_memdup2(>msi_devs[i].key,
> sizeof(sphb->msi_devs[i].key));
> +value = g_memdup2(>msi_devs[i].value,
> + sizeof(sphb->msi_devs[i].value));
> g_hash_table_insert(sphb->msi, key, value);
> }
> g_free(sphb->msi_devs);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2_qemu()
On Fri, Sep 03, 2021 at 01:06:50PM +0200, Philippe Mathieu-Daudé wrote:
> Per
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> The old API took the size of the memory to duplicate as a guint,
> whereas most memory functions take memory sizes as a gsize. This
> made it easy to accidentally pass a gsize to g_memdup(). For large
> values, that would lead to a silent truncation of the size from 64
> to 32 bits, and result in a heap area being returned which is
> significantly smaller than what the caller expects. This can likely
> be exploited in various modules to cause a heap buffer overflow.
>
> Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/spapr_pci.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7430bd63142..79c0e8d4f98 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2201,10 +2201,10 @@ static int spapr_pci_post_load(void *opaque, int
> version_id)
> int i;
>
> for (i = 0; i < sphb->msi_devs_num; ++i) {
> -key = g_memdup(>msi_devs[i].key,
> - sizeof(sphb->msi_devs[i].key));
> -value = g_memdup(>msi_devs[i].value,
> - sizeof(sphb->msi_devs[i].value));
> +key = g_memdup2_qemu(>msi_devs[i].key,
> + sizeof(sphb->msi_devs[i].key));
> +value = g_memdup2_qemu(>msi_devs[i].value,
> + sizeof(sphb->msi_devs[i].value));
> g_hash_table_insert(sphb->msi, key, value);
> }
> g_free(sphb->msi_devs);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 26/28] target/ppc: Replace g_memdup() by g_memdup2_qemu()
On Fri, Sep 03, 2021 at 01:07:00PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2_qemu() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson > --- > target/ppc/mmu-hash64.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index 19832c4b46f..2ee6025a406 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -1122,7 +1122,8 @@ void ppc_hash64_init(PowerPCCPU *cpu) > return; > } > > -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); > +cpu->hash64_opts = g_memdup2_qemu(pcc->hash64_opts, > + sizeof(*cpu->hash64_opts)); > } > > void ppc_hash64_finalize(PowerPCCPU *cpu) -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v6 04/11] block: use int64_t instead of uint64_t in driver write handlers
On Fri, Sep 03, 2021 at 01:28:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
...
> Still, the functions may be called directly, not only by drv->...
> Let's check:
>
> git grep '\.bdrv_\(aio\|co\)_pwritev\(_part\)\?\s*=' | \
> awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \
> while read func; do git grep "$func(" | \
> grep -v "$func(BlockDriverState"; done
>
> shows several callers:
>
> qcow2:
> qcow2_co_truncate() write at most up to @offset, which is checked in
> generic qcow2_co_truncate() by bdrv_check_request().
> qcow2_co_pwritev_compressed_task() pass the request (or part of the
> request) that already went through normal write path, so it should
> be OK
>
> qcow:
> qcow_co_pwritev_compressed() pass int64_t, it's updated by this patch
>
> quorum:
> quorum_co_pwrite_zeroes() pass int64_t and int - OK
>
> throttle:
> throttle_co_pwritev_compressed() pass int64_t, it's updated by this
> patch
>
> vmdk:
> vmdk_co_pwritev_compressed() pass int64_t, it's updated by this
> patch
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 03/11] block: use int64_t instead of uint64_t in driver read handlers
On Fri, Sep 03, 2021 at 01:27:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
>
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
>
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
>
> So, convert driver read handlers parameters which are already 64bit to
> signed type.
>
> While being here, convert also flags parameter to be BdrvRequestFlags.
>
> Now let's consider all callers. Simple
>
> git grep '\->bdrv_\(aio\|co\)_preadv\(_part\)\?'
>
> shows that's there three callers of driver function:
>
> bdrv_driver_preadv() in block/io.c, passes int64_t, checked by
>bdrv_check_qiov_request() to be non-negative.
>
> qcow2_load_vmstate() does bdrv_check_qiov_request().
>
> do_perform_cow_read() has uint64_t argument. And a lot of things in
> qcow2 driver are uint64_t, so converting it is big job. But we must
> not work with requests that don't satisfy bdrv_check_qiov_request(),
> so let's just assert it here.
>
> Still, the functions may be called directly, not only by drv->...
> Let's check:
>
> git grep '\.bdrv_\(aio\|co\)_preadv\(_part\)\?\s*=' | \
> awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \
> while read func; do git grep "$func(" | \
> grep -v "$func(BlockDriverState"; done
>
> The only one such caller:
>
> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, , 1);
> ...
> ret = bdrv_replace_test_co_preadv(bs, 0, 1, , 0);
>
> in tesTS/unit/test-bdrv-drain.c, and it's OK obviously.
Odd capitalization.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> +++ b/block/qcow2-cluster.c
> @@ -505,7 +505,19 @@ static int coroutine_fn
> do_perform_cow_read(BlockDriverState *bs,
> return -ENOMEDIUM;
> }
>
> -/* Call .bdrv_co_readv() directly instead of using the public block-layer
> +/*
> + * We never deal with requests that doesn't satisfy
> + * bdrv_check_qiov_request(), and aligning requests to clusters never
> break
never breaks
> + * this condition. So, do some assertions before calling
> + * bs->drv->bdrv_co_preadv_part() which has int64_t arguments.
> + */
> +assert(src_cluster_offset <= INT64_MAX);
> +assert(src_cluster_offset + offset_in_cluster <= INT64_MAX);
> +assert(qiov->size <= INT64_MAX);
> +bdrv_check_qiov_request(src_cluster_offset + offset_in_cluster,
> qiov->size,
> +qiov, 0, _abort);
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit a88d9ace234a24ce1c17189642ef9104799425e0
> +Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> diff --git a/ui/keycodemapdb b/ui/keycodemapdb
> index d21009b1c9..6119e6e19a 16
> --- a/ui/keycodemapdb
> +++ b/ui/keycodemapdb
> @@ -1 +1 @@
> -Subproject commit d21009b1c9f94b740ea66be8e48a1d8ad8124023
> +Subproject commit 6119e6e19a050df847418de7babe5166779955e4
Oops. Fix that (or I can do it while staging, if the rest of the
series is okay), and you have:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 05/28] block/qcow2-bitmap: Replace g_memdup() by g_memdup2()
On 9/3/21 11:13 PM, Eric Blake wrote: > On Fri, Sep 03, 2021 at 07:44:47PM +0200, Philippe Mathieu-Daudé wrote: >> Per >> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 >> >> The old API took the size of the memory to duplicate as a guint, >> whereas most memory functions take memory sizes as a gsize. This >> made it easy to accidentally pass a gsize to g_memdup(). For large >> values, that would lead to a silent truncation of the size from 64 >> to 32 bits, and result in a heap area being returned which is >> significantly smaller than what the caller expects. This can likely >> be exploited in various modules to cause a heap buffer overflow. >> >> Replace g_memdup() by the safer g_memdup2() wrapper. >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> block/qcow2-bitmap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index 8fb47315515..218a0dc712a 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c >> @@ -1599,7 +1599,7 @@ bool >> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, >> name); >> goto fail; >> } >> -tb = g_memdup(>table, sizeof(bm->table)); >> +tb = g_memdup2(>table, sizeof(bm->table)); > > Trivially safe. It might be worth a comment in the various commit > messages for which patches are trivially safe (because the argument > was directly from sizeof), and which would require a larger audit of > callers to see if we had any (unlikely) bug (such as patch 3/28). Yes, will do. > Reviewed-by: Eric Blake Thanks!
Re: [PATCH v3 28/28] checkpatch: Do not allow deprecated g_memdup()
On Fri, Sep 03, 2021 at 07:45:10PM +0200, Philippe Mathieu-Daudé wrote: > g_memdup() is insecure and as been deprecated in GLib 2.68. > QEMU provides the safely equivalent g_memdup2() wrapper. > > Do not allow more g_memdup() calls in the repository, provide > a hint to use g_memdup2(). > > Signed-off-by: Philippe Mathieu-Daudé > --- > scripts/checkpatch.pl | 5 + > 1 file changed, 5 insertions(+) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 05/28] block/qcow2-bitmap: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:44:47PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé > --- > block/qcow2-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 8fb47315515..218a0dc712a 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -1599,7 +1599,7 @@ bool > qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, > name); > goto fail; > } > -tb = g_memdup(>table, sizeof(bm->table)); > +tb = g_memdup2(>table, sizeof(bm->table)); Trivially safe. It might be worth a comment in the various commit messages for which patches are trivially safe (because the argument was directly from sizeof), and which would require a larger audit of callers to see if we had any (unlikely) bug (such as patch 3/28). Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 03/28] qapi: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:44:45PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé > --- > qapi/qapi-clone-visitor.c | 16 > qapi/qapi-visit-core.c| 6 -- > 2 files changed, 12 insertions(+), 10 deletions(-) Reviewed-by: Eric Blake > > diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c > index c45c5caa3b8..b014119d368 100644 > --- a/qapi/qapi-clone-visitor.c > +++ b/qapi/qapi-clone-visitor.c > @@ -37,7 +37,7 @@ static bool qapi_clone_start_struct(Visitor *v, const char > *name, void **obj, > return true; > } > > -*obj = g_memdup(*obj, size); > +*obj = g_memdup2(*obj, size); I did not audit whether any callers were previously vulnerable, although I suspect most (if not all) callers were from the generated QAPI code passing in the results of sizeof, and none of our QAPI types are 4G large to cause overflow. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Sep 03, 2021 at 07:44:44PM +0200, Philippe Mathieu-Daudé wrote: > When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68 > (Fedora 34 provides GLib 2.68.1) we get: > > hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use > 'g_memdup2' instead [-Werror,-Wdeprecated-declarations] > ... > > g_memdup() has been updated by g_memdup2() to fix eventual security > issues (size argument is 32-bit and could be truncated / wrapping). > GLib recommends to copy their static inline version of g_memdup2(): > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > Our glib-compat.h provides a comment explaining how to deal with > these deprecated declarations (see commit e71e8cc0355 > "glib: enforce the minimum required version and warn about old APIs"). > > Following this comment suggestion, implement the g_memdup2_qemu() > wrapper to g_memdup2(), and use the safer equivalent inlined when > we are using pre-2.68 GLib. > > Reported-by: Eric Blake > Signed-off-by: Philippe Mathieu-Daudé > --- > include/glib-compat.h | 37 + > 1 file changed, 37 insertions(+) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2] block: drop BLK_PERM_GRAPH_MOD
On Thu, Sep 02, 2021 at 12:37:54PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> First, this permission never protected a node from being changed, as
> generic child-replacing functions don't check it.
>
> Second, it's a strange thing: it presents a permission of parent node
> to change its child. But generally, children are replaced by different
> mechanisms, like jobs or qmp commands, not by nodes.
>
> Graph-mod permission is hard to understand. All other permissions
> describe operations which done by parent node on its child: read,
> write, resize. Graph modification operations are something completely
> different.
>
> The only place where BLK_PERM_GRAPH_MOD is used as "perm" (not shared
> perm) is mirror_start_job, for s->target. Still modern code should use
> bdrv_freeze_backing_chain() to protect from graph modification, if we
> don't do it somewhere it may be considered as a bug. So, it's a bit
> risky to drop GRAPH_MOD, and analyzing of possible loss of protection
> is hard. But one day we should do it, let's do it now.
>
> One more bit of information is that locking the corresponding byte in
> file-posix doesn't make sense at all.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
>
> v2: fix grammar
>
> qapi/block-core.json | 7 ++-
> include/block/block.h | 9 +
> block.c | 7 +--
> block/commit.c| 1 -
> block/mirror.c| 15 +++
> hw/block/block.c | 3 +--
> scripts/render_block_graph.py | 1 -
> tests/qemu-iotests/273.out| 4
> 8 files changed, 12 insertions(+), 35 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 06674c25c9..6fa2c4ab82 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1825,14 +1825,11 @@
> #
> # @resize: This permission is required to change the size of a block node.
> #
> -# @graph-mod: This permission is required to change the node that this
> -# BdrvChild points to.
> -#
Do we need to mention that graph-mod was removed in 6.2?
> # Since: 4.0
> ##
> { 'enum': 'BlockPermission',
> - 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
> -'graph-mod' ] }
> + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize' ] }
> +
> ##
> # @XDbgBlockGraphEdge:
> #
Otherwise the patch makes sense to me, but I'd rather that Kevin chime in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v6 5/5] block/nbd: check that received handle is valid
On Thu, Sep 02, 2021 at 01:38:05PM +0300, Vladimir Sementsov-Ogievskiy wrote: > If we don't have active request, that waiting for this handle to be > received, we should report an error. If we don't have an active request waiting for this handle to be received, > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/nbd.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) [I'm still taking my time going through 4/5, but hopefully that doesn't impact my quicker review here] Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH v3 27/28] contrib: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
contrib/plugins/lockstep.c | 2 +-
contrib/rdmacm-mux/main.c | 10 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c
index 7fd35eb6692..1c6a9f7a044 100644
--- a/contrib/plugins/lockstep.c
+++ b/contrib/plugins/lockstep.c
@@ -130,7 +130,7 @@ static void report_divergance(ExecState *us, ExecState
*them)
}
}
divergence_log = g_slist_prepend(divergence_log,
- g_memdup(, sizeof(divrec)));
+ g_memdup2(, sizeof(divrec)));
/* Output short log entry of going out of sync... */
if (verbose || divrec.distance == 1 || diverged) {
diff --git a/contrib/rdmacm-mux/main.c b/contrib/rdmacm-mux/main.c
index 771ca01e03f..0899dca2885 100644
--- a/contrib/rdmacm-mux/main.c
+++ b/contrib/rdmacm-mux/main.c
@@ -227,8 +227,8 @@ static RdmaCmMuxErrCode add_fd_ifid_pair(int fd, __be64
gid_ifid)
RDMACM_MUX_ERR_CODE_EACCES;
}
-g_hash_table_insert(server.umad_agent.gid2fd, g_memdup(_ifid,
-sizeof(gid_ifid)), g_memdup(, sizeof(fd)));
+g_hash_table_insert(server.umad_agent.gid2fd, g_memdup2(_ifid,
+sizeof(gid_ifid)), g_memdup2(, sizeof(fd)));
pthread_rwlock_unlock();
@@ -250,7 +250,7 @@ static RdmaCmMuxErrCode delete_fd_ifid_pair(int fd, __be64
gid_ifid)
return RDMACM_MUX_ERR_CODE_ENOTFOUND;
}
-g_hash_table_remove(server.umad_agent.gid2fd, g_memdup(_ifid,
+g_hash_table_remove(server.umad_agent.gid2fd, g_memdup2(_ifid,
sizeof(gid_ifid)));
pthread_rwlock_unlock();
@@ -267,8 +267,8 @@ static void hash_tbl_save_fd_comm_id_pair(int fd, uint32_t
comm_id,
pthread_rwlock_wrlock();
g_hash_table_insert(server.umad_agent.commid2fd,
-g_memdup(_id, sizeof(comm_id)),
-g_memdup(, sizeof(fde)));
+g_memdup2(_id, sizeof(comm_id)),
+g_memdup2(, sizeof(fde)));
pthread_rwlock_unlock();
}
--
2.31.1
[PATCH v3 25/28] target/arm: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
target/arm/helper.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a7ae78146d4..96ff81fe68e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6242,8 +6242,8 @@ static void define_arm_vh_e2h_redirects_aliases(ARMCPU
*cpu)
/* Create alias before redirection so we dup the right data. */
if (a->new_key) {
-ARMCPRegInfo *new_reg = g_memdup(src_reg, sizeof(ARMCPRegInfo));
-uint32_t *new_key = g_memdup(>new_key, sizeof(uint32_t));
+ARMCPRegInfo *new_reg = g_memdup2(src_reg, sizeof(ARMCPRegInfo));
+uint32_t *new_key = g_memdup2(>new_key, sizeof(uint32_t));
bool ok;
new_reg->name = a->new_name;
@@ -8818,7 +8818,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const
ARMCPRegInfo *r,
* add a single reginfo struct to the hash table.
*/
uint32_t *key = g_new(uint32_t, 1);
-ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
+ARMCPRegInfo *r2 = g_memdup2(r, sizeof(ARMCPRegInfo));
int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
--
2.31.1
[PATCH v3 26/28] target/ppc: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- target/ppc/mmu-hash64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c index 19832c4b46f..bc6f8748acb 100644 --- a/target/ppc/mmu-hash64.c +++ b/target/ppc/mmu-hash64.c @@ -1122,7 +1122,7 @@ void ppc_hash64_init(PowerPCCPU *cpu) return; } -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); +cpu->hash64_opts = g_memdup2(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); } void ppc_hash64_finalize(PowerPCCPU *cpu) -- 2.31.1
[RFC PATCH v3 22/28] linux-user: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
do_open_by_handle_at() doesn't check:
size + sizeof(struct file_handle) < 4GiB
---
linux-user/syscall.c | 2 +-
linux-user/uaccess.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ccd3892b2df..d3701007cb3 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7665,7 +7665,7 @@ static abi_long do_open_by_handle_at(abi_long mount_fd,
abi_long handle,
return -TARGET_EFAULT;
}
-fh = g_memdup(target_fh, total_size);
+fh = g_memdup2(target_fh, total_size);
fh->handle_bytes = size;
fh->handle_type = tswap32(target_fh->handle_type);
diff --git a/linux-user/uaccess.c b/linux-user/uaccess.c
index 6a5b029607c..49eddbf4a4d 100644
--- a/linux-user/uaccess.c
+++ b/linux-user/uaccess.c
@@ -15,7 +15,7 @@ void *lock_user(int type, abi_ulong guest_addr, ssize_t len,
bool copy)
host_addr = g2h_untagged(guest_addr);
#ifdef DEBUG_REMAP
if (copy) {
-host_addr = g_memdup(host_addr, len);
+host_addr = g_memdup2(host_addr, len);
} else {
host_addr = g_malloc0(len);
}
--
2.31.1
[RFC PATCH v3 19/28] hw/virtio: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
Should we check in_num/out_num in range?
---
hw/net/virtio-net.c | 3 ++-
hw/virtio/virtio-crypto.c | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee52..338fbeb8c57 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1449,7 +1449,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
}
iov_cnt = elem->out_num;
-iov2 = iov = g_memdup(elem->out_sg, sizeof(struct iovec) *
elem->out_num);
+iov2 = iov = g_memdup2(elem->out_sg,
+ sizeof(struct iovec) * elem->out_num);
s = iov_to_buf(iov, iov_cnt, 0, , sizeof(ctrl));
iov_discard_front(, _cnt, sizeof(ctrl));
if (s != sizeof(ctrl)) {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 54f9bbb789c..59886c1790d 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -242,7 +242,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
}
out_num = elem->out_num;
-out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+out_iov_copy = g_memdup2(elem->out_sg, sizeof(out_iov[0]) * out_num);
out_iov = out_iov_copy;
in_num = elem->in_num;
@@ -605,11 +605,11 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
}
out_num = elem->out_num;
-out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+out_iov_copy = g_memdup2(elem->out_sg, sizeof(out_iov[0]) * out_num);
out_iov = out_iov_copy;
in_num = elem->in_num;
-in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
+in_iov_copy = g_memdup2(elem->in_sg, sizeof(in_iov[0]) * in_num);
in_iov = in_iov_copy;
if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
--
2.31.1
[PATCH v3 20/28] net/colo: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
packet_new() is called from packet_enqueue() with size being 32-bit
(of type SocketReadState::packet_len).
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
net/colo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/colo.c b/net/colo.c
index 3a3e6e89a0c..c04a7fe6dbb 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -159,7 +159,7 @@ Packet *packet_new(const void *data, int size, int
vnet_hdr_len)
{
Packet *pkt = g_slice_new0(Packet);
-pkt->data = g_memdup(data, size);
+pkt->data = g_memdup2(data, size);
pkt->size = size;
pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST);
pkt->vnet_hdr_len = vnet_hdr_len;
@@ -214,7 +214,7 @@ Connection *connection_get(GHashTable
*connection_track_table,
Connection *conn = g_hash_table_lookup(connection_track_table, key);
if (conn == NULL) {
-ConnectionKey *new_key = g_memdup(key, sizeof(*key));
+ConnectionKey *new_key = g_memdup2(key, sizeof(*key));
conn = connection_new(key);
--
2.31.1
[PATCH v3 28/28] checkpatch: Do not allow deprecated g_memdup()
g_memdup() is insecure and as been deprecated in GLib 2.68.
QEMU provides the safely equivalent g_memdup2() wrapper.
Do not allow more g_memdup() calls in the repository, provide
a hint to use g_memdup2().
Signed-off-by: Philippe Mathieu-Daudé
---
scripts/checkpatch.pl | 5 +
1 file changed, 5 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb8eff233e0..5caa739db48 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2850,6 +2850,11 @@ sub process {
WARN("consider using g_path_get_$1() in preference to
g_strdup($1())\n" . $herecurr);
}
+# enforce g_memdup2() over g_memdup()
+ if ($line =~ /\bg_memdup\s*\(/) {
+ ERROR("use g_memdup2() instead of unsafe g_memdup()\n"
. $herecurr);
+ }
+
# recommend qemu_strto* over strto* for numeric conversions
if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
ERROR("consider using qemu_$1 in preference to $1\n" .
$herecurr);
--
2.31.1
[PATCH v3 24/28] tests/qtest: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/libqos/ahci.c | 6 +++---
tests/qtest/libqos/qgraph.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index fba3e7a954e..eaa2096512e 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -639,8 +639,8 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
AHCIOpts *opts;
uint64_t buffer_in;
-opts = g_memdup((opts_in == NULL ? _opts : opts_in),
-sizeof(AHCIOpts));
+opts = g_memdup2((opts_in == NULL ? _opts : opts_in),
+ sizeof(AHCIOpts));
buffer_in = opts->buffer;
@@ -860,7 +860,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
g_assert(!props->ncq || props->lba48);
/* Defaults and book-keeping */
-cmd->props = g_memdup(props, sizeof(AHCICommandProp));
+cmd->props = g_memdup2(props, sizeof(AHCICommandProp));
cmd->name = command_name;
cmd->xbytes = props->size;
cmd->prd_size = 4096;
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index d1dc4919305..109ff04e1e8 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -93,7 +93,7 @@ static void add_edge(const char *source, const char *dest,
edge->type = type;
edge->dest = g_strdup(dest);
edge->edge_name = g_strdup(opts->edge_name ?: dest);
-edge->arg = g_memdup(opts->arg, opts->size_arg);
+edge->arg = g_memdup2(opts->arg, opts->size_arg);
edge->before_cmd_line =
opts->before_cmd_line ? g_strconcat(" ", opts->before_cmd_line, NULL)
: NULL;
--
2.31.1
[PATCH v3 18/28] hw/vfio/pci: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/vfio/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e1ea1d8a23b..f7d0ef8cc61 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2040,7 +2040,7 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) * physical device, we cache the config space to avoid overwriting * the original config space when we parse the extended capabilities. */ -config = g_memdup(pdev->config, vdev->config_size); +config = g_memdup2(pdev->config, vdev->config_size); /* * Extended capabilities are chained with each pointing to the next, so we -- 2.31.1
[PATCH v3 23/28] tests/unit: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/ptimer-test.c | 22 +++---
tests/unit/test-iov.c| 26 +-
2 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/tests/unit/ptimer-test.c b/tests/unit/ptimer-test.c
index 9176b96c1ce..9ba5ffe273b 100644
--- a/tests/unit/ptimer-test.c
+++ b/tests/unit/ptimer-test.c
@@ -798,64 +798,64 @@ static void add_ptimer_tests(uint8_t policy)
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
-g_memdup(, 1), check_set_count, g_free);
+g_memdup2(, 1), check_set_count, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
-g_memdup(, 1), check_set_limit, g_free);
+g_memdup2(, 1), check_set_limit, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
-g_memdup(, 1), check_oneshot, g_free);
+g_memdup2(, 1), check_oneshot, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
-g_memdup(, 1), check_periodic, g_free);
+g_memdup2(, 1), check_periodic, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_mode_change, g_free);
+g_memdup2(, 1), check_on_the_fly_mode_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_period_change, g_free);
+g_memdup2(, 1), check_on_the_fly_period_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_freq_change, g_free);
+g_memdup2(, 1), check_on_the_fly_freq_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/run_with_period_0 policy=%s",
policy_name),
-g_memdup(, 1), check_run_with_period_0, g_free);
+g_memdup2(, 1), check_run_with_period_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/run_with_delta_0 policy=%s",
policy_name),
-g_memdup(, 1), check_run_with_delta_0, g_free);
+g_memdup2(, 1), check_run_with_delta_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s",
policy_name),
-g_memdup(, 1), check_periodic_with_load_0, g_free);
+g_memdup2(, 1), check_periodic_with_load_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s",
policy_name),
-g_memdup(, 1), check_oneshot_with_load_0, g_free);
+g_memdup2(, 1), check_oneshot_with_load_0, g_free);
g_free(tmp);
}
diff --git a/tests/unit/test-iov.c b/tests/unit/test-iov.c
index 5371066fb6a..aa679b56131 100644
--- a/tests/unit/test-iov.c
+++ b/tests/unit/test-iov.c
@@ -173,7 +173,7 @@ static void test_io(void)
}
iov_from_buf(iov, niov, 0, buf, sz);
-siov = g_memdup(iov, sizeof(*iov) * niov);
+siov = g_memdup2(iov, sizeof(*iov) * niov);
if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) {
perror("socketpair");
@@ -350,7 +350,7 @@ static void test_discard_front_undo(void)
/* Discard zero bytes */
iov_random(, _cnt);
-iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_tmp = iov;
iov_cnt_tmp = iov_cnt;
iov_discard_front_undoable(_tmp, _cnt_tmp, 0, );
@@ -361,7 +361,7 @@ static void test_discard_front_undo(void)
/* Discard more bytes than vector size */
iov_random(, _cnt);
-iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+iov_orig
[PATCH v3 14/28] hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/nvram/fw_cfg.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 9b8dcca4ead..0c3cfa8a41e 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -205,7 +205,8 @@ static void fw_cfg_bootsplash(FWCfgState *s) /* use little endian format */ bst_le16 = cpu_to_le16(bst_val); fw_cfg_add_file(s, "etc/boot-menu-wait", -g_memdup(_le16, sizeof bst_le16), sizeof bst_le16); +g_memdup2(_le16, sizeof bst_le16), +sizeof bst_le16); } /* insert splash file if user configurated */ @@ -260,7 +261,7 @@ static void fw_cfg_reboot(FWCfgState *s) } rt_le32 = cpu_to_le32(rt_val); -fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(_le32, 4), 4); +fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup2(_le32, 4), 4); } static void fw_cfg_write(FWCfgState *s, uint8_t value) @@ -755,7 +756,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value) size_t sz = strlen(value) + 1; trace_fw_cfg_add_string(key, trace_key_name(key), value); -fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); +fw_cfg_add_bytes(s, key, g_memdup2(value, sz), sz); } void fw_cfg_modify_string(FWCfgState *s, uint16_t key, const char *value) @@ -763,7 +764,7 @@ void fw_cfg_modify_string(FWCfgState *s, uint16_t key, const char *value) size_t sz = strlen(value) + 1; char *old; -old = fw_cfg_modify_bytes_read(s, key, g_memdup(value, sz), sz); +old = fw_cfg_modify_bytes_read(s, key, g_memdup2(value, sz), sz); g_free(old); } -- 2.31.1
[PATCH v3 17/28] hw/rdma: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/rdma/rdma_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
index 98df58f6897..6d6b8286b69 100644
--- a/hw/rdma/rdma_utils.c
+++ b/hw/rdma/rdma_utils.c
@@ -71,7 +71,7 @@ void rdma_protected_gqueue_append_int64(RdmaProtectedGQueue
*list,
int64_t value)
{
qemu_mutex_lock(>lock);
-g_queue_push_tail(list->list, g_memdup(, sizeof(value)));
+g_queue_push_tail(list->list, g_memdup2(, sizeof(value)));
qemu_mutex_unlock(>lock);
}
--
2.31.1
[PATCH v3 13/28] hw/net/eepro100: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/net/eepro100.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 16e95ef9cc9..a4e67f69752 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -1872,7 +1872,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp) qemu_register_reset(nic_reset, s); -s->vmstate = g_memdup(_eepro100, sizeof(vmstate_eepro100)); +s->vmstate = g_memdup2(_eepro100, sizeof(vmstate_eepro100)); s->vmstate->name = qemu_get_queue(s->nic)->model; vmstate_register(VMSTATE_IF(_dev->qdev), VMSTATE_INSTANCE_ID_ANY, s->vmstate, s); -- 2.31.1
[RFC PATCH v3 21/28] ui/clipboard: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- TODO: audit qemu_clipboard_set_data() calls --- ui/clipboard.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/clipboard.c b/ui/clipboard.c index d7b008d62a0..d8e11bb6596 100644 --- a/ui/clipboard.c +++ b/ui/clipboard.c @@ -123,7 +123,7 @@ void qemu_clipboard_set_data(QemuClipboardPeer *peer, } g_free(info->types[type].data); -info->types[type].data = g_memdup(data, size); +info->types[type].data = g_memdup2(data, size); info->types[type].size = size; info->types[type].available = true; -- 2.31.1
[PATCH v3 10/28] hw/core/machine: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/core/machine.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 067f42b528f..c3e5371b177 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -615,8 +615,8 @@ HotpluggableCPUList
*machine_query_hotpluggable_cpus(MachineState *machine)
cpu_item->type = g_strdup(machine->possible_cpus->cpus[i].type);
cpu_item->vcpus_count = machine->possible_cpus->cpus[i].vcpus_count;
-cpu_item->props = g_memdup(>possible_cpus->cpus[i].props,
- sizeof(*cpu_item->props));
+cpu_item->props = g_memdup2(>possible_cpus->cpus[i].props,
+sizeof(*cpu_item->props));
cpu = machine->possible_cpus->cpus[i].cpu;
if (cpu) {
--
2.31.1
[PATCH v3 08/28] hw/acpi: Avoid truncating acpi_data_len() to 32-bit
acpi_data_len() returns an unsigned type, which might be bigger
than 32-bit (although it is unlikely such value is returned).
Hold the returned value in an 'unsigned' type to avoid unlikely
size truncation.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/arm/virt-acpi-build.c | 2 +-
hw/i386/acpi-build.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 037cc1fd82c..95543d43e2a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -885,7 +885,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables
*tables)
static void acpi_ram_update(MemoryRegion *mr, GArray *data)
{
-uint32_t size = acpi_data_len(data);
+unsigned size = acpi_data_len(data);
/* Make sure RAM size is correct - in case it got changed
* e.g. by migration */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a33ac8b91e1..aa269914b49 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2660,7 +2660,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState
*machine)
static void acpi_ram_update(MemoryRegion *mr, GArray *data)
{
-uint32_t size = acpi_data_len(data);
+unsigned size = acpi_data_len(data);
/* Make sure RAM size is correct - in case it got changed e.g. by
migration */
memory_region_ram_resize(mr, size, _abort);
@@ -2783,7 +2783,7 @@ void acpi_setup(void)
* Though RSDP is small, its contents isn't immutable, so
* we'll update it along with the rest of tables on guest access.
*/
-uint32_t rsdp_size = acpi_data_len(tables.rsdp);
+unsigned rsdp_size = acpi_data_len(tables.rsdp);
build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE,
--
2.31.1
[PATCH v3 07/28] hw/9pfs: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/9pfs/9p-synth.c | 2 +-
hw/9pfs/9p.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b38088e0664..d6168c653d2 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -497,7 +497,7 @@ static int synth_name_to_path(FsContext *ctx, V9fsPath
*dir_path,
out:
/* Copy the node pointer to fid */
g_free(target->data);
-target->data = g_memdup(, sizeof(void *));
+target->data = g_memdup2(, sizeof(void *));
target->size = sizeof(void *);
return 0;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c857b313213..a80166fcaff 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -202,7 +202,7 @@ void v9fs_path_copy(V9fsPath *dst, const V9fsPath *src)
{
v9fs_path_free(dst);
dst->size = src->size;
-dst->data = g_memdup(src->data, src->size);
+dst->data = g_memdup2(src->data, src->size);
}
int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
--
2.31.1
[PATCH v3 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/ppc/spapr_pci.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7430bd63142..8e36cffab79 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2201,10 +2201,9 @@ static int spapr_pci_post_load(void *opaque, int
version_id)
int i;
for (i = 0; i < sphb->msi_devs_num; ++i) {
-key = g_memdup(>msi_devs[i].key,
- sizeof(sphb->msi_devs[i].key));
-value = g_memdup(>msi_devs[i].value,
- sizeof(sphb->msi_devs[i].value));
+key = g_memdup2(>msi_devs[i].key, sizeof(sphb->msi_devs[i].key));
+value = g_memdup2(>msi_devs[i].value,
+ sizeof(sphb->msi_devs[i].value));
g_hash_table_insert(sphb->msi, key, value);
}
g_free(sphb->msi_devs);
--
2.31.1
[PATCH v3 15/28] hw/scsi/mptsas: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/mptsas.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index db3219e7d20..f53ea358161 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -449,7 +449,8 @@ static void mptsas_process_scsi_task_mgmt(MPTSASState *s,
MPIMsgSCSITaskMgmt *re
} else {
MPTSASCancelNotifier *notifier;
-reply_async = g_memdup(,
sizeof(MPIMsgSCSITaskMgmtReply));
+reply_async = g_memdup2(,
+sizeof(MPIMsgSCSITaskMgmtReply));
reply_async->IOCLogInfo = INT_MAX;
count = 1;
@@ -476,7 +477,7 @@ static void mptsas_process_scsi_task_mgmt(MPTSASState *s,
MPIMsgSCSITaskMgmt *re
goto out;
}
-reply_async = g_memdup(, sizeof(MPIMsgSCSITaskMgmtReply));
+reply_async = g_memdup2(, sizeof(MPIMsgSCSITaskMgmtReply));
reply_async->IOCLogInfo = INT_MAX;
count = 0;
--
2.31.1
[PATCH v3 12/28] hw/i386/multiboot: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/multiboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 9e7d69d4705..754415d17f3 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -387,7 +387,7 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug(" mb_mods_count = %d", mbs.mb_mods_count);
/* save bootinfo off the stack */
-mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
+mb_bootinfo_data = g_memdup2(bootinfo, sizeof(bootinfo));
/* Pass variables to option rom */
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
--
2.31.1
[PATCH v3 11/28] hw/hppa/machine: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/hppa/machine.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c index 2a46af5bc9b..e602e863a7d 100644 --- a/hw/hppa/machine.c +++ b/hw/hppa/machine.c @@ -101,19 +101,19 @@ static FWCfgState *create_fw_cfg(MachineState *ms) val = cpu_to_le64(MIN_SEABIOS_HPPA_VERSION); fw_cfg_add_file(fw_cfg, "/etc/firmware-min-version", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPPA_TLB_ENTRIES); fw_cfg_add_file(fw_cfg, "/etc/cpu/tlb_entries", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPPA_BTLB_ENTRIES); fw_cfg_add_file(fw_cfg, "/etc/cpu/btlb_entries", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPA_POWER_BUTTON); fw_cfg_add_file(fw_cfg, "/etc/power-button-addr", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2(, sizeof(val)), sizeof(val)); fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ms->boot_order[0]); qemu_register_boot_set(fw_cfg_boot_set, fw_cfg); -- 2.31.1
[PATCH v3 09/28] hw/acpi: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/acpi/core.c | 3 ++- hw/i386/acpi-build.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 1e004d0078d..50ee821aae5 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -637,7 +637,8 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent, suspend[3] = 1 | ((!disable_s3) << 7); suspend[4] = s4_val | ((!disable_s4) << 7); -fw_cfg_add_file(fw_cfg, "etc/system-states", g_memdup(suspend, 6), 6); +fw_cfg_add_file(fw_cfg, "etc/system-states", +g_memdup2(suspend, 6), 6); } } diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index aa269914b49..dd5c06c8cd5 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2785,7 +2785,7 @@ void acpi_setup(void) */ unsigned rsdp_size = acpi_data_len(tables.rsdp); -build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size); +build_state->rsdp = g_memdup2(tables.rsdp->data, rsdp_size); fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE, acpi_build_update, NULL, build_state, build_state->rsdp, rsdp_size, true); -- 2.31.1
[PATCH v3 05/28] block/qcow2-bitmap: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- block/qcow2-bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 8fb47315515..218a0dc712a 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1599,7 +1599,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, name); goto fail; } -tb = g_memdup(>table, sizeof(bm->table)); +tb = g_memdup2(>table, sizeof(bm->table)); bm->table.offset = 0; bm->table.size = 0; QSIMPLEQ_INSERT_TAIL(_tables, tb, entry); -- 2.31.1
[PATCH v3 04/28] accel/tcg: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
accel/tcg/cputlb.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b1e5471f949..08951f0683e 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -826,7 +826,7 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, target_ulong
addr,
tlb_flush_range_by_mmuidx_async_0(cpu, d);
} else {
/* Otherwise allocate a structure, freed by the worker. */
-TLBFlushRangeData *p = g_memdup(, sizeof(d));
+TLBFlushRangeData *p = g_memdup2(, sizeof(d));
async_run_on_cpu(cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
@@ -868,7 +868,7 @@ void tlb_flush_range_by_mmuidx_all_cpus(CPUState *src_cpu,
/* Allocate a separate data block for each destination cpu. */
CPU_FOREACH(dst_cpu) {
if (dst_cpu != src_cpu) {
-TLBFlushRangeData *p = g_memdup(, sizeof(d));
+TLBFlushRangeData *p = g_memdup2(, sizeof(d));
async_run_on_cpu(dst_cpu,
tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
@@ -918,13 +918,13 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState
*src_cpu,
/* Allocate a separate data block for each destination cpu. */
CPU_FOREACH(dst_cpu) {
if (dst_cpu != src_cpu) {
-p = g_memdup(, sizeof(d));
+p = g_memdup2(, sizeof(d));
async_run_on_cpu(dst_cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
}
-p = g_memdup(, sizeof(d));
+p = g_memdup2(, sizeof(d));
async_safe_run_on_cpu(src_cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
--
2.31.1
[PATCH v3 03/28] qapi: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- qapi/qapi-clone-visitor.c | 16 qapi/qapi-visit-core.c| 6 -- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c index c45c5caa3b8..b014119d368 100644 --- a/qapi/qapi-clone-visitor.c +++ b/qapi/qapi-clone-visitor.c @@ -37,7 +37,7 @@ static bool qapi_clone_start_struct(Visitor *v, const char *name, void **obj, return true; } -*obj = g_memdup(*obj, size); +*obj = g_memdup2(*obj, size); qcv->depth++; return true; } @@ -65,8 +65,8 @@ static GenericList *qapi_clone_next_list(Visitor *v, GenericList *tail, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Unshare the tail of the list cloned by g_memdup() */ -tail->next = g_memdup(tail->next, size); +/* Unshare the tail of the list cloned by g_memdup2() */ +tail->next = g_memdup2(tail->next, size); return tail->next; } @@ -83,7 +83,7 @@ static bool qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -93,7 +93,7 @@ static bool qapi_clone_type_uint64(Visitor *v, const char *name, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -103,7 +103,7 @@ static bool qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -114,7 +114,7 @@ static bool qapi_clone_type_str(Visitor *v, const char *name, char **obj, assert(qcv->depth); /* - * Pointer was already cloned by g_memdup; create fresh copy. + * Pointer was already cloned by g_memdup2; create fresh copy. * Note that as long as qobject-output-visitor accepts NULL instead of * "", then we must do likewise. However, we want to obey the * input visitor semantics of never producing NULL when the empty @@ -130,7 +130,7 @@ static bool qapi_clone_type_number(Visitor *v, const char *name, double *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index a641adec51e..ebabe63b6ea 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -413,8 +413,10 @@ bool visit_type_enum(Visitor *v, const char *name, int *obj, case VISITOR_OUTPUT: return output_type_enum(v, name, obj, lookup, errp); case VISITOR_CLONE: -/* nothing further to do, scalar value was already copied by - * g_memdup() during visit_start_*() */ +/* + * nothing further to do, scalar value was already copied by + * g_memdup2() during visit_start_*() + */ return true; case VISITOR_DEALLOC: /* nothing to deallocate for a scalar */ -- 2.31.1
[PATCH v3 01/28] hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
vmbus_save_req() and vmbus_load_req() are not used.
Remove them to avoid maintaining dead code.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/hyperv/vmbus.h | 3 --
hw/hyperv/vmbus.c | 59 ---
2 files changed, 62 deletions(-)
diff --git a/include/hw/hyperv/vmbus.h b/include/hw/hyperv/vmbus.h
index f98bea3888d..8ea660dd8e6 100644
--- a/include/hw/hyperv/vmbus.h
+++ b/include/hw/hyperv/vmbus.h
@@ -223,7 +223,4 @@ int vmbus_map_sgl(VMBusChanReq *req, DMADirection dir,
struct iovec *iov,
void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
unsigned iov_cnt, size_t accessed);
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req);
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size);
-
#endif
diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index c9887d5a7bc..18d3c3b9240 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1311,65 +1311,6 @@ static const VMStateDescription vmstate_vmbus_chan_req =
{
}
};
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req)
-{
-VMBusChanReqSave req_save;
-
-req_save.chan_idx = req->chan->subchan_idx;
-req_save.pkt_type = req->pkt_type;
-req_save.msglen = req->msglen;
-req_save.msg = req->msg;
-req_save.transaction_id = req->transaction_id;
-req_save.need_comp = req->need_comp;
-req_save.num = req->sgl.nsg;
-req_save.sgl = g_memdup(req->sgl.sg,
-req_save.num * sizeof(ScatterGatherEntry));
-
-vmstate_save_state(f, _vmbus_chan_req, _save, NULL);
-
-g_free(req_save.sgl);
-}
-
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size)
-{
-VMBusChanReqSave req_save;
-VMBusChanReq *req = NULL;
-VMBusChannel *chan = NULL;
-uint32_t i;
-
-vmstate_load_state(f, _vmbus_chan_req, _save, 0);
-
-if (req_save.chan_idx >= dev->num_channels) {
-error_report("%s: %u(chan_idx) > %u(num_channels)", __func__,
- req_save.chan_idx, dev->num_channels);
-goto out;
-}
-chan = >channels[req_save.chan_idx];
-
-if (vmbus_channel_reserve(chan, 0, req_save.msglen)) {
-goto out;
-}
-
-req = vmbus_alloc_req(chan, size, req_save.pkt_type, req_save.msglen,
- req_save.transaction_id, req_save.need_comp);
-if (req_save.msglen) {
-memcpy(req->msg, req_save.msg, req_save.msglen);
-}
-
-for (i = 0; i < req_save.num; i++) {
-qemu_sglist_add(>sgl, req_save.sgl[i].base, req_save.sgl[i].len);
-}
-
-out:
-if (req_save.msglen) {
-g_free(req_save.msg);
-}
-if (req_save.num) {
-g_free(req_save.sgl);
-}
-return req;
-}
-
static void channel_event_cb(EventNotifier *e)
{
VMBusChannel *chan = container_of(e, VMBusChannel, notifier);
--
2.31.1
[PATCH v3 02/28] glib-compat: Introduce g_memdup2() wrapper
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 37 +
1 file changed, 37 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..8d01a8c01fb 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,43 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+#define g_memdup2(m, s) g_memdup2_qemu(m, s)
+
#if defined(G_OS_UNIX)
/*
* Note: The fallback implementation is not MT-safe, and it returns a copy of
--
2.31.1
[PATCH v3 06/28] softmmu: Replace g_memdup() by g_memdup2()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/memory.c | 2 +-
softmmu/vl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..1db019393b6 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1140,7 +1140,7 @@ static char *memory_region_escape_name(const char *name)
bytes += memory_region_need_escape(*p) ? 4 : 1;
}
if (bytes == p - name) {
- return g_memdup(name, bytes + 1);
+return g_memdup2(name, bytes + 1);
}
escaped = g_malloc(bytes + 1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ea05bb39c50..7a44c63a6ad 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1154,7 +1154,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
}
if (nonempty_str(str)) {
size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-buf = g_memdup(str, size);
+buf = g_memdup2(str, size);
} else if (nonempty_str(gen_id)) {
if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
return -1;
--
2.31.1
[PATCH v2 01/30] hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
vmbus_save_req() and vmbus_load_req() are not used.
Remove them to avoid maintaining dead code.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/hyperv/vmbus.h | 3 --
hw/hyperv/vmbus.c | 59 ---
2 files changed, 62 deletions(-)
diff --git a/include/hw/hyperv/vmbus.h b/include/hw/hyperv/vmbus.h
index f98bea3888d..8ea660dd8e6 100644
--- a/include/hw/hyperv/vmbus.h
+++ b/include/hw/hyperv/vmbus.h
@@ -223,7 +223,4 @@ int vmbus_map_sgl(VMBusChanReq *req, DMADirection dir,
struct iovec *iov,
void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
unsigned iov_cnt, size_t accessed);
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req);
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size);
-
#endif
diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index c9887d5a7bc..18d3c3b9240 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1311,65 +1311,6 @@ static const VMStateDescription vmstate_vmbus_chan_req =
{
}
};
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req)
-{
-VMBusChanReqSave req_save;
-
-req_save.chan_idx = req->chan->subchan_idx;
-req_save.pkt_type = req->pkt_type;
-req_save.msglen = req->msglen;
-req_save.msg = req->msg;
-req_save.transaction_id = req->transaction_id;
-req_save.need_comp = req->need_comp;
-req_save.num = req->sgl.nsg;
-req_save.sgl = g_memdup(req->sgl.sg,
-req_save.num * sizeof(ScatterGatherEntry));
-
-vmstate_save_state(f, _vmbus_chan_req, _save, NULL);
-
-g_free(req_save.sgl);
-}
-
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size)
-{
-VMBusChanReqSave req_save;
-VMBusChanReq *req = NULL;
-VMBusChannel *chan = NULL;
-uint32_t i;
-
-vmstate_load_state(f, _vmbus_chan_req, _save, 0);
-
-if (req_save.chan_idx >= dev->num_channels) {
-error_report("%s: %u(chan_idx) > %u(num_channels)", __func__,
- req_save.chan_idx, dev->num_channels);
-goto out;
-}
-chan = >channels[req_save.chan_idx];
-
-if (vmbus_channel_reserve(chan, 0, req_save.msglen)) {
-goto out;
-}
-
-req = vmbus_alloc_req(chan, size, req_save.pkt_type, req_save.msglen,
- req_save.transaction_id, req_save.need_comp);
-if (req_save.msglen) {
-memcpy(req->msg, req_save.msg, req_save.msglen);
-}
-
-for (i = 0; i < req_save.num; i++) {
-qemu_sglist_add(>sgl, req_save.sgl[i].base, req_save.sgl[i].len);
-}
-
-out:
-if (req_save.msglen) {
-g_free(req_save.msg);
-}
-if (req_save.num) {
-g_free(req_save.sgl);
-}
-return req;
-}
-
static void channel_event_cb(EventNotifier *e)
{
VMBusChannel *chan = container_of(e, VMBusChannel, notifier);
--
2.31.1
[PATCH v3 00/28] glib: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. g_memdup() as been deprecated in GLib 2.68. Since QEMU defines GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_56, the deprecation is not displayed (on GLib >= 2.68 such available on Fedora 34). However the function is still unsafe, so it is better to avoid its use. This series provides the safely equivalent g_memdup2() wrapper, and replace all g_memdup() calls by it. The previous link recommend to audit the call sites. Most of the calls use byte_size=sizeof(STRUCT), and no STRUCT appears to be > 4GiB. Few calls use unsigned/size_t/uint16_t. Where code is doing multiplication, patches are sent as RFC. In particular: hw/net/virtio-net.c hw/virtio/virtio-crypto.c Since v1: - Added missing g_memdup2 -> g_memdup2_qemu compat definition (danpb) - Do not call g_memdup2_qemu() but directly g_memdup2() (danpb) Tested with the following snippet: -- >8 -- diff --git a/include/glib-compat.h b/include/glib-compat.h index 8d01a8c01fb..2b33dea71a8 100644 --- a/include/glib-compat.h +++ b/include/glib-compat.h @@ -21,3 +21,3 @@ */ -#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 +#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_68 @@ -26,3 +26,3 @@ */ -#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_56 +#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_68 --- Philippe Mathieu-Daudé (28): hw/hyperv/vmbus: Remove unused vmbus_load/save_req() glib-compat: Introduce g_memdup2() wrapper qapi: Replace g_memdup() by g_memdup2() accel/tcg: Replace g_memdup() by g_memdup2() block/qcow2-bitmap: Replace g_memdup() by g_memdup2() softmmu: Replace g_memdup() by g_memdup2() hw/9pfs: Replace g_memdup() by g_memdup2() hw/acpi: Avoid truncating acpi_data_len() to 32-bit hw/acpi: Replace g_memdup() by g_memdup2() hw/core/machine: Replace g_memdup() by g_memdup2() hw/hppa/machine: Replace g_memdup() by g_memdup2() hw/i386/multiboot: Replace g_memdup() by g_memdup2() hw/net/eepro100: Replace g_memdup() by g_memdup2() hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2() hw/scsi/mptsas: Replace g_memdup() by g_memdup2() hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2() hw/rdma: Replace g_memdup() by g_memdup2() hw/vfio/pci: Replace g_memdup() by g_memdup2() hw/virtio: Replace g_memdup() by g_memdup2() net/colo: Replace g_memdup() by g_memdup2() ui/clipboard: Replace g_memdup() by g_memdup2() linux-user: Replace g_memdup() by g_memdup2() tests/unit: Replace g_memdup() by g_memdup2() tests/qtest: Replace g_memdup() by g_memdup2() target/arm: Replace g_memdup() by g_memdup2() target/ppc: Replace g_memdup() by g_memdup2() contrib: Replace g_memdup() by g_memdup2() checkpatch: Do not allow deprecated g_memdup() include/glib-compat.h | 37 +++ include/hw/hyperv/vmbus.h | 3 -- accel/tcg/cputlb.c | 8 ++--- block/qcow2-bitmap.c| 2 +- contrib/plugins/lockstep.c | 2 +- contrib/rdmacm-mux/main.c | 10 +++ hw/9pfs/9p-synth.c | 2 +- hw/9pfs/9p.c| 2 +- hw/acpi/core.c | 3 +- hw/arm/virt-acpi-build.c| 2 +- hw/core/machine.c | 4 +-- hw/hppa/machine.c | 8 ++--- hw/hyperv/vmbus.c | 59 - hw/i386/acpi-build.c| 6 ++-- hw/i386/multiboot.c | 2 +- hw/net/eepro100.c | 2 +- hw/net/virtio-net.c | 3 +- hw/nvram/fw_cfg.c | 9 +++--- hw/ppc/spapr_pci.c | 7 ++--- hw/rdma/rdma_utils.c| 2 +- hw/scsi/mptsas.c| 5 ++-- hw/vfio/pci.c | 2 +- hw/virtio/virtio-crypto.c | 6 ++-- linux-user/syscall.c| 2 +- linux-user/uaccess.c| 2 +- net/colo.c | 4 +-- qapi/qapi-clone-visitor.c | 16 +- qapi/qapi-visit-core.c | 6 ++-- softmmu/memory.c| 2 +- softmmu/vl.c| 2 +- target/arm/helper.c | 6 ++-- target/ppc/mmu-hash64.c | 2 +- tests/qtest/libqos/ahci.c | 6 ++-- tests/qtest/libqos/qgraph.c | 2 +- tests/unit/ptimer-test.c| 22 +++--- tests/unit/test-iov.c | 26 ui/clipboard.c | 2 +- scripts/checkpatch.pl | 5 38 files changed, 138 insertions(+), 153 deletions(-) -- 2.31.1
Re: [PATCH v2 00/30] glib: Replace g_memdup() by g_memdup2()
On 9/3/21 7:39 PM, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > g_memdup() as been deprecated in GLib 2.68. Since QEMU defines > GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_56, the deprecation > is not displayed (on GLib >= 2.68 such available on Fedora 34). > However the function is still unsafe, so it is better to avoid > its use. > > This series provides the safely equivalent g_memdup2() wrapper, > and replace all g_memdup() calls by it. > > The previous link recommend to audit the call sites. Most of the > calls use byte_size=sizeof(STRUCT), and no STRUCT appears to be >> 4GiB. Few calls use unsigned/size_t/uint16_t. Where code is > doing multiplication, patches are sent as RFC. In particular: > hw/net/virtio-net.c > hw/virtio/virtio-crypto.c > > Since v1: > - Added missing g_memdup2 -> g_memdup2_qemu compat definition (danpb) > - Do not call g_memdup2_qemu() but directly g_memdup2() (danpb) > > Philippe Mathieu-Daudé (30): > hw/hyperv/vmbus: Remove unused vmbus_load/save_req() > glib-compat: Introduce g_memdup2() wrapper > qapi: Replace g_memdup() by g_memdup2() > accel/tcg: Replace g_memdup() by g_memdup2() > block/qcow2-bitmap: Replace g_memdup() by g_memdup2() > softmmu: Replace g_memdup() by g_memdup2() > hw/9pfs: Replace g_memdup() by g_memdup2() > hw/acpi: Avoid truncating acpi_data_len() to 32-bit > hw/acpi: Replace g_memdup() by g_memdup2() > hw/core/machine: Replace g_memdup() by g_memdup2() > hw/hppa/machine: Replace g_memdup() by g_memdup2() > hw/i386/multiboot: Replace g_memdup() by g_memdup2() > hw/net/eepro100: Replace g_memdup() by g_memdup2() > hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2() > hw/scsi/mptsas: Replace g_memdup() by g_memdup2() > hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2() > hw/rdma: Replace g_memdup() by g_memdup2() > hw/vfio/pci: Replace g_memdup() by g_memdup2() > RFC hw/virtio: Replace g_memdup() by g_memdup2() > net/colo: Replace g_memdup() by g_memdup2() > RFC ui/clipboard: Replace g_memdup() by g_memdup2() > RFC linux-user: Replace g_memdup() by g_memdup2() > tests/unit: Replace g_memdup() by g_memdup2() > tests/qtest: Replace g_memdup() by g_memdup2() > target/arm: Replace g_memdup() by g_memdup2() > target/ppc: Replace g_memdup() by g_memdup2() > contrib: Replace g_memdup() by g_memdup2() > checkpatch: Do not allow deprecated g_memdup() > f > test I figured too late I was not placed in the correct commit, please disregard this incomplete series...
[PATCH v2 02/30] glib-compat: Introduce g_memdup2() wrapper
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 37 +
1 file changed, 37 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..915d0156da0 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,43 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+#define g_memdup2(a) g_memdup2_qemu(a)
+
#if defined(G_OS_UNIX)
/*
* Note: The fallback implementation is not MT-safe, and it returns a copy of
--
2.31.1
[PATCH v2 00/30] glib: Replace g_memdup() by g_memdup2()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. g_memdup() as been deprecated in GLib 2.68. Since QEMU defines GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_56, the deprecation is not displayed (on GLib >= 2.68 such available on Fedora 34). However the function is still unsafe, so it is better to avoid its use. This series provides the safely equivalent g_memdup2() wrapper, and replace all g_memdup() calls by it. The previous link recommend to audit the call sites. Most of the calls use byte_size=sizeof(STRUCT), and no STRUCT appears to be > 4GiB. Few calls use unsigned/size_t/uint16_t. Where code is doing multiplication, patches are sent as RFC. In particular: hw/net/virtio-net.c hw/virtio/virtio-crypto.c Since v1: - Added missing g_memdup2 -> g_memdup2_qemu compat definition (danpb) - Do not call g_memdup2_qemu() but directly g_memdup2() (danpb) Philippe Mathieu-Daudé (30): hw/hyperv/vmbus: Remove unused vmbus_load/save_req() glib-compat: Introduce g_memdup2() wrapper qapi: Replace g_memdup() by g_memdup2() accel/tcg: Replace g_memdup() by g_memdup2() block/qcow2-bitmap: Replace g_memdup() by g_memdup2() softmmu: Replace g_memdup() by g_memdup2() hw/9pfs: Replace g_memdup() by g_memdup2() hw/acpi: Avoid truncating acpi_data_len() to 32-bit hw/acpi: Replace g_memdup() by g_memdup2() hw/core/machine: Replace g_memdup() by g_memdup2() hw/hppa/machine: Replace g_memdup() by g_memdup2() hw/i386/multiboot: Replace g_memdup() by g_memdup2() hw/net/eepro100: Replace g_memdup() by g_memdup2() hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2() hw/scsi/mptsas: Replace g_memdup() by g_memdup2() hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2() hw/rdma: Replace g_memdup() by g_memdup2() hw/vfio/pci: Replace g_memdup() by g_memdup2() RFC hw/virtio: Replace g_memdup() by g_memdup2() net/colo: Replace g_memdup() by g_memdup2() RFC ui/clipboard: Replace g_memdup() by g_memdup2() RFC linux-user: Replace g_memdup() by g_memdup2() tests/unit: Replace g_memdup() by g_memdup2() tests/qtest: Replace g_memdup() by g_memdup2() target/arm: Replace g_memdup() by g_memdup2() target/ppc: Replace g_memdup() by g_memdup2() contrib: Replace g_memdup() by g_memdup2() checkpatch: Do not allow deprecated g_memdup() f test include/glib-compat.h | 41 -- include/hw/hyperv/vmbus.h | 3 -- accel/tcg/cputlb.c | 8 ++--- block/qcow2-bitmap.c| 2 +- contrib/plugins/lockstep.c | 2 +- contrib/rdmacm-mux/main.c | 10 +++ hw/9pfs/9p-synth.c | 2 +- hw/9pfs/9p.c| 2 +- hw/acpi/core.c | 3 +- hw/arm/virt-acpi-build.c| 2 +- hw/core/machine.c | 4 +-- hw/hppa/machine.c | 8 ++--- hw/hyperv/vmbus.c | 59 - hw/i386/acpi-build.c| 6 ++-- hw/i386/multiboot.c | 2 +- hw/net/eepro100.c | 2 +- hw/net/virtio-net.c | 3 +- hw/nvram/fw_cfg.c | 9 +++--- hw/ppc/spapr_pci.c | 7 ++--- hw/rdma/rdma_utils.c| 2 +- hw/scsi/mptsas.c| 5 ++-- hw/vfio/pci.c | 2 +- hw/virtio/virtio-crypto.c | 6 ++-- linux-user/syscall.c| 2 +- linux-user/uaccess.c| 2 +- net/colo.c | 4 +-- qapi/qapi-clone-visitor.c | 16 +- qapi/qapi-visit-core.c | 6 ++-- softmmu/memory.c| 2 +- softmmu/vl.c| 2 +- target/arm/helper.c | 6 ++-- target/ppc/mmu-hash64.c | 2 +- tests/qtest/libqos/ahci.c | 6 ++-- tests/qtest/libqos/qgraph.c | 2 +- tests/unit/ptimer-test.c| 22 +++--- tests/unit/test-iov.c | 26 ui/clipboard.c | 2 +- scripts/checkpatch.pl | 5 38 files changed, 140 insertions(+), 155 deletions(-) -- 2.31.1
Re: [PATCH v6 3/5] block/nbd: refactor nbd_recv_coroutines_wake_all()
On Thu, Sep 02, 2021 at 01:38:03PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Split out nbd_recv_coroutine_wake_one(), as it will be used in > separate. > Rename the function and add a possibility to wake only first found > sleeping coroutine. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/nbd.c | 26 +- > 1 file changed, 17 insertions(+), 9 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2] iotests/check: move long options to double dash
On Fri, Sep 03, 2021 at 03:00:39PM +0300, Vladimir Sementsov-Ogievskiy wrote: > So, the change: > > -makecheck -> --makecheck > -gdb -> --gdb > -valgrind -> --valgrind > -misalign -> --misalign > -nocache -> --nocache > -qcow2 (and other formats) -> --qcow2 > -file (and other protocols) -> --file > > Motivation: > > 1. check scripts uses ArgumentParser to parse options, which supports >combining of short options. So using one dash for long options is a >bit ambiguous. > > 2. Several long options are already have double dash: > --dry-run, --color, --groups, --exclude-groups, --start-from > > 3. -misalign is used to add --misalign option (two dashes) to qemu-io > > 4. qemu-io and qemu-nbd has --nocache option (two dashes) > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 03/28] qapi: Replace g_memdup() by g_memdup2_qemu()
On 9/3/21 1:18 PM, Daniel P. Berrangé wrote: > On Fri, Sep 03, 2021 at 01:06:37PM +0200, Philippe Mathieu-Daudé wrote: >> Per >> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 >> >> The old API took the size of the memory to duplicate as a guint, >> whereas most memory functions take memory sizes as a gsize. This >> made it easy to accidentally pass a gsize to g_memdup(). For large >> values, that would lead to a silent truncation of the size from 64 >> to 32 bits, and result in a heap area being returned which is >> significantly smaller than what the caller expects. This can likely >> be exploited in various modules to cause a heap buffer overflow. >> >> Replace g_memdup() by the safer g_memdup2_qemu() wrapper. > > This and all following patches should directly use "g_memdup2" > rather than the wrapper which is supposed to remain "secret" > in the glib-compat.h header. Yep, got it, thanks for the quick review!
Re: [PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
On 9/3/21 1:16 PM, Daniel P. Berrangé wrote:
> On Fri, Sep 03, 2021 at 01:06:36PM +0200, Philippe Mathieu-Daudé wrote:
>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
>> (Fedora 34 provides GLib 2.68.1) we get:
>>
>> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
>> ...
>>
>> g_memdup() has been updated by g_memdup2() to fix eventual security
>> issues (size argument is 32-bit and could be truncated / wrapping).
>> GLib recommends to copy their static inline version of g_memdup2():
>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>>
>> Our glib-compat.h provides a comment explaining how to deal with
>> these deprecated declarations (see commit e71e8cc0355
>> "glib: enforce the minimum required version and warn about old APIs").
>>
>> Following this comment suggestion, implement the g_memdup2_qemu()
>> wrapper to g_memdup2(), and use the safer equivalent inlined when
>> we are using pre-2.68 GLib.
>>
>> Reported-by: Eric Blake
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> include/glib-compat.h | 36
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/include/glib-compat.h b/include/glib-compat.h
>> index 9e95c888f54..6577d9ab393 100644
>> --- a/include/glib-compat.h
>> +++ b/include/glib-compat.h
>> @@ -68,6 +68,42 @@
>> * without generating warnings.
>> */
>>
>> +/*
>> + * g_memdup2_qemu:
>> + * @mem: (nullable): the memory to copy.
>> + * @byte_size: the number of bytes to copy.
>> + *
>> + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
>> + * from @mem. If @mem is %NULL it returns %NULL.
>> + *
>> + * This replaces g_memdup(), which was prone to integer overflows when
>> + * converting the argument from a #gsize to a #guint.
>> + *
>> + * This static inline version is a backport of the new public API from
>> + * GLib 2.68, kept internal to GLib for backport to older stable releases.
>> + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
>> + *
>> + * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
>> + * or %NULL if @mem is %NULL.
>> + */
>> +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
>> +{
>> +#if GLIB_CHECK_VERSION(2, 68, 0)
>> +return g_memdup2(mem, byte_size);
>> +#else
>> +gpointer new_mem;
>> +
>> +if (mem && byte_size != 0) {
>> +new_mem = g_malloc(byte_size);
>> +memcpy(new_mem, mem, byte_size);
>> +} else {
>> +new_mem = NULL;
>> +}
>> +
>> +return new_mem;
>> +#endif
>> +}
>
> Close, but you missed the final piece of the puzzle
>
>#define g_memdup2(a) g_memdup2_qemu(a)
Doh :/
> Such that in all following patches you can use the normal "g_memdup2"
> API. This means when we later update min glib, we just delete the
> compat code here, and the callers don't need updates.
Painful rebase in perspective...
Re: [PATCH v6 1/5] block/nbd: nbd_channel_error() shutdown channel unconditionally
On Thu, Sep 02, 2021 at 01:38:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Don't rely on connection being totally broken in case of -EIO. More
> safe and correct just shutdown the channel anyway, as we change the
> state and going to reconnect.
If you don't mind me tweaking grammar, I propose:
Safer and more correct is to just shut down the channel anyway, since
we change the state and plan on reconnecting.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> Reviewed-by: Eric Blake
> ---
> block/nbd.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index f6ff1c4fb4..d88f4b954c 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -129,15 +129,16 @@ static bool nbd_client_connected(BDRVNBDState *s)
>
> static void nbd_channel_error(BDRVNBDState *s, int ret)
> {
> +if (nbd_client_connected(s)) {
> +qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +}
> +
> if (ret == -EIO) {
> if (nbd_client_connected(s)) {
> s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
> NBD_CLIENT_CONNECTING_NOWAIT;
> }
> } else {
> -if (nbd_client_connected(s)) {
> -qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> -}
> s->state = NBD_CLIENT_QUIT;
> }
> }
> --
> 2.29.2
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 10/19] block: introduce fleecing block driver
01.09.2021 14:44, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy writes:
Introduce a new driver, that works in pair with copy-before-write to
improve fleecing.
Without fleecing driver, old fleecing scheme looks as follows:
[guest]
|
|root
v
[copy-before-write] -> [temp.qcow2] <--- [nbd export]
| target |
|file |backing
v |
[active disk] <-+
With fleecing driver, new scheme is:
[guest]
|
|root
v
[copy-before-write] -> [fleecing] <--- [nbd export]
| target ||
|file ||file
v |v
[active disk]<--source--+ [temp.img]
Benefits of new scheme:
1. Access control: if remote client try to read data that not covered
by original dirty bitmap used on copy-before-write open, client gets
-EACCES.
2. Discard support: if remote client do DISCARD, this additionally to
discarding data in temp.img informs block-copy process to not copy
these clusters. Next read from discarded area will return -EACCES.
This is significant thing: when fleecing user reads data that was
not yet copied to temp.img, we can avoid copying it on further guest
write.
3. Synchronisation between client reads and block-copy write is more
efficient: it doesn't block intersecting block-copy write during
client read.
4. We don't rely on backing feature: active disk should not be backing
of temp image, so we avoid some permission-related difficulties and
temp image now is not required to support backing, it may be simple
raw image.
Note that now nobody calls fleecing_drv_activate(), so new driver is
actually unusable. It's a work for the following patch: support
fleecing block driver in copy-before-write filter driver.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
qapi/block-core.json | 17 ++-
block/fleecing.h | 16 +++
block/fleecing-drv.c | 260 +++
MAINTAINERS | 1 +
block/meson.build| 1 +
5 files changed, 294 insertions(+), 1 deletion(-)
create mode 100644 block/fleecing-drv.c
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c42d23752d..8a333136f5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2826,13 +2826,14 @@
# @blkreplay: Since 4.2
# @compress: Since 5.0
# @copy-before-write: Since 6.2
+# @fleecing: Since 6.2
#
# Since: 2.9
##
{ 'enum': 'BlockdevDriver',
'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
'cloop', 'compress', 'copy-before-write', 'copy-on-read', 'dmg',
-'file', 'ftp', 'ftps', 'gluster',
+'file', 'fleecing', 'ftp', 'ftps', 'gluster',
{'name': 'host_cdrom', 'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
{'name': 'host_device', 'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
'http', 'https', 'iscsi',
@@ -4077,6 +4078,19 @@
'base': 'BlockdevOptionsGenericFormat',
'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
+##
+# @BlockdevOptionsFleecing:
+#
+# Driver that works in pair with copy-before-write to make fleecing scheme.
This is really terse. Do we explain the driver's intended use anywhere?
Hmm. I can duplicate here the ASII art from commit message together with some
explanations.
I'd suggest s/to make fleecing scheme/to make a fleecing scheme/, except
it doesn't make much sense to me either way :)
+#
+# @source: source node of fleecing
We usually say "node name of ...".
OK
+#
+# Since: 6.2
+##
+{ 'struct': 'BlockdevOptionsFleecing',
+ 'base': 'BlockdevOptionsGenericFormat',
+ 'data': { 'source': 'str' } }
+
##
# @BlockdevOptions:
#
@@ -4133,6 +4147,7 @@
'copy-on-read':'BlockdevOptionsCor',
'dmg':'BlockdevOptionsGenericFormat',
'file': 'BlockdevOptionsFile',
+ 'fleecing': 'BlockdevOptionsFleecing',
'ftp':'BlockdevOptionsCurlFtp',
'ftps': 'BlockdevOptionsCurlFtps',
'gluster':'BlockdevOptionsGluster',
[...]
--
Best regards,
Vladimir
Re: [PATCH v2] fdc: check null block pointer before r/w data transfer
On Tue, Sep 22, 2020 at 06:42:22PM +0800, Li Qiang wrote: > P J P 于2020年9月22日周二 下午5:29写道： > > > > From: Prasad J Pandit > > > > While transferring data via fdctrl_read/write_data() routines, > > check that current drive does not have a null block pointer. > > Avoid null pointer dereference. > > > > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1 > > ==1658854==Hint: address points to the zero page. > > #0 blk_inc_in_flight block/block-backend.c:1327 > > #1 blk_prw block/block-backend.c:1299 > > #2 blk_pwrite block/block-backend.c:1464 > > #3 fdctrl_write_data hw/block/fdc.c:2418 > > #4 fdctrl_write hw/block/fdc.c:962 > > #5 portio_write ioport.c:205 > > #6 memory_region_write_accessor memory.c:483 > > #7 access_with_adjusted_size memory.c:544 > > #8 memory_region_dispatch_write memory.c:1476 > > > > Reported-by: Ruhr-University > > Signed-off-by: Prasad J Pandit > > Reviewed-by: Li Qiang Did this one just felt through the cracks, or was it not further considered? Regards, Salvatore
Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
Jonah Palmer writes:
> No problem! Comments below:
>
> On 8/23/21 9:27 AM, Markus Armbruster wrote:
[...]
>> Hmm... how is this enum used? In this patch:
>>
>> VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
>> {
>> VirtioInfoList *list = NULL;
>> VirtioInfoList *node;
>> VirtIODevice *vdev;
>>
>> QTAILQ_FOREACH(vdev, _list, next) {
>> DeviceState *dev = DEVICE(vdev);
>> node = g_new0(VirtioInfoList, 1);
>> node->value = g_new(VirtioInfo, 1);
>> node->value->path = g_strdup(dev->canonical_path);
>> --->node->value->type = qapi_enum_parse(_lookup,
>> vdev->name,
>> --->VIRTIO_TYPE_UNKNOWN, NULL);
>> QAPI_LIST_PREPEND(list, node->value);
>> }
>>
>> return list;
>> }
>>
>> This maps VirtioDevice member @name (a string) to an enum value.
>>
>> As far as I can tell, this member is set in virtio_init() only, to its
>> argument. All callers pass string literals. They also pass a numeric
>> device_id, and the two seem to match, e.g. "virtio-blk" and
>> VIRTIO_ID_BLOCK.
>>
>> Thus, the pairs (numeric device ID, string device ID) already exist in
>> the code, but not in a way that lets you map between them. To get that,
>> you *duplicate* the pairs in QAPI.
>>
>> Having two copies means we get to keep them consistent. Can we avoid
>> that?
>>
>> The enum has a special member 'unknown' that gets used when @name does
>> not parse as enum member, i.e. we failed at keeping the copies
>> consistent. I'm afraid this sweeps a programming error under the rug.
>>
>> The enum has a bunch of dummy members like 'unknown-14' to make QAPI
>> generate numeric enum values match the device IDs. Error prone. Can't
>> see offhand why we need them to match.
>
> Sure, I don't mind doing this instead. Just as an FYI, from the previous
> RFC series (RFC v5 1/6), David recommended here that I create a list of
> all the types and in the same order as
> include/standard-headers/linux/virtio_ids.h.
>
> The benefit from this was that we never have to convert between the QAPI
> number
> and the header number.
Yes, but it comes with the disadvantages I listed above.
> Let me know if this is still something you'd like me to do!
I think it's simpler overall, especially if you can pick option
"2. Without QAPI enum VirtioType" below.
I could be wrong. Suggest to try it out to see what you like better.
>>
>> What about the following. Define a map from numeric device ID to
>> string, like so
>>
>> const char *virtio_device_name[] = {
>> [VIRTIO_ID_NET] = "virtio-net",
>> [VIRTIO_ID_BLOCK] = "virtio-blk",
>> ...
>> }
>
> Sorry if this is obvious, but where should I define this mapping?
> virtio.c or virtio.h?
Variable definitions normally live in .c.
> Jonah
>
>> This lets you
>>
>> * map device ID to string by subscripting virtio_device_name[].
>>Guarding with assert(device_id < G_N_ELEMENTS(virtio_device_name)) may
>>be advisable.
>>
>> * map string to device ID by searching virtio_device_name[]. May want a
>>function for that,
>>
>> Now you can have virtio_init() map parameter @device_id to string, and
>> drop parameter @name.
>>
>> Then you have two options:
>>
>> 1. With QAPI enum VirtioType
>>
>> Define it without the special and the dummy members.
>>
>> To map from string to QAPI enum, use qapi_enum_parse().
>>
>> To map from QAPI enum to string, use VirtioType_str().
>>
>> To map from QAPI enum to device ID, map through string.
>>
>> 2. Without QAPI enum VirtioType
>>
>> Simply use uint16_t device_id, just like struct VirtioDevice.
>>
>> The choice between 1. and 2. depends on whether you actually need
>> additional functionality provided by QAPI, such as types being visible
>> in query-qmp-schema.
>>
>> [...]
>>
Re: [PATCH v2 10/19] block: introduce fleecing block driver
Vladimir Sementsov-Ogievskiy writes:
> Introduce a new driver, that works in pair with copy-before-write to
> improve fleecing.
>
> Without fleecing driver, old fleecing scheme looks as follows:
>
> [guest]
> |
> |root
> v
> [copy-before-write] -> [temp.qcow2] <--- [nbd export]
> | target |
> |file |backing
> v |
> [active disk] <-+
>
> With fleecing driver, new scheme is:
>
> [guest]
> |
> |root
> v
> [copy-before-write] -> [fleecing] <--- [nbd export]
> | target ||
> |file ||file
> v |v
> [active disk]<--source--+ [temp.img]
>
> Benefits of new scheme:
>
> 1. Access control: if remote client try to read data that not covered
>by original dirty bitmap used on copy-before-write open, client gets
>-EACCES.
>
> 2. Discard support: if remote client do DISCARD, this additionally to
>discarding data in temp.img informs block-copy process to not copy
>these clusters. Next read from discarded area will return -EACCES.
>This is significant thing: when fleecing user reads data that was
>not yet copied to temp.img, we can avoid copying it on further guest
>write.
>
> 3. Synchronisation between client reads and block-copy write is more
>efficient: it doesn't block intersecting block-copy write during
>client read.
>
> 4. We don't rely on backing feature: active disk should not be backing
>of temp image, so we avoid some permission-related difficulties and
>temp image now is not required to support backing, it may be simple
>raw image.
>
> Note that now nobody calls fleecing_drv_activate(), so new driver is
> actually unusable. It's a work for the following patch: support
> fleecing block driver in copy-before-write filter driver.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> qapi/block-core.json | 17 ++-
> block/fleecing.h | 16 +++
> block/fleecing-drv.c | 260 +++
> MAINTAINERS | 1 +
> block/meson.build| 1 +
> 5 files changed, 294 insertions(+), 1 deletion(-)
> create mode 100644 block/fleecing-drv.c
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c42d23752d..8a333136f5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2826,13 +2826,14 @@
> # @blkreplay: Since 4.2
> # @compress: Since 5.0
> # @copy-before-write: Since 6.2
> +# @fleecing: Since 6.2
> #
> # Since: 2.9
> ##
> { 'enum': 'BlockdevDriver',
>'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
> 'cloop', 'compress', 'copy-before-write', 'copy-on-read', 'dmg',
> -'file', 'ftp', 'ftps', 'gluster',
> +'file', 'fleecing', 'ftp', 'ftps', 'gluster',
> {'name': 'host_cdrom', 'if': 'defined(HAVE_HOST_BLOCK_DEVICE)' },
> {'name': 'host_device', 'if': 'defined(HAVE_HOST_BLOCK_DEVICE)'
> },
> 'http', 'https', 'iscsi',
> @@ -4077,6 +4078,19 @@
>'base': 'BlockdevOptionsGenericFormat',
>'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
>
> +##
> +# @BlockdevOptionsFleecing:
> +#
> +# Driver that works in pair with copy-before-write to make fleecing scheme.
This is really terse. Do we explain the driver's intended use anywhere?
I'd suggest s/to make fleecing scheme/to make a fleecing scheme/, except
it doesn't make much sense to me either way :)
> +#
> +# @source: source node of fleecing
We usually say "node name of ...".
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'BlockdevOptionsFleecing',
> + 'base': 'BlockdevOptionsGenericFormat',
> + 'data': { 'source': 'str' } }
> +
> ##
> # @BlockdevOptions:
> #
> @@ -4133,6 +4147,7 @@
>'copy-on-read':'BlockdevOptionsCor',
>'dmg':'BlockdevOptionsGenericFormat',
>'file': 'BlockdevOptionsFile',
> + 'fleecing': 'BlockdevOptionsFleecing',
>'ftp':'BlockdevOptionsCurlFtp',
>'ftps': 'BlockdevOptionsCurlFtps',
>'gluster':'BlockdevOptionsGluster',
[...]
Re: [PATCH v2 3/3] qapi: deprecate drive-backup
Vladimir Sementsov-Ogievskiy writes: > 24.05.2021 21:37, John Snow wrote: >> On 5/24/21 10:06 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 15.05.2021 01:38, John Snow wrote: On 5/6/21 5:57 AM, Kashyap Chamarthy wrote: > TODO: We also need to deprecate drive-backup transaction action.. > But union members in QAPI doesn't support 'deprecated' feature. I tried > to dig a bit, but failed :/ Markus, could you please help with it? At > least by advice? Oho, I see. OK, I'm not Markus, but I've been getting into lots of trouble in the QAPI generator lately, so let me see if I can help get you started... https://gitlab.com/jsnow/qemu/-/commits/hack-deprecate-union-branches/ Here's a quick hack that might expose that feature. I suppose we can discuss this with Markus and turn these into real patches if that's the direction we wanna head. >>> >>> Hi! Markus is silent.. Maybe, you'll send patches ? ) >>> >>> >> He just went on PTO for 2 weeks :') >> It's going to have to wait, I'm afraid ... >> > > Hi! > > Any plans or updates? John, may be you just send your patches? Yes, please! I intend to resume working on feature flags next week.
Re: [PATCH v2 18/19] qapi: backup: add immutable-source parameter
Vladimir Sementsov-Ogievskiy writes: > We are on the way to implement internal-backup with fleecing scheme, > which includes backup job copying from fleecing block driver node > (which is target of copy-before-write filter) to final target of > backup. This job doesn't need own filter, as fleecing block driver node > is a kind of snapshot, it's immutable from reader point of view. > > Let's add a parameter for backup to not insert filter but instead > unshare writes on source. This way backup job becomes a simple copying > process. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > qapi/block-core.json | 12 +++- > include/block/block_int.h | 1 + > block/backup.c| 61 +++ > block/replication.c | 2 +- > blockdev.c| 1 + > 5 files changed, 70 insertions(+), 7 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 8a333136f5..995ca16a5e 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1391,6 +1391,15 @@ > #above node specified by @drive. If this option is not > given, > #a node name is autogenerated. (Since: 4.2) > # > +# @immutable-source: If true, assume source is immutable and don't insert > filter Suggest comma after immutable. > +#as no copy-before-write operations are needed. It will > +#fail if there are existing writers on source node, as > well, > +#any attempt to add writer to source node during backup > will > +#fail. @filter-node-name must not be set. Suggest to split the sentence like "... fail if there are existing writers on source node. Any attempt ... will also fail." > +#If false, insert copy-before-write filter above source > node > +#(see also @filter-node-name parameter). > +#Default is false. (Since 6.2) > +# > # @x-perf: Performance options. (Since 6.0) > # > # Note: @on-source-error and @on-target-error only affect background > @@ -1407,7 +1416,8 @@ > '*on-source-error': 'BlockdevOnError', > '*on-target-error': 'BlockdevOnError', > '*auto-finalize': 'bool', '*auto-dismiss': 'bool', > -'*filter-node-name': 'str', '*x-perf': 'BackupPerf' } } > +'*filter-node-name': 'str', '*immutable-source': 'bool', > +'*x-perf': 'BackupPerf' } } > > ## > # @DriveBackup: [...]
Re: [PATCH] spec: Relax NBD_OPT_LIST_META_CONTEXTS
Ping. On Mon, Aug 16, 2021 at 01:40:59PM -0500, Eric Blake wrote: > Using OPT_SET_META_CONTEXTS is stateful (it is documented to wipe out > any previously-requested contexts, and we just tightened the spec to > clarify that starting TLS also wipes it out). But > OPT_LIST_META_CONTEXTS is not stateful; and in fact, with a > SELECTIVETLS server, it can be handy to list the meta contexts > available on an unencrypted export, then enable encryption, and then > further list what contexts are available on encrypted exports (as the > server is permitted to let them differ). Thus, while a wise client > will renegotiate structured replies after the starttls, there's no > reason to forbid a server from answering a client that uses > list_meta_contexts prior to encryption without also requesting > structured replies. I originally wrote this patch prior to the point where we decided that OPT_STARTTLS should also wipe the effects of OPT_STRUCTURED_REPLY; given that change in the meantime, I'm tweaking that last sentence: Although such a client must negotiate structured replies after starttls if it is going to actually connect to an export, this change permits the client to shorten the handshake by two commands if it is only being used to list available exports and their meta contexts. > --- > doc/proto.md | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/doc/proto.md b/doc/proto.md > index 9dd59da..1586d7d 100644 > --- a/doc/proto.md > +++ b/doc/proto.md > @@ -1325,9 +1325,9 @@ of the newstyle negotiation. > Return a list of `NBD_REP_META_CONTEXT` replies, one per context, > followed by an `NBD_REP_ACK` or an error. > > -This option MUST NOT be requested unless structured replies have > +This option SHOULD NOT be requested unless structured replies have > been negotiated first. If a client attempts to do so, a server > -SHOULD send `NBD_REP_ERR_INVALID`. > +MAY send `NBD_REP_ERR_INVALID`. > > Data: > - 32 bits, length of export name. > -- > 2.31.1 > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
03.09.2021 14:56, Daniel P. Berrangé wrote:
On Fri, Sep 03, 2021 at 02:51:21PM +0300, Vladimir Sementsov-Ogievskiy wrote:
That was most probably already discussed, so sorry my question:
Why we can't just do
#if ! GLIB_CHECK_VERSION(2, 68, 0)
static inline gpointer g_memdup2(gconstpointer mem, gsize byte_size)
{
gpointer new_mem;
if (mem && byte_size != 0) {
new_mem = g_malloc(byte_size);
memcpy(new_mem, mem, byte_size);
} else {
new_mem = NULL;
}
return new_mem;
}
#endif
?
This doesn't play with GLIB_VERSION_MAX_ALLOWED - any use of
g_memdup2 will trigger compile warnings since we're using an
API that only exists in a glib version newer than our declared
baseline.
The inline wrapper + macro is a trick that lets us backport
new features, while avoiding the compile warnings.
Ah right, with macro, compiler doesn't see g_memdup2() invocations in code.
Thanks!
This is documented in the include/glib-compat.h file that Philippe
is modifying.
Regards,
Daniel
--
Best regards,
Vladimir
[PATCH v2] iotests/check: move long options to double dash
So, the change:
-makecheck -> --makecheck
-gdb -> --gdb
-valgrind -> --valgrind
-misalign -> --misalign
-nocache -> --nocache
-qcow2 (and other formats) -> --qcow2
-file (and other protocols) -> --file
Motivation:
1. check scripts uses ArgumentParser to parse options, which supports
combining of short options. So using one dash for long options is a
bit ambiguous.
2. Several long options are already have double dash:
--dry-run, --color, --groups, --exclude-groups, --start-from
3. -misalign is used to add --misalign option (two dashes) to qemu-io
4. qemu-io and qemu-nbd has --nocache option (two dashes)
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
v2: cover more things, update also format and protocol options
docs/devel/testing.rst | 12 ++--
.gitlab-ci.d/buildtest.yml | 4 ++--
tests/check-block.sh | 2 +-
tests/qemu-iotests/README| 7 ---
tests/qemu-iotests/check | 14 +++---
tests/qemu-iotests/common.rc | 4 ++--
6 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 4a0abbf23d..907b18a600 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -153,16 +153,16 @@ with arguments:
.. code::
# test with qcow2 format
- ./check -qcow2
+ ./check --qcow2
# or test a different protocol
- ./check -nbd
+ ./check --nbd
It's also possible to list test numbers explicitly:
.. code::
# run selected cases with qcow2 format
- ./check -qcow2 001 030 153
+ ./check --qcow2 001 030 153
Cache mode can be selected with the "-c" option, which may help reveal bugs
that are specific to certain cache mode.
@@ -229,7 +229,7 @@ Debugging a test case
The following options to the ``check`` script can be useful when debugging
a failing test:
-* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a
+* ``--gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a
connection from a gdb client. The options given to ``gdbserver`` (e.g. the
address on which to listen for connections) are taken from the
``$GDB_OPTIONS``
environment variable. By default (if ``$GDB_OPTIONS`` is empty), it listens
on
@@ -237,10 +237,10 @@ a failing test:
It is possible to connect to it for example with
``gdb -iex "target remote $addr"``, where ``$addr`` is the address
``gdbserver`` listens on.
- If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
+ If the ``--gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
regardless of whether it is set or not.
-* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects
+* ``--valgrind`` attaches a valgrind instance to QEMU. If it detects
warnings, it will print and save the log in
``$TEST_DIR/.valgrind``.
The final command line will be ``valgrind --log-file=$TEST_DIR/
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index e74998efb9..139c27554d 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -303,10 +303,10 @@ build-tcg-disabled:
- make check-unit
- make check-qapi-schema
- cd tests/qemu-iotests/
-- ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048
+- ./check --raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048
052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163
170 171 183 184 192 194 208 221 226 227 236 253 277 image-fleecing
-- ./check -qcow2 028 051 056 057 058 065 068 082 085 091 095 096 102 122
+- ./check --qcow2 028 051 056 057 058 065 068 082 085 091 095 096 102 122
124 132 139 142 144 145 151 152 155 157 165 194 196 200 202
208 209 216 218 227 234 246 247 248 250 254 255 257 258
260 261 262 263 264 270 272 273 277 279 image-fleecing
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f86cb863de..cff1263c0b 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -77,7 +77,7 @@ export PYTHONUTF8=1
ret=0
for fmt in $format_list ; do
-${PYTHON} ./check -makecheck -$fmt $group || ret=1
+${PYTHON} ./check --makecheck --$fmt $group || ret=1
done
exit $ret
diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README
index 6079b401ae..8e1f3e19c3 100644
--- a/tests/qemu-iotests/README
+++ b/tests/qemu-iotests/README
@@ -10,9 +10,10 @@ but no actual block drivers like ide, scsi or virtio.
* Usage
-Just run ./check to run all tests for the raw image format, or ./check
--qcow2 to test the qcow2 image format. The output of ./check -h explains
-additional options to test further image formats or I/O methods.
+Just run ./check to run all tests for the raw image format,
+or ./check --qcow2 to test the qcow2 image format. The output of
+./check -h explains additional options to test further image formats
+or I/O methods.
* Feedback and patches
diff --git a/tests/qemu-iotests/check
Re: [PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
03.09.2021 14:16, Daniel P. Berrangé wrote:
On Fri, Sep 03, 2021 at 01:06:36PM +0200, Philippe Mathieu-Daudé wrote:
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..6577d9ab393 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,42 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
Close, but you missed the final piece of the puzzle
#define g_memdup2(a) g_memdup2_qemu(a)
Such that in all following patches you can use the normal "g_memdup2"
API. This means when we later update min glib, we just delete the
compat code here, and the callers don't need updates.
That was most probably already discussed, so sorry my question:
Why we can't just do
#if ! GLIB_CHECK_VERSION(2, 68, 0)
static inline gpointer g_memdup2(gconstpointer mem, gsize byte_size)
{
gpointer new_mem;
if (mem && byte_size != 0) {
new_mem = g_malloc(byte_size);
memcpy(new_mem, mem, byte_size);
} else {
new_mem = NULL;
}
return new_mem;
}
#endif
?
--
Best regards,
Vladimir
Re: [PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Sep 03, 2021 at 02:51:21PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> That was most probably already discussed, so sorry my question:
>
> Why we can't just do
>
> #if ! GLIB_CHECK_VERSION(2, 68, 0)
> static inline gpointer g_memdup2(gconstpointer mem, gsize byte_size)
> {
> gpointer new_mem;
>
> if (mem && byte_size != 0) {
> new_mem = g_malloc(byte_size);
> memcpy(new_mem, mem, byte_size);
> } else {
> new_mem = NULL;
> }
>
> return new_mem;
> }
> #endif
>
> ?
This doesn't play with GLIB_VERSION_MAX_ALLOWED - any use of
g_memdup2 will trigger compile warnings since we're using an
API that only exists in a glib version newer than our declared
baseline.
The inline wrapper + macro is a trick that lets us backport
new features, while avoiding the compile warnings.
This is documented in the include/glib-compat.h file that Philippe
is modifying.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH v2] include/block.h: remove outdated comment
There are a couple of errors in bdrv_drained_begin header comment: - block_job_pause does not exist anymore, it has been replaced with job_pause in b15de82867 - job_pause is automatically invoked as a .drained_begin callback (child_job_drained_begin) by the child_job BdrvChildClass struct in blockjob.c. So no additional pause should be required. Signed-off-by: Emanuele Giuseppe Esposito --- v2: + add "block jobs" to the external request sources include/block/block.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index 3477290f9a..de40758e71 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -749,9 +749,7 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive, * bdrv_drained_begin: * * Begin a quiesced section for exclusive access to the BDS, by disabling - * external request sources including NBD server and device model. Note that - * this doesn't block timers or coroutines from submitting more requests, which - * means block_job_pause is still necessary. + * external request sources including NBD server, block jobs, and device model. * * This function can be recursive. */ -- 2.31.1
Re: [PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
On Fri, Sep 03, 2021 at 01:06:36PM +0200, Philippe Mathieu-Daudé wrote:
> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
> (Fedora 34 provides GLib 2.68.1) we get:
>
> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
> ...
>
> g_memdup() has been updated by g_memdup2() to fix eventual security
> issues (size argument is 32-bit and could be truncated / wrapping).
> GLib recommends to copy their static inline version of g_memdup2():
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> Our glib-compat.h provides a comment explaining how to deal with
> these deprecated declarations (see commit e71e8cc0355
> "glib: enforce the minimum required version and warn about old APIs").
>
> Following this comment suggestion, implement the g_memdup2_qemu()
> wrapper to g_memdup2(), and use the safer equivalent inlined when
> we are using pre-2.68 GLib.
>
> Reported-by: Eric Blake
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/glib-compat.h | 36
> 1 file changed, 36 insertions(+)
>
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 9e95c888f54..6577d9ab393 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -68,6 +68,42 @@
> * without generating warnings.
> */
>
> +/*
> + * g_memdup2_qemu:
> + * @mem: (nullable): the memory to copy.
> + * @byte_size: the number of bytes to copy.
> + *
> + * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
> + * from @mem. If @mem is %NULL it returns %NULL.
> + *
> + * This replaces g_memdup(), which was prone to integer overflows when
> + * converting the argument from a #gsize to a #guint.
> + *
> + * This static inline version is a backport of the new public API from
> + * GLib 2.68, kept internal to GLib for backport to older stable releases.
> + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
> + *
> + * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
> + * or %NULL if @mem is %NULL.
> + */
> +static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
> +{
> +#if GLIB_CHECK_VERSION(2, 68, 0)
> +return g_memdup2(mem, byte_size);
> +#else
> +gpointer new_mem;
> +
> +if (mem && byte_size != 0) {
> +new_mem = g_malloc(byte_size);
> +memcpy(new_mem, mem, byte_size);
> +} else {
> +new_mem = NULL;
> +}
> +
> +return new_mem;
> +#endif
> +}
Close, but you missed the final piece of the puzzle
#define g_memdup2(a) g_memdup2_qemu(a)
Such that in all following patches you can use the normal "g_memdup2"
API. This means when we later update min glib, we just delete the
compat code here, and the callers don't need updates.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH 13/28] hw/net/eepro100: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/net/eepro100.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 16e95ef9cc9..ed2bc54c052 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -1872,7 +1872,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp) qemu_register_reset(nic_reset, s); -s->vmstate = g_memdup(_eepro100, sizeof(vmstate_eepro100)); +s->vmstate = g_memdup2_qemu(_eepro100, sizeof(vmstate_eepro100)); s->vmstate->name = qemu_get_queue(s->nic)->model; vmstate_register(VMSTATE_IF(_dev->qdev), VMSTATE_INSTANCE_ID_ANY, s->vmstate, s); -- 2.31.1
Re: [PATCH 03/28] qapi: Replace g_memdup() by g_memdup2_qemu()
On Fri, Sep 03, 2021 at 01:06:37PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2_qemu() wrapper. This and all following patches should directly use "g_memdup2" rather than the wrapper which is supposed to remain "secret" in the glib-compat.h header. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH 15/28] hw/scsi/mptsas: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/scsi/mptsas.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index db3219e7d20..d05735d3e11 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -449,7 +449,8 @@ static void mptsas_process_scsi_task_mgmt(MPTSASState *s,
MPIMsgSCSITaskMgmt *re
} else {
MPTSASCancelNotifier *notifier;
-reply_async = g_memdup(,
sizeof(MPIMsgSCSITaskMgmtReply));
+reply_async = g_memdup2_qemu(,
+ sizeof(MPIMsgSCSITaskMgmtReply));
reply_async->IOCLogInfo = INT_MAX;
count = 1;
@@ -476,7 +477,7 @@ static void mptsas_process_scsi_task_mgmt(MPTSASState *s,
MPIMsgSCSITaskMgmt *re
goto out;
}
-reply_async = g_memdup(, sizeof(MPIMsgSCSITaskMgmtReply));
+reply_async = g_memdup2_qemu(, sizeof(MPIMsgSCSITaskMgmtReply));
reply_async->IOCLogInfo = INT_MAX;
count = 0;
--
2.31.1
[PATCH 28/28] checkpatch: Do not allow deprecated g_memdup()
g_memdup() is insecure and as been deprecated in GLib 2.68.
QEMU provides the safely equivalent g_memdup2_qemu() wrapper.
Do not allow more g_memdup() calls in the repository, provide
a hint to use g_memdup2_qemu().
Signed-off-by: Philippe Mathieu-Daudé
---
scripts/checkpatch.pl | 5 +
1 file changed, 5 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb8eff233e0..4ce9d753492 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2850,6 +2850,11 @@ sub process {
WARN("consider using g_path_get_$1() in preference to
g_strdup($1())\n" . $herecurr);
}
+# enforce g_memdup2_qemu() over g_memdup()
+ if ($line =~ /\bg_memdup\s*\(/) {
+ ERROR("use g_memdup2_qemu() instead of unsafe
g_memdup()\n" . $herecurr);
+ }
+
# recommend qemu_strto* over strto* for numeric conversions
if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
ERROR("consider using qemu_$1 in preference to $1\n" .
$herecurr);
--
2.31.1
[PATCH 26/28] target/ppc: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- target/ppc/mmu-hash64.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c index 19832c4b46f..2ee6025a406 100644 --- a/target/ppc/mmu-hash64.c +++ b/target/ppc/mmu-hash64.c @@ -1122,7 +1122,8 @@ void ppc_hash64_init(PowerPCCPU *cpu) return; } -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); +cpu->hash64_opts = g_memdup2_qemu(pcc->hash64_opts, + sizeof(*cpu->hash64_opts)); } void ppc_hash64_finalize(PowerPCCPU *cpu) -- 2.31.1
[PATCH 27/28] contrib: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
contrib/plugins/lockstep.c | 2 +-
contrib/rdmacm-mux/main.c | 10 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c
index 7fd35eb6692..119a8054b3f 100644
--- a/contrib/plugins/lockstep.c
+++ b/contrib/plugins/lockstep.c
@@ -130,7 +130,7 @@ static void report_divergance(ExecState *us, ExecState
*them)
}
}
divergence_log = g_slist_prepend(divergence_log,
- g_memdup(, sizeof(divrec)));
+ g_memdup2_qemu(, sizeof(divrec)));
/* Output short log entry of going out of sync... */
if (verbose || divrec.distance == 1 || diverged) {
diff --git a/contrib/rdmacm-mux/main.c b/contrib/rdmacm-mux/main.c
index 771ca01e03f..d447d50f538 100644
--- a/contrib/rdmacm-mux/main.c
+++ b/contrib/rdmacm-mux/main.c
@@ -227,8 +227,8 @@ static RdmaCmMuxErrCode add_fd_ifid_pair(int fd, __be64
gid_ifid)
RDMACM_MUX_ERR_CODE_EACCES;
}
-g_hash_table_insert(server.umad_agent.gid2fd, g_memdup(_ifid,
-sizeof(gid_ifid)), g_memdup(, sizeof(fd)));
+g_hash_table_insert(server.umad_agent.gid2fd, g_memdup2_qemu(_ifid,
+sizeof(gid_ifid)), g_memdup2_qemu(, sizeof(fd)));
pthread_rwlock_unlock();
@@ -250,7 +250,7 @@ static RdmaCmMuxErrCode delete_fd_ifid_pair(int fd, __be64
gid_ifid)
return RDMACM_MUX_ERR_CODE_ENOTFOUND;
}
-g_hash_table_remove(server.umad_agent.gid2fd, g_memdup(_ifid,
+g_hash_table_remove(server.umad_agent.gid2fd, g_memdup2_qemu(_ifid,
sizeof(gid_ifid)));
pthread_rwlock_unlock();
@@ -267,8 +267,8 @@ static void hash_tbl_save_fd_comm_id_pair(int fd, uint32_t
comm_id,
pthread_rwlock_wrlock();
g_hash_table_insert(server.umad_agent.commid2fd,
-g_memdup(_id, sizeof(comm_id)),
-g_memdup(, sizeof(fde)));
+g_memdup2_qemu(_id, sizeof(comm_id)),
+g_memdup2_qemu(, sizeof(fde)));
pthread_rwlock_unlock();
}
--
2.31.1
[PATCH 23/28] tests/unit: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/unit/ptimer-test.c | 22 +++---
tests/unit/test-iov.c| 26 +-
2 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/tests/unit/ptimer-test.c b/tests/unit/ptimer-test.c
index 9176b96c1ce..23efeb04a57 100644
--- a/tests/unit/ptimer-test.c
+++ b/tests/unit/ptimer-test.c
@@ -798,64 +798,64 @@ static void add_ptimer_tests(uint8_t policy)
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/set_count policy=%s", policy_name),
-g_memdup(, 1), check_set_count, g_free);
+g_memdup2_qemu(, 1), check_set_count, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/set_limit policy=%s", policy_name),
-g_memdup(, 1), check_set_limit, g_free);
+g_memdup2_qemu(, 1), check_set_limit, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/oneshot policy=%s", policy_name),
-g_memdup(, 1), check_oneshot, g_free);
+g_memdup2_qemu(, 1), check_oneshot, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/periodic policy=%s", policy_name),
-g_memdup(, 1), check_periodic, g_free);
+g_memdup2_qemu(, 1), check_periodic, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_mode_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_mode_change, g_free);
+g_memdup2_qemu(, 1), check_on_the_fly_mode_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_period_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_period_change, g_free);
+g_memdup2_qemu(, 1), check_on_the_fly_period_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/on_the_fly_freq_change policy=%s",
policy_name),
-g_memdup(, 1), check_on_the_fly_freq_change, g_free);
+g_memdup2_qemu(, 1), check_on_the_fly_freq_change, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/run_with_period_0 policy=%s",
policy_name),
-g_memdup(, 1), check_run_with_period_0, g_free);
+g_memdup2_qemu(, 1), check_run_with_period_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/run_with_delta_0 policy=%s",
policy_name),
-g_memdup(, 1), check_run_with_delta_0, g_free);
+g_memdup2_qemu(, 1), check_run_with_delta_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/periodic_with_load_0 policy=%s",
policy_name),
-g_memdup(, 1), check_periodic_with_load_0, g_free);
+g_memdup2_qemu(, 1), check_periodic_with_load_0, g_free);
g_free(tmp);
g_test_add_data_func_full(
tmp = g_strdup_printf("/ptimer/oneshot_with_load_0 policy=%s",
policy_name),
-g_memdup(, 1), check_oneshot_with_load_0, g_free);
+g_memdup2_qemu(, 1), check_oneshot_with_load_0, g_free);
g_free(tmp);
}
diff --git a/tests/unit/test-iov.c b/tests/unit/test-iov.c
index 5371066fb6a..19ae24adb70 100644
--- a/tests/unit/test-iov.c
+++ b/tests/unit/test-iov.c
@@ -173,7 +173,7 @@ static void test_io(void)
}
iov_from_buf(iov, niov, 0, buf, sz);
-siov = g_memdup(iov, sizeof(*iov) * niov);
+siov = g_memdup2_qemu(iov, sizeof(*iov) * niov);
if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) {
perror("socketpair");
@@ -350,7 +350,7 @@ static void test_discard_front_undo(void)
/* Discard zero bytes */
iov_random(, _cnt);
-iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+iov_orig = g_memdup2_qemu(iov, sizeof(iov[0]) * iov_cnt);
iov_tmp = iov;
iov_cnt_tmp = iov_cnt;
iov_discard_front_undoable(_tmp, _cnt_tmp, 0, );
@@ -361,7 +361,7 @@ static void test_discard_front_undo(void)
/* Discard more bytes than vector size */
iov_random(, _cnt);
-
[PATCH 25/28] target/arm: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
target/arm/helper.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a7ae78146d4..f3aeff399b9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6242,8 +6242,9 @@ static void define_arm_vh_e2h_redirects_aliases(ARMCPU
*cpu)
/* Create alias before redirection so we dup the right data. */
if (a->new_key) {
-ARMCPRegInfo *new_reg = g_memdup(src_reg, sizeof(ARMCPRegInfo));
-uint32_t *new_key = g_memdup(>new_key, sizeof(uint32_t));
+ARMCPRegInfo *new_reg = g_memdup2_qemu(src_reg,
+ sizeof(ARMCPRegInfo));
+uint32_t *new_key = g_memdup2_qemu(>new_key, sizeof(uint32_t));
bool ok;
new_reg->name = a->new_name;
@@ -8818,7 +8819,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const
ARMCPRegInfo *r,
* add a single reginfo struct to the hash table.
*/
uint32_t *key = g_new(uint32_t, 1);
-ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
+ARMCPRegInfo *r2 = g_memdup2_qemu(r, sizeof(ARMCPRegInfo));
int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
--
2.31.1
[PATCH 24/28] tests/qtest: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/libqos/ahci.c | 6 +++---
tests/qtest/libqos/qgraph.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index fba3e7a954e..8ef1bda7c1c 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -639,8 +639,8 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
AHCIOpts *opts;
uint64_t buffer_in;
-opts = g_memdup((opts_in == NULL ? _opts : opts_in),
-sizeof(AHCIOpts));
+opts = g_memdup2_qemu((opts_in == NULL ? _opts : opts_in),
+ sizeof(AHCIOpts));
buffer_in = opts->buffer;
@@ -860,7 +860,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
g_assert(!props->ncq || props->lba48);
/* Defaults and book-keeping */
-cmd->props = g_memdup(props, sizeof(AHCICommandProp));
+cmd->props = g_memdup2_qemu(props, sizeof(AHCICommandProp));
cmd->name = command_name;
cmd->xbytes = props->size;
cmd->prd_size = 4096;
diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index d1dc4919305..c2e7719bed9 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -93,7 +93,7 @@ static void add_edge(const char *source, const char *dest,
edge->type = type;
edge->dest = g_strdup(dest);
edge->edge_name = g_strdup(opts->edge_name ?: dest);
-edge->arg = g_memdup(opts->arg, opts->size_arg);
+edge->arg = g_memdup2_qemu(opts->arg, opts->size_arg);
edge->before_cmd_line =
opts->before_cmd_line ? g_strconcat(" ", opts->before_cmd_line, NULL)
: NULL;
--
2.31.1
[RFC PATCH 22/28] linux-user: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
do_open_by_handle_at() doesn't check:
size + sizeof(struct file_handle) < 4GiB
---
linux-user/syscall.c | 2 +-
linux-user/uaccess.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ccd3892b2df..e127927f0b9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7665,7 +7665,7 @@ static abi_long do_open_by_handle_at(abi_long mount_fd,
abi_long handle,
return -TARGET_EFAULT;
}
-fh = g_memdup(target_fh, total_size);
+fh = g_memdup2_qemu(target_fh, total_size);
fh->handle_bytes = size;
fh->handle_type = tswap32(target_fh->handle_type);
diff --git a/linux-user/uaccess.c b/linux-user/uaccess.c
index 6a5b029607c..80992e2e233 100644
--- a/linux-user/uaccess.c
+++ b/linux-user/uaccess.c
@@ -15,7 +15,7 @@ void *lock_user(int type, abi_ulong guest_addr, ssize_t len,
bool copy)
host_addr = g2h_untagged(guest_addr);
#ifdef DEBUG_REMAP
if (copy) {
-host_addr = g_memdup(host_addr, len);
+host_addr = g_memdup2_qemu(host_addr, len);
} else {
host_addr = g_malloc0(len);
}
--
2.31.1
[PATCH 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/ppc/spapr_pci.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7430bd63142..79c0e8d4f98 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2201,10 +2201,10 @@ static int spapr_pci_post_load(void *opaque, int
version_id)
int i;
for (i = 0; i < sphb->msi_devs_num; ++i) {
-key = g_memdup(>msi_devs[i].key,
- sizeof(sphb->msi_devs[i].key));
-value = g_memdup(>msi_devs[i].value,
- sizeof(sphb->msi_devs[i].value));
+key = g_memdup2_qemu(>msi_devs[i].key,
+ sizeof(sphb->msi_devs[i].key));
+value = g_memdup2_qemu(>msi_devs[i].value,
+ sizeof(sphb->msi_devs[i].value));
g_hash_table_insert(sphb->msi, key, value);
}
g_free(sphb->msi_devs);
--
2.31.1
[RFC PATCH 21/28] ui/clipboard: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- TODO: audit qemu_clipboard_set_data() calls --- ui/clipboard.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/clipboard.c b/ui/clipboard.c index d7b008d62a0..0e12a55d3e5 100644 --- a/ui/clipboard.c +++ b/ui/clipboard.c @@ -123,7 +123,7 @@ void qemu_clipboard_set_data(QemuClipboardPeer *peer, } g_free(info->types[type].data); -info->types[type].data = g_memdup(data, size); +info->types[type].data = g_memdup2_qemu(data, size); info->types[type].size = size; info->types[type].available = true; -- 2.31.1
[PATCH 17/28] hw/rdma: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/rdma/rdma_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
index 98df58f6897..9792b1c8ef5 100644
--- a/hw/rdma/rdma_utils.c
+++ b/hw/rdma/rdma_utils.c
@@ -71,7 +71,7 @@ void rdma_protected_gqueue_append_int64(RdmaProtectedGQueue
*list,
int64_t value)
{
qemu_mutex_lock(>lock);
-g_queue_push_tail(list->list, g_memdup(, sizeof(value)));
+g_queue_push_tail(list->list, g_memdup2_qemu(, sizeof(value)));
qemu_mutex_unlock(>lock);
}
--
2.31.1
[PATCH 20/28] net/colo: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
packet_new() is called from packet_enqueue() with size being 32-bit
(of type SocketReadState::packet_len).
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
net/colo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/colo.c b/net/colo.c
index 3a3e6e89a0c..cfe37b19eac 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -159,7 +159,7 @@ Packet *packet_new(const void *data, int size, int
vnet_hdr_len)
{
Packet *pkt = g_slice_new0(Packet);
-pkt->data = g_memdup(data, size);
+pkt->data = g_memdup2_qemu(data, size);
pkt->size = size;
pkt->creation_ms = qemu_clock_get_ms(QEMU_CLOCK_HOST);
pkt->vnet_hdr_len = vnet_hdr_len;
@@ -214,7 +214,7 @@ Connection *connection_get(GHashTable
*connection_track_table,
Connection *conn = g_hash_table_lookup(connection_track_table, key);
if (conn == NULL) {
-ConnectionKey *new_key = g_memdup(key, sizeof(*key));
+ConnectionKey *new_key = g_memdup2_qemu(key, sizeof(*key));
conn = connection_new(key);
--
2.31.1
[RFC PATCH 19/28] hw/virtio: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
Should we check in_num/out_num in range?
---
hw/net/virtio-net.c | 3 ++-
hw/virtio/virtio-crypto.c | 7 ---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee52..8fa23d5f941 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1449,7 +1449,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
}
iov_cnt = elem->out_num;
-iov2 = iov = g_memdup(elem->out_sg, sizeof(struct iovec) *
elem->out_num);
+iov2 = iov = g_memdup2_qemu(elem->out_sg,
+sizeof(struct iovec) * elem->out_num);
s = iov_to_buf(iov, iov_cnt, 0, , sizeof(ctrl));
iov_discard_front(, _cnt, sizeof(ctrl));
if (s != sizeof(ctrl)) {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 54f9bbb789c..43c1a39e469 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -242,7 +242,8 @@ static void virtio_crypto_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
}
out_num = elem->out_num;
-out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+out_iov_copy = g_memdup2_qemu(elem->out_sg,
+ sizeof(out_iov[0]) * out_num);
out_iov = out_iov_copy;
in_num = elem->in_num;
@@ -605,11 +606,11 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
}
out_num = elem->out_num;
-out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
+out_iov_copy = g_memdup2_qemu(elem->out_sg, sizeof(out_iov[0]) * out_num);
out_iov = out_iov_copy;
in_num = elem->in_num;
-in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
+in_iov_copy = g_memdup2_qemu(elem->in_sg, sizeof(in_iov[0]) * in_num);
in_iov = in_iov_copy;
if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
--
2.31.1
[PATCH 18/28] hw/vfio/pci: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/vfio/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e1ea1d8a23b..5c9acfd9c40 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2040,7 +2040,7 @@ static void vfio_add_ext_cap(VFIOPCIDevice *vdev) * physical device, we cache the config space to avoid overwriting * the original config space when we parse the extended capabilities. */ -config = g_memdup(pdev->config, vdev->config_size); +config = g_memdup2_qemu(pdev->config, vdev->config_size); /* * Extended capabilities are chained with each pointing to the next, so we -- 2.31.1
[PATCH 07/28] hw/9pfs: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/9pfs/9p-synth.c | 2 +-
hw/9pfs/9p.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index b38088e0664..7d983574af5 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -497,7 +497,7 @@ static int synth_name_to_path(FsContext *ctx, V9fsPath
*dir_path,
out:
/* Copy the node pointer to fid */
g_free(target->data);
-target->data = g_memdup(, sizeof(void *));
+target->data = g_memdup2_qemu(, sizeof(void *));
target->size = sizeof(void *);
return 0;
}
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 2815257f425..5bf1bd7229f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -202,7 +202,7 @@ void v9fs_path_copy(V9fsPath *dst, const V9fsPath *src)
{
v9fs_path_free(dst);
dst->size = src->size;
-dst->data = g_memdup(src->data, src->size);
+dst->data = g_memdup2_qemu(src->data, src->size);
}
int v9fs_name_to_path(V9fsState *s, V9fsPath *dirpath,
--
2.31.1
[PATCH 06/28] softmmu: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
softmmu/memory.c | 2 +-
softmmu/vl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..838a274b627 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1140,7 +1140,7 @@ static char *memory_region_escape_name(const char *name)
bytes += memory_region_need_escape(*p) ? 4 : 1;
}
if (bytes == p - name) {
- return g_memdup(name, bytes + 1);
+return g_memdup2_qemu(name, bytes + 1);
}
escaped = g_malloc(bytes + 1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ea05bb39c50..a136ef0bfb6 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1154,7 +1154,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts,
Error **errp)
}
if (nonempty_str(str)) {
size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
-buf = g_memdup(str, size);
+buf = g_memdup2_qemu(str, size);
} else if (nonempty_str(gen_id)) {
if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
return -1;
--
2.31.1
[PATCH 14/28] hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/nvram/fw_cfg.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 9b8dcca4ead..fefcdeb8241 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -205,7 +205,8 @@ static void fw_cfg_bootsplash(FWCfgState *s) /* use little endian format */ bst_le16 = cpu_to_le16(bst_val); fw_cfg_add_file(s, "etc/boot-menu-wait", -g_memdup(_le16, sizeof bst_le16), sizeof bst_le16); +g_memdup2_qemu(_le16, sizeof bst_le16), +sizeof bst_le16); } /* insert splash file if user configurated */ @@ -260,7 +261,7 @@ static void fw_cfg_reboot(FWCfgState *s) } rt_le32 = cpu_to_le32(rt_val); -fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(_le32, 4), 4); +fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup2_qemu(_le32, 4), 4); } static void fw_cfg_write(FWCfgState *s, uint8_t value) @@ -755,7 +756,7 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value) size_t sz = strlen(value) + 1; trace_fw_cfg_add_string(key, trace_key_name(key), value); -fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz); +fw_cfg_add_bytes(s, key, g_memdup2_qemu(value, sz), sz); } void fw_cfg_modify_string(FWCfgState *s, uint16_t key, const char *value) @@ -763,7 +764,7 @@ void fw_cfg_modify_string(FWCfgState *s, uint16_t key, const char *value) size_t sz = strlen(value) + 1; char *old; -old = fw_cfg_modify_bytes_read(s, key, g_memdup(value, sz), sz); +old = fw_cfg_modify_bytes_read(s, key, g_memdup2_qemu(value, sz), sz); g_free(old); } -- 2.31.1
[PATCH 12/28] hw/i386/multiboot: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/multiboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 9e7d69d4705..f536e3c8c96 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -387,7 +387,7 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug(" mb_mods_count = %d", mbs.mb_mods_count);
/* save bootinfo off the stack */
-mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
+mb_bootinfo_data = g_memdup2_qemu(bootinfo, sizeof(bootinfo));
/* Pass variables to option rom */
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
--
2.31.1
[PATCH 11/28] hw/hppa/machine: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/hppa/machine.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c index 2a46af5bc9b..058a81e85dd 100644 --- a/hw/hppa/machine.c +++ b/hw/hppa/machine.c @@ -101,19 +101,19 @@ static FWCfgState *create_fw_cfg(MachineState *ms) val = cpu_to_le64(MIN_SEABIOS_HPPA_VERSION); fw_cfg_add_file(fw_cfg, "/etc/firmware-min-version", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2_qemu(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPPA_TLB_ENTRIES); fw_cfg_add_file(fw_cfg, "/etc/cpu/tlb_entries", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2_qemu(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPPA_BTLB_ENTRIES); fw_cfg_add_file(fw_cfg, "/etc/cpu/btlb_entries", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2_qemu(, sizeof(val)), sizeof(val)); val = cpu_to_le64(HPA_POWER_BUTTON); fw_cfg_add_file(fw_cfg, "/etc/power-button-addr", -g_memdup(, sizeof(val)), sizeof(val)); +g_memdup2_qemu(, sizeof(val)), sizeof(val)); fw_cfg_add_i16(fw_cfg, FW_CFG_BOOT_DEVICE, ms->boot_order[0]); qemu_register_boot_set(fw_cfg_boot_set, fw_cfg); -- 2.31.1
[PATCH 08/28] hw/acpi: Avoid truncating acpi_data_len() to 32-bit
acpi_data_len() returns an unsigned type, which might be bigger
than 32-bit (although it is unlikely such value is returned).
Hold the returned value in an 'unsigned' type to avoid unlikely
size truncation.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/arm/virt-acpi-build.c | 2 +-
hw/i386/acpi-build.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 037cc1fd82c..95543d43e2a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -885,7 +885,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables
*tables)
static void acpi_ram_update(MemoryRegion *mr, GArray *data)
{
-uint32_t size = acpi_data_len(data);
+unsigned size = acpi_data_len(data);
/* Make sure RAM size is correct - in case it got changed
* e.g. by migration */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a33ac8b91e1..aa269914b49 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2660,7 +2660,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState
*machine)
static void acpi_ram_update(MemoryRegion *mr, GArray *data)
{
-uint32_t size = acpi_data_len(data);
+unsigned size = acpi_data_len(data);
/* Make sure RAM size is correct - in case it got changed e.g. by
migration */
memory_region_ram_resize(mr, size, _abort);
@@ -2783,7 +2783,7 @@ void acpi_setup(void)
* Though RSDP is small, its contents isn't immutable, so
* we'll update it along with the rest of tables on guest access.
*/
-uint32_t rsdp_size = acpi_data_len(tables.rsdp);
+unsigned rsdp_size = acpi_data_len(tables.rsdp);
build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE,
--
2.31.1
[PATCH 10/28] hw/core/machine: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/core/machine.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 067f42b528f..0808a681360 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -615,8 +615,8 @@ HotpluggableCPUList
*machine_query_hotpluggable_cpus(MachineState *machine)
cpu_item->type = g_strdup(machine->possible_cpus->cpus[i].type);
cpu_item->vcpus_count = machine->possible_cpus->cpus[i].vcpus_count;
-cpu_item->props = g_memdup(>possible_cpus->cpus[i].props,
- sizeof(*cpu_item->props));
+cpu_item->props =
g_memdup2_qemu(>possible_cpus->cpus[i].props,
+ sizeof(*cpu_item->props));
cpu = machine->possible_cpus->cpus[i].cpu;
if (cpu) {
--
2.31.1
[PATCH 09/28] hw/acpi: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- hw/acpi/core.c | 3 ++- hw/i386/acpi-build.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/acpi/core.c b/hw/acpi/core.c index 1e004d0078d..9dd2cf09a0b 100644 --- a/hw/acpi/core.c +++ b/hw/acpi/core.c @@ -637,7 +637,8 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent, suspend[3] = 1 | ((!disable_s3) << 7); suspend[4] = s4_val | ((!disable_s4) << 7); -fw_cfg_add_file(fw_cfg, "etc/system-states", g_memdup(suspend, 6), 6); +fw_cfg_add_file(fw_cfg, "etc/system-states", +g_memdup2_qemu(suspend, 6), 6); } } diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index aa269914b49..54494ca1f65 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2785,7 +2785,7 @@ void acpi_setup(void) */ unsigned rsdp_size = acpi_data_len(tables.rsdp); -build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size); +build_state->rsdp = g_memdup2_qemu(tables.rsdp->data, rsdp_size); fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE, acpi_build_update, NULL, build_state, build_state->rsdp, rsdp_size, true); -- 2.31.1
[PATCH 05/28] block/qcow2-bitmap: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- block/qcow2-bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 8fb47315515..ec303acb46b 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1599,7 +1599,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, name); goto fail; } -tb = g_memdup(>table, sizeof(bm->table)); +tb = g_memdup2_qemu(>table, sizeof(bm->table)); bm->table.offset = 0; bm->table.size = 0; QSIMPLEQ_INSERT_TAIL(_tables, tb, entry); -- 2.31.1
[PATCH 04/28] accel/tcg: Replace g_memdup() by g_memdup2_qemu()
Per
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
The old API took the size of the memory to duplicate as a guint,
whereas most memory functions take memory sizes as a gsize. This
made it easy to accidentally pass a gsize to g_memdup(). For large
values, that would lead to a silent truncation of the size from 64
to 32 bits, and result in a heap area being returned which is
significantly smaller than what the caller expects. This can likely
be exploited in various modules to cause a heap buffer overflow.
Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
Signed-off-by: Philippe Mathieu-Daudé
---
accel/tcg/cputlb.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b1e5471f949..1d5069a30d1 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -826,7 +826,7 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, target_ulong
addr,
tlb_flush_range_by_mmuidx_async_0(cpu, d);
} else {
/* Otherwise allocate a structure, freed by the worker. */
-TLBFlushRangeData *p = g_memdup(, sizeof(d));
+TLBFlushRangeData *p = g_memdup2_qemu(, sizeof(d));
async_run_on_cpu(cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
@@ -868,7 +868,7 @@ void tlb_flush_range_by_mmuidx_all_cpus(CPUState *src_cpu,
/* Allocate a separate data block for each destination cpu. */
CPU_FOREACH(dst_cpu) {
if (dst_cpu != src_cpu) {
-TLBFlushRangeData *p = g_memdup(, sizeof(d));
+TLBFlushRangeData *p = g_memdup2_qemu(, sizeof(d));
async_run_on_cpu(dst_cpu,
tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
@@ -918,13 +918,13 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState
*src_cpu,
/* Allocate a separate data block for each destination cpu. */
CPU_FOREACH(dst_cpu) {
if (dst_cpu != src_cpu) {
-p = g_memdup(, sizeof(d));
+p = g_memdup2_qemu(, sizeof(d));
async_run_on_cpu(dst_cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
}
-p = g_memdup(, sizeof(d));
+p = g_memdup2_qemu(, sizeof(d));
async_safe_run_on_cpu(src_cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
--
2.31.1
[PATCH 03/28] qapi: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. Replace g_memdup() by the safer g_memdup2_qemu() wrapper. Signed-off-by: Philippe Mathieu-Daudé --- qapi/qapi-clone-visitor.c | 16 qapi/qapi-visit-core.c| 6 -- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c index c45c5caa3b8..fb38505d982 100644 --- a/qapi/qapi-clone-visitor.c +++ b/qapi/qapi-clone-visitor.c @@ -37,7 +37,7 @@ static bool qapi_clone_start_struct(Visitor *v, const char *name, void **obj, return true; } -*obj = g_memdup(*obj, size); +*obj = g_memdup2_qemu(*obj, size); qcv->depth++; return true; } @@ -65,8 +65,8 @@ static GenericList *qapi_clone_next_list(Visitor *v, GenericList *tail, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Unshare the tail of the list cloned by g_memdup() */ -tail->next = g_memdup(tail->next, size); +/* Unshare the tail of the list cloned by g_memdup2() */ +tail->next = g_memdup2_qemu(tail->next, size); return tail->next; } @@ -83,7 +83,7 @@ static bool qapi_clone_type_int64(Visitor *v, const char *name, int64_t *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -93,7 +93,7 @@ static bool qapi_clone_type_uint64(Visitor *v, const char *name, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -103,7 +103,7 @@ static bool qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } @@ -114,7 +114,7 @@ static bool qapi_clone_type_str(Visitor *v, const char *name, char **obj, assert(qcv->depth); /* - * Pointer was already cloned by g_memdup; create fresh copy. + * Pointer was already cloned by g_memdup2; create fresh copy. * Note that as long as qobject-output-visitor accepts NULL instead of * "", then we must do likewise. However, we want to obey the * input visitor semantics of never producing NULL when the empty @@ -130,7 +130,7 @@ static bool qapi_clone_type_number(Visitor *v, const char *name, double *obj, QapiCloneVisitor *qcv = to_qcv(v); assert(qcv->depth); -/* Value was already cloned by g_memdup() */ +/* Value was already cloned by g_memdup2() */ return true; } diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index a641adec51e..ebabe63b6ea 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -413,8 +413,10 @@ bool visit_type_enum(Visitor *v, const char *name, int *obj, case VISITOR_OUTPUT: return output_type_enum(v, name, obj, lookup, errp); case VISITOR_CLONE: -/* nothing further to do, scalar value was already copied by - * g_memdup() during visit_start_*() */ +/* + * nothing further to do, scalar value was already copied by + * g_memdup2() during visit_start_*() + */ return true; case VISITOR_DEALLOC: /* nothing to deallocate for a scalar */ -- 2.31.1
[PATCH 02/28] glib-compat: Introduce g_memdup2() wrapper
When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68
(Fedora 34 provides GLib 2.68.1) we get:
hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use
'g_memdup2' instead [-Werror,-Wdeprecated-declarations]
...
g_memdup() has been updated by g_memdup2() to fix eventual security
issues (size argument is 32-bit and could be truncated / wrapping).
GLib recommends to copy their static inline version of g_memdup2():
https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
Our glib-compat.h provides a comment explaining how to deal with
these deprecated declarations (see commit e71e8cc0355
"glib: enforce the minimum required version and warn about old APIs").
Following this comment suggestion, implement the g_memdup2_qemu()
wrapper to g_memdup2(), and use the safer equivalent inlined when
we are using pre-2.68 GLib.
Reported-by: Eric Blake
Signed-off-by: Philippe Mathieu-Daudé
---
include/glib-compat.h | 36
1 file changed, 36 insertions(+)
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 9e95c888f54..6577d9ab393 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,42 @@
* without generating warnings.
*/
+/*
+ * g_memdup2_qemu:
+ * @mem: (nullable): the memory to copy.
+ * @byte_size: the number of bytes to copy.
+ *
+ * Allocates @byte_size bytes of memory, and copies @byte_size bytes into it
+ * from @mem. If @mem is %NULL it returns %NULL.
+ *
+ * This replaces g_memdup(), which was prone to integer overflows when
+ * converting the argument from a #gsize to a #guint.
+ *
+ * This static inline version is a backport of the new public API from
+ * GLib 2.68, kept internal to GLib for backport to older stable releases.
+ * See https://gitlab.gnome.org/GNOME/glib/-/issues/2319.
+ *
+ * Returns: (nullable): a pointer to the newly-allocated copy of the memory,
+ * or %NULL if @mem is %NULL.
+ */
+static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size)
+{
+#if GLIB_CHECK_VERSION(2, 68, 0)
+return g_memdup2(mem, byte_size);
+#else
+gpointer new_mem;
+
+if (mem && byte_size != 0) {
+new_mem = g_malloc(byte_size);
+memcpy(new_mem, mem, byte_size);
+} else {
+new_mem = NULL;
+}
+
+return new_mem;
+#endif
+}
+
#if defined(G_OS_UNIX)
/*
* Note: The fallback implementation is not MT-safe, and it returns a copy of
--
2.31.1
[PATCH 01/28] hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
vmbus_save_req() and vmbus_load_req() are not used.
Remove them to avoid maintaining dead code.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/hyperv/vmbus.h | 3 --
hw/hyperv/vmbus.c | 59 ---
2 files changed, 62 deletions(-)
diff --git a/include/hw/hyperv/vmbus.h b/include/hw/hyperv/vmbus.h
index f98bea3888d..8ea660dd8e6 100644
--- a/include/hw/hyperv/vmbus.h
+++ b/include/hw/hyperv/vmbus.h
@@ -223,7 +223,4 @@ int vmbus_map_sgl(VMBusChanReq *req, DMADirection dir,
struct iovec *iov,
void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
unsigned iov_cnt, size_t accessed);
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req);
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size);
-
#endif
diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index c9887d5a7bc..18d3c3b9240 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1311,65 +1311,6 @@ static const VMStateDescription vmstate_vmbus_chan_req =
{
}
};
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req)
-{
-VMBusChanReqSave req_save;
-
-req_save.chan_idx = req->chan->subchan_idx;
-req_save.pkt_type = req->pkt_type;
-req_save.msglen = req->msglen;
-req_save.msg = req->msg;
-req_save.transaction_id = req->transaction_id;
-req_save.need_comp = req->need_comp;
-req_save.num = req->sgl.nsg;
-req_save.sgl = g_memdup(req->sgl.sg,
-req_save.num * sizeof(ScatterGatherEntry));
-
-vmstate_save_state(f, _vmbus_chan_req, _save, NULL);
-
-g_free(req_save.sgl);
-}
-
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size)
-{
-VMBusChanReqSave req_save;
-VMBusChanReq *req = NULL;
-VMBusChannel *chan = NULL;
-uint32_t i;
-
-vmstate_load_state(f, _vmbus_chan_req, _save, 0);
-
-if (req_save.chan_idx >= dev->num_channels) {
-error_report("%s: %u(chan_idx) > %u(num_channels)", __func__,
- req_save.chan_idx, dev->num_channels);
-goto out;
-}
-chan = >channels[req_save.chan_idx];
-
-if (vmbus_channel_reserve(chan, 0, req_save.msglen)) {
-goto out;
-}
-
-req = vmbus_alloc_req(chan, size, req_save.pkt_type, req_save.msglen,
- req_save.transaction_id, req_save.need_comp);
-if (req_save.msglen) {
-memcpy(req->msg, req_save.msg, req_save.msglen);
-}
-
-for (i = 0; i < req_save.num; i++) {
-qemu_sglist_add(>sgl, req_save.sgl[i].base, req_save.sgl[i].len);
-}
-
-out:
-if (req_save.msglen) {
-g_free(req_save.msg);
-}
-if (req_save.num) {
-g_free(req_save.sgl);
-}
-return req;
-}
-
static void channel_event_cb(EventNotifier *e)
{
VMBusChannel *chan = container_of(e, VMBusChannel, notifier);
--
2.31.1
[PATCH 00/28] glib: Replace g_memdup() by g_memdup2_qemu()
Per https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 The old API took the size of the memory to duplicate as a guint, whereas most memory functions take memory sizes as a gsize. This made it easy to accidentally pass a gsize to g_memdup(). For large values, that would lead to a silent truncation of the size from 64 to 32 bits, and result in a heap area being returned which is significantly smaller than what the caller expects. This can likely be exploited in various modules to cause a heap buffer overflow. g_memdup() as been deprecated in GLib 2.68. Since QEMU defines GLIB_VERSION_MAX_ALLOWED to GLIB_VERSION_2_56, the deprecation is not displayed (on GLib >= 2.68 such available on Fedora 34). However the function is still unsafe, so it is better to avoid its use. This series provides the safely equivalent g_memdup2_qemu() wrapper, and replace all g_memdup() calls by it. The previous link recommend to audit the call sites. Most of the calls use byte_size=sizeof(STRUCT), and no STRUCT appears to be > 4GiB. Few calls use unsigned/size_t/uint16_t. Where code is doing multiplication, patches are sent as RFC. In particular: hw/net/virtio-net.c hw/virtio/virtio-crypto.c Please review, Phil. Philippe Mathieu-Daudé (28): hw/hyperv/vmbus: Remove unused vmbus_load/save_req() glib-compat: Introduce g_memdup2() wrapper qapi: Replace g_memdup() by g_memdup2_qemu() accel/tcg: Replace g_memdup() by g_memdup2_qemu() block/qcow2-bitmap: Replace g_memdup() by g_memdup2_qemu() softmmu: Replace g_memdup() by g_memdup2_qemu() hw/9pfs: Replace g_memdup() by g_memdup2_qemu() hw/acpi: Avoid truncating acpi_data_len() to 32-bit hw/acpi: Replace g_memdup() by g_memdup2_qemu() hw/core/machine: Replace g_memdup() by g_memdup2_qemu() hw/hppa/machine: Replace g_memdup() by g_memdup2_qemu() hw/i386/multiboot: Replace g_memdup() by g_memdup2_qemu() hw/net/eepro100: Replace g_memdup() by g_memdup2_qemu() hw/nvram/fw_cfg: Replace g_memdup() by g_memdup2_qemu() hw/scsi/mptsas: Replace g_memdup() by g_memdup2_qemu() hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2_qemu() hw/rdma: Replace g_memdup() by g_memdup2_qemu() hw/vfio/pci: Replace g_memdup() by g_memdup2_qemu() RFC hw/virtio: Replace g_memdup() by g_memdup2_qemu() net/colo: Replace g_memdup() by g_memdup2_qemu() RFC ui/clipboard: Replace g_memdup() by g_memdup2_qemu() RFC linux-user: Replace g_memdup() by g_memdup2_qemu() tests/unit: Replace g_memdup() by g_memdup2_qemu() tests/qtest: Replace g_memdup() by g_memdup2_qemu() target/arm: Replace g_memdup() by g_memdup2_qemu() target/ppc: Replace g_memdup() by g_memdup2_qemu() contrib: Replace g_memdup() by g_memdup2_qemu() checkpatch: Do not allow deprecated g_memdup() include/glib-compat.h | 36 ++ include/hw/hyperv/vmbus.h | 3 -- accel/tcg/cputlb.c | 8 ++--- block/qcow2-bitmap.c| 2 +- contrib/plugins/lockstep.c | 2 +- contrib/rdmacm-mux/main.c | 10 +++ hw/9pfs/9p-synth.c | 2 +- hw/9pfs/9p.c| 2 +- hw/acpi/core.c | 3 +- hw/arm/virt-acpi-build.c| 2 +- hw/core/machine.c | 4 +-- hw/hppa/machine.c | 8 ++--- hw/hyperv/vmbus.c | 59 - hw/i386/acpi-build.c| 6 ++-- hw/i386/multiboot.c | 2 +- hw/net/eepro100.c | 2 +- hw/net/virtio-net.c | 3 +- hw/nvram/fw_cfg.c | 9 +++--- hw/ppc/spapr_pci.c | 8 ++--- hw/rdma/rdma_utils.c| 2 +- hw/scsi/mptsas.c| 5 ++-- hw/vfio/pci.c | 2 +- hw/virtio/virtio-crypto.c | 7 +++-- linux-user/syscall.c| 2 +- linux-user/uaccess.c| 2 +- net/colo.c | 4 +-- qapi/qapi-clone-visitor.c | 16 +- qapi/qapi-visit-core.c | 6 ++-- softmmu/memory.c| 2 +- softmmu/vl.c| 2 +- target/arm/helper.c | 7 +++-- target/ppc/mmu-hash64.c | 3 +- tests/qtest/libqos/ahci.c | 6 ++-- tests/qtest/libqos/qgraph.c | 2 +- tests/unit/ptimer-test.c| 22 +++--- tests/unit/test-iov.c | 26 ui/clipboard.c | 2 +- scripts/checkpatch.pl | 5 38 files changed, 141 insertions(+), 153 deletions(-) -- 2.31.1
[PATCH v6 10/11] block: use int64_t instead of int in driver discard handlers
We are generally moving to int64_t for both offset and bytes parameters
on all io paths.
Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.
We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).
So, convert driver discard handlers bytes parameter to int64_t.
The only caller of all updated function is bdrv_co_pdiscard in
block/io.c. It is already prepared to work with 64bit requests, but
pass at most max(bs->bl.max_pdiscard, INT_MAX) to the driver.
Let's look at all updated functions:
blkdebug: all calculations are still OK, thanks to
bdrv_check_qiov_request().
both rule_check and bdrv_co_pdiscard are 64bit
blklogwrites: pass to blk_loc_writes_co_log which is 64bit
blkreplay, copy-on-read, filter-compress: pass to bdrv_co_pdiscard, OK
copy-before-write: pass to bdrv_co_pdiscard which is 64bit and to
cbw_do_copy_before_write which is 64bit
file-posix: one handler calls raw_account_discard() is 64bit and both
handlers calls raw_do_pdiscard(). Update raw_do_pdiscard, which pass
to RawPosixAIOData::aio_nbytes, which is 64bit (and calls
raw_account_discard())
gluster: somehow, third argument of glfs_discard_async is size_t.
Let's set max_pdiscard accordingly.
iscsi: iscsi_allocmap_set_invalid is 64bit,
!is_byte_request_lun_aligned is 64bit.
list.num is uint32_t. Let's clarify max_pdiscard and
pdiscard_alignment.
mirror_top: pass to bdrv_mirror_top_do_write() which is
64bit
nbd: protocol limitation. max_pdiscard is alredy set strict enough,
keep it as is for now.
nvme: buf.nlb is uint32_t and we do shift. So, add corresponding limits
to nvme_refresh_limits().
preallocate: pass to bdrv_co_pdiscard() which is 64bit.
rbd: pass to qemu_rbd_start_co() which is 64bit.
qcow2: calculations are still OK, thanks to bdrv_check_qiov_request(),
qcow2_cluster_discard() is 64bit.
raw-format: raw_adjust_offset() is 64bit, bdrv_co_pdiscard too.
throttle: pass to bdrv_co_pdiscard() which is 64bit and to
throttle_group_co_io_limits_intercept() which is 64bit as well.
test-block-iothread: bytes argument is unused
Great! Now all drivers are prepared to handle 64bit discard requests,
or else have explicit max_pdiscard limits.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block_int.h| 2 +-
block/blkdebug.c | 2 +-
block/blklogwrites.c | 4 ++--
block/blkreplay.c| 2 +-
block/copy-before-write.c| 2 +-
block/copy-on-read.c | 2 +-
block/file-posix.c | 7 ---
block/filter-compress.c | 2 +-
block/gluster.c | 7 +--
block/iscsi.c| 16 +++-
block/mirror.c | 2 +-
block/nbd.c | 6 --
block/nvme.c | 14 +-
block/preallocate.c | 2 +-
block/qcow2.c| 2 +-
block/raw-format.c | 2 +-
block/rbd.c | 4 ++--
block/throttle.c | 2 +-
tests/unit/test-block-iothread.c | 2 +-
block/trace-events | 4 ++--
20 files changed, 55 insertions(+), 31 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5977859f80..4e45352268 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -302,7 +302,7 @@ struct BlockDriver {
int coroutine_fn (*bdrv_co_pwrite_zeroes)(BlockDriverState *bs,
int64_t offset, int64_t bytes, BdrvRequestFlags flags);
int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs,
-int64_t offset, int bytes);
+int64_t offset, int64_t bytes);
/* Map [offset, offset + nbytes) range onto a child of @bs to copy from,
* and invoke bdrv_co_copy_range_from(child, ...), or invoke
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 742b4a3834..bbf2948703 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -717,7 +717,7 @@ static int coroutine_fn
blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
}
static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
- int64_t offset, int bytes)
+ int64_t offset, int64_t bytes)
{
uint32_t align = bs->bl.pdiscard_alignment;
int err;
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index d7ae64c22d..f7a251e91f 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -484,9 +484,9 @@ static int coroutine_fn
blk_log_writes_co_flush_to_disk(BlockDriverState *bs)
}
static int coroutine_fn
-blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
+blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
-return blk_log_writes_co_log(bs, offset, count, NULL, 0,
+return
[PATCH v6 09/11] block: make BlockLimits::max_pdiscard 64bit
We are going to support 64 bit discard requests. Now update the
limit variable. It's absolutely safe. The variable is set in some
drivers, and used in bdrv_co_pdiscard().
Update also max_pdiscard variable in bdrv_co_pdiscard(), so that
bdrv_co_pdiscard() is now prepared for 64bit requests. The remaining
logic including num, offset and bytes variables is already
supporting 64bit requests.
So the only thing that prevents 64 bit requests is limiting
max_pdiscard variable to INT_MAX in bdrv_co_pdiscard().
We'll drop this limitation after updating all block drivers.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Eric Blake
---
include/block/block_int.h | 11 ++-
block/io.c| 3 ++-
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 112a42ae8f..5977859f80 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -664,11 +664,12 @@ typedef struct BlockLimits {
* otherwise. */
uint32_t request_alignment;
-/* Maximum number of bytes that can be discarded at once (since it
- * is signed, it must be < 2G, if set). Must be multiple of
- * pdiscard_alignment, but need not be power of 2. May be 0 if no
- * inherent 32-bit limit */
-int32_t max_pdiscard;
+/*
+ * Maximum number of bytes that can be discarded at once. Must be multiple
+ * of pdiscard_alignment, but need not be power of 2. May be 0 if no
+ * inherent 64-bit limit.
+ */
+int64_t max_pdiscard;
/* Optimal alignment for discard requests in bytes. A power of 2
* is best but not mandatory. Must be a multiple of
diff --git a/block/io.c b/block/io.c
index c386cd700e..a5ba1f4cf2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2997,7 +2997,8 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child,
int64_t offset,
int64_t bytes)
{
BdrvTrackedRequest req;
-int max_pdiscard, ret;
+int ret;
+int64_t max_pdiscard;
int head, tail, align;
BlockDriverState *bs = child->bs;
--
2.29.2
[PATCH v6 11/11] block/io: allow 64bit discard requests
Now that all drivers are updated by the previous commit, we can drop the last limiter on pdiscard path: INT_MAX in bdrv_co_pdiscard(). Now everything is prepared for implementing incredibly cool and fast big-discard requests in NBD and qcow2. And any other driver which wants it of course. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/io.c b/block/io.c index a5ba1f4cf2..1e9a3a1752 100644 --- a/block/io.c +++ b/block/io.c @@ -3042,7 +3042,7 @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset, goto out; } -max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard, INT_MAX), +max_pdiscard = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pdiscard, INT64_MAX), align); assert(max_pdiscard >= bs->bl.request_alignment); -- 2.29.2
[PATCH v6 07/11] block: use int64_t instead of int in driver write_zeroes handlers
We are generally moving to int64_t for both offset and bytes parameters on all io paths. Main motivation is realization of 64-bit write_zeroes operation for fast zeroing large disk chunks, up to the whole disk. We chose signed type, to be consistent with off_t (which is signed) and with possibility for signed return type (where negative value means error). So, convert driver write_zeroes handlers bytes parameter to int64_t. The only caller of all updated function is bdrv_co_do_pwrite_zeroes(). bdrv_co_do_pwrite_zeroes() itself is of course OK with widening of callee parameter type. Also, bdrv_co_do_pwrite_zeroes()'s max_write_zeroes is limited to INT_MAX. So, updated functions all are safe, they will not get "bytes" larger than before. Still, let's look through all updated functions, and add assertions to the ones which are actually unprepared to values larger than INT_MAX. For these drivers also set explicit max_pwrite_zeroes limit. Let's go: blkdebug: calculations can't overflow, thanks to bdrv_check_qiov_request() in generic layer. rule_check() and bdrv_co_pwrite_zeroes() both have 64bit argument. blklogwrites: pass to blk_log_writes_co_log() with 64bit argument. blkreplay, copy-on-read, filter-compress: pass to bdrv_co_pwrite_zeroes() which is OK copy-before-write: Calls cbw_do_copy_before_write() and bdrv_co_pwrite_zeroes, both have 64bit argument. file-posix: both handler calls raw_do_pwrite_zeroes, which is updated. In raw_do_pwrite_zeroes() calculations are OK due to bdrv_check_qiov_request(), bytes go to RawPosixAIOData::aio_nbytes which is uint64_t. Check also where that uint64_t gets handed: handle_aiocb_write_zeroes_block() passes a uint64_t[2] to ioctl(BLKZEROOUT), handle_aiocb_write_zeroes() calls do_fallocate() which takes off_t (and we compile to always have 64-bit off_t), as does handle_aiocb_write_zeroes_unmap. All look safe. gluster: bytes go to GlusterAIOCB::size which is int64_t and to glfs_zerofill_async works with off_t. iscsi: Aha, here we deal with iscsi_writesame16_task() that has uint32_t num_blocks argument and iscsi_writesame16_task() has uint16_t argument. Make comments, add assertions and clarify max_pwrite_zeroes calculation. iscsi_allocmap_() functions already has int64_t argument is_byte_request_lun_aligned is simple to update, do it. mirror_top: pass to bdrv_mirror_top_do_write which has uint64_t argument nbd: Aha, here we have protocol limitation, and NBDRequest::len is uint32_t. max_pwrite_zeroes is cleanly set to 32bit value, so we are OK for now. nvme: Again, protocol limitation. And no inherent limit for write-zeroes at all. But from code that calculates cdw12 it's obvious that we do have limit and alignment. Let's clarify it. Also, obviously the code is not prepared to handle bytes=0. Let's handle this case too. trace events already 64bit preallocate: pass to handle_write() and bdrv_co_pwrite_zeroes(), both 64bit. rbd: pass to qemu_rbd_start_co() which is 64bit. qcow2: offset + bytes and alignment still works good (thanks to bdrv_check_qiov_request()), so tail calculation is OK qcow2_subcluster_zeroize() has 64bit argument, should be OK trace events updated qed: qed_co_request wants int nb_sectors. Also in code we have size_t used for request length which may be 32bit. So, let's just keep INT_MAX as a limit (aligning it down to pwrite_zeroes_alignment) and don't care. raw-format: Is OK. raw_adjust_offset and bdrv_co_pwrite_zeroes are both 64bit. throttle: Both throttle_group_co_io_limits_intercept() and bdrv_co_pwrite_zeroes() are 64bit. vmdk: pass to vmdk_pwritev which is 64bit quorum: pass to quorum_co_pwritev() which is 64bit Hooray! At this point all block drivers are prepared to support 64bit write-zero requests, or have explicitly set max_pwrite_zeroes. Signed-off-by: Vladimir Sementsov-Ogievskiy --- include/block/block_int.h | 2 +- block/blkdebug.c | 2 +- block/blklogwrites.c | 4 ++-- block/blkreplay.c | 2 +- block/copy-before-write.c | 2 +- block/copy-on-read.c | 2 +- block/file-posix.c| 6 +++--- block/filter-compress.c | 2 +- block/gluster.c | 6 +++--- block/iscsi.c | 30 -- block/mirror.c| 2 +- block/nbd.c | 6 -- block/nvme.c | 24 +--- block/preallocate.c | 2 +- block/qcow2.c | 2 +- block/qed.c | 9 - block/quorum.c| 2 +- block/raw-format.c| 2 +- block/rbd.c | 4 ++-- block/throttle.c | 2 +- block/vmdk.c | 2 +- block/trace-events| 4 ++-- 22 files changed, 78 insertions(+), 41 deletions(-) diff --git a/include/block/block_int.h b/include/block/block_int.h index 6c47985d5f..112a42ae8f 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -300,7 +300,7
[PATCH v6 06/11] block: make BlockLimits::max_pwrite_zeroes 64bit
We are going to support 64 bit write-zeroes requests. Now update the
limit variable. It's absolutely safe. The variable is set in some
drivers, and used in bdrv_co_do_pwrite_zeroes().
Update also max_write_zeroes variable in bdrv_co_do_pwrite_zeroes(), so
that bdrv_co_do_pwrite_zeroes() is now prepared to 64bit requests. The
remaining logic including num, offset and bytes variables is already
supporting 64bit requests.
So the only thing that prevents 64 bit requests is limiting
max_write_zeroes variable to INT_MAX in bdrv_co_do_pwrite_zeroes().
We'll drop this limitation after updating all block drivers.
Ah, we also have bdrv_check_request32() in bdrv_co_pwritev_part(). It
will be modified to do bdrv_check_request() for write-zeroes path.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block_int.h | 9 +
block/io.c| 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f40a2e1f5d..6c47985d5f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -676,10 +676,11 @@ typedef struct BlockLimits {
* that is set. May be 0 if bl.request_alignment is good enough */
uint32_t pdiscard_alignment;
-/* Maximum number of bytes that can zeroized at once (since it is
- * signed, it must be < 2G, if set). Must be multiple of
- * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
-int32_t max_pwrite_zeroes;
+/*
+ * Maximum number of bytes that can zeroized at once. Must be multiple of
+ * pwrite_zeroes_alignment. 0 means no limit.
+ */
+int64_t max_pwrite_zeroes;
/* Optimal alignment for write zeroes requests in bytes. A power
* of 2 is best but not mandatory. Must be a multiple of
diff --git a/block/io.c b/block/io.c
index 5aca909a80..b4dce946bd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1869,7 +1869,7 @@ static int coroutine_fn
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
int head = 0;
int tail = 0;
-int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
+int64_t max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
bs->bl.request_alignment);
int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
--
2.29.2
