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(-)
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
Hi Laurent, On 9/3/21 19:44, Philippe Mathieu-Daudé wrote: > This series provides the safely equivalent g_memdup2() wrapper, > and replace all g_memdup() calls by it. > 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( Is it possible to take the reviewed patches 2, 24 and 28 via qemu-trivial, so the rest can be merged slowly by each submaintainer?
Le 15/12/2021 à 17:54, Philippe Mathieu-Daudé a écrit : > Hi Laurent, > > On 9/3/21 19:44, Philippe Mathieu-Daudé wrote: > >> This series provides the safely equivalent g_memdup2() wrapper, >> and replace all g_memdup() calls by it. > >> 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( > Is it possible to take the reviewed patches 2, 24 and 28 > via qemu-trivial, so the rest can be merged slowly by > each submaintainer? > > Done for these 3 patches. Thanks, Laurent
vmbus_save_req() and vmbus_load_req() are not used.
Remove them to avoid maintaining dead code.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
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, &vmstate_vmbus_chan_req, &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, &vmstate_vmbus_chan_req, &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 = &dev->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(&req->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
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 <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
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
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 <eblake@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/glib-compat.h | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 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 <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> 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)
> +
As per our style wouldn't it make sense to just call it qemu_memdup(m,
s)?
--
Alex Bennée
On 12/16/21 15:11, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> 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 <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> 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)
>> +
>
> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> s)?
I followed the documentation in include/glib-compat.h:
/*
* Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
allowing
* use of functions from newer GLib via this compat header needs a little
* trickery to prevent warnings being emitted.
*
* Consider a function from newer glib-X.Y that we want to use
*
* int g_foo(const char *wibble)
*
* We must define a static inline function with the same signature that does
* what we need, but with a "_qemu" suffix e.g.
*
* static inline void g_foo_qemu(const char *wibble)
* {
* #if GLIB_CHECK_VERSION(X, Y, 0)
* g_foo(wibble)
* #else
* g_something_equivalent_in_older_glib(wibble);
* #endif
* }
*
* The #pragma at the top of this file turns off -Wdeprecated-declarations,
* ensuring this wrapper function impl doesn't trigger the compiler warning
* about using too new glib APIs. Finally we can do
*
* #define g_foo(a) g_foo_qemu(a)
*
* So now the code elsewhere in QEMU, which *does* have the
* -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
* without generating warnings.
*/
which is how g_unix_get_passwd_entry_qemu() is implemented.
Should we reword the documentation first?
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 12/16/21 15:11, Alex Bennée wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> 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").
>>>
<snip>
>>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>>> +
>>
>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>> s)?
>
> I followed the documentation in include/glib-compat.h:
>
> /*
> * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> allowing
> * use of functions from newer GLib via this compat header needs a little
> * trickery to prevent warnings being emitted.
> *
> * Consider a function from newer glib-X.Y that we want to use
> *
> * int g_foo(const char *wibble)
> *
> * We must define a static inline function with the same signature that does
> * what we need, but with a "_qemu" suffix e.g.
> *
> * static inline void g_foo_qemu(const char *wibble)
> * {
> * #if GLIB_CHECK_VERSION(X, Y, 0)
> * g_foo(wibble)
> * #else
> * g_something_equivalent_in_older_glib(wibble);
> * #endif
> * }
> *
> * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> * ensuring this wrapper function impl doesn't trigger the compiler warning
> * about using too new glib APIs. Finally we can do
> *
> * #define g_foo(a) g_foo_qemu(a)
> *
> * So now the code elsewhere in QEMU, which *does* have the
> * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> * without generating warnings.
> */
>
> which is how g_unix_get_passwd_entry_qemu() is implemented.
Yet later we have qemu_g_test_slow following the style guide. Also I'm
confused by the usage of g_unix_get_passwd_entry_qemu because the only
place I see it in qga/commands-posix-ssh.c right before it does:
#define g_unix_get_passwd_entry_qemu(username, err) \
test_get_passwd_entry(username, err)
although I think that only hold when the file is built with
QGA_BUILD_UNIT_TEST.
> Should we reword the documentation first?
The original wording in glib-compat.h was added by Daniel in 2018 but
the commit that added the password function comments:
Since the fallback version is still unsafe, I would rather keep the
_qemu postfix, to make sure it's not being misused by mistake. When/if
necessary, we can implement a safer fallback and drop the _qemu suffix.
So if we are going to make a distinction between a qemu prefix and
suffix we should agree that and add it to the style document.
--
Alex Bennée
Alex,
I've added this patch to my trivial patches branch, do you want I drop it?
Thanks,
Laurent
On 17/12/2021 12:10, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> On 12/16/21 15:11, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>
>>>> 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").
>>>>
> <snip>
>>>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
>>>> +
>>> As per our style wouldn't it make sense to just call it qemu_memdup(m,
>>> s)?
>> I followed the documentation in include/glib-compat.h:
>>
>> /*
>> * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
>> allowing
>> * use of functions from newer GLib via this compat header needs a little
>> * trickery to prevent warnings being emitted.
>> *
>> * Consider a function from newer glib-X.Y that we want to use
>> *
>> * int g_foo(const char *wibble)
>> *
>> * We must define a static inline function with the same signature that does
>> * what we need, but with a "_qemu" suffix e.g.
>> *
>> * static inline void g_foo_qemu(const char *wibble)
>> * {
>> * #if GLIB_CHECK_VERSION(X, Y, 0)
>> * g_foo(wibble)
>> * #else
>> * g_something_equivalent_in_older_glib(wibble);
>> * #endif
>> * }
>> *
>> * The #pragma at the top of this file turns off -Wdeprecated-declarations,
>> * ensuring this wrapper function impl doesn't trigger the compiler warning
>> * about using too new glib APIs. Finally we can do
>> *
>> * #define g_foo(a) g_foo_qemu(a)
>> *
>> * So now the code elsewhere in QEMU, which *does* have the
>> * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
>> * without generating warnings.
>> */
>>
>> which is how g_unix_get_passwd_entry_qemu() is implemented.
> Yet later we have qemu_g_test_slow following the style guide. Also I'm
> confused by the usage of g_unix_get_passwd_entry_qemu because the only
> place I see it in qga/commands-posix-ssh.c right before it does:
>
> #define g_unix_get_passwd_entry_qemu(username, err) \
> test_get_passwd_entry(username, err)
>
> although I think that only hold when the file is built with
> QGA_BUILD_UNIT_TEST.
>
>> Should we reword the documentation first?
> The original wording in glib-compat.h was added by Daniel in 2018 but
> the commit that added the password function comments:
>
> Since the fallback version is still unsafe, I would rather keep the
> _qemu postfix, to make sure it's not being misused by mistake. When/if
> necessary, we can implement a safer fallback and drop the _qemu suffix.
>
> So if we are going to make a distinction between a qemu prefix and
> suffix we should agree that and add it to the style document.
>
Laurent Vivier <lvivier@redhat.com> writes: > Alex, > > I've added this patch to my trivial patches branch, do you want I drop > it? No - the patch itself is fine. I would like to fix up the consistency later if we can agree on what it should be. -- Alex Bennée
On Fri, Dec 17, 2021 at 11:10:31AM +0000, Alex Bennée wrote:
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > On 12/16/21 15:11, Alex Bennée wrote:
> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> >>
> >>> 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").
> >>>
> <snip>
> >>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s)
> >>> +
> >>
> >> As per our style wouldn't it make sense to just call it qemu_memdup(m,
> >> s)?
> >
> > I followed the documentation in include/glib-compat.h:
> >
> > /*
> > * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above,
> > allowing
> > * use of functions from newer GLib via this compat header needs a little
> > * trickery to prevent warnings being emitted.
> > *
> > * Consider a function from newer glib-X.Y that we want to use
> > *
> > * int g_foo(const char *wibble)
> > *
> > * We must define a static inline function with the same signature that does
> > * what we need, but with a "_qemu" suffix e.g.
> > *
> > * static inline void g_foo_qemu(const char *wibble)
> > * {
> > * #if GLIB_CHECK_VERSION(X, Y, 0)
> > * g_foo(wibble)
> > * #else
> > * g_something_equivalent_in_older_glib(wibble);
> > * #endif
> > * }
> > *
> > * The #pragma at the top of this file turns off -Wdeprecated-declarations,
> > * ensuring this wrapper function impl doesn't trigger the compiler warning
> > * about using too new glib APIs. Finally we can do
> > *
> > * #define g_foo(a) g_foo_qemu(a)
> > *
> > * So now the code elsewhere in QEMU, which *does* have the
> > * -Wdeprecated-declarations warning active, can call g_foo(...) as normal,
> > * without generating warnings.
> > */
> >
> > which is how g_unix_get_passwd_entry_qemu() is implemented.
>
> Yet later we have qemu_g_test_slow following the style guide. Also I'm
> confused by the usage of g_unix_get_passwd_entry_qemu because the only
> place I see it in qga/commands-posix-ssh.c right before it does:
>
> #define g_unix_get_passwd_entry_qemu(username, err) \
> test_get_passwd_entry(username, err)
The g_unix_get_passwd_entry method is a bad example as it is
not following the guide for transparent replacement.
>
> although I think that only hold when the file is built with
> QGA_BUILD_UNIT_TEST.
>
> > Should we reword the documentation first?
>
> The original wording in glib-compat.h was added by Daniel in 2018 but
> the commit that added the password function comments:
>
> Since the fallback version is still unsafe, I would rather keep the
> _qemu postfix, to make sure it's not being misused by mistake. When/if
> necessary, we can implement a safer fallback and drop the _qemu suffix.
>
> So if we are going to make a distinction between a qemu prefix and
> suffix we should agree that and add it to the style document.
The glibcompat.h header should only be used for cases where
we are doing transparent replacement such that callers continue
using the normal glib API name, and the suffix is a hidden
impl detail only seen in the header.
I think in that particular case we should have just had
'qemu_unix_get_passwd_entry' defined in a completely separate
place like osdep.c.
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 :|
Le 03/09/2021 à 19:44, Philippe Mathieu-Daudé a écrit :
> 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 <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> 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
>
Applied to my trivial-patches branch.
Thanks,
Laurent
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com> > --- > qapi/qapi-clone-visitor.c | 16 ++++++++-------- > qapi/qapi-visit-core.c | 6 ++++-- > 2 files changed, 12 insertions(+), 10 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> > > 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
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é <philmd@redhat.com>
---
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(&d, sizeof(d));
+ TLBFlushRangeData *p = g_memdup2(&d, 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(&d, sizeof(d));
+ TLBFlushRangeData *p = g_memdup2(&d, 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(&d, sizeof(d));
+ p = g_memdup2(&d, sizeof(d));
async_run_on_cpu(dst_cpu, tlb_flush_range_by_mmuidx_async_1,
RUN_ON_CPU_HOST_PTR(p));
}
}
- p = g_memdup(&d, sizeof(d));
+ p = g_memdup2(&d, 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
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é <philmd@redhat.com>
---
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(&bm->table, sizeof(bm->table));
+ tb = g_memdup2(&bm->table, sizeof(bm->table));
bm->table.offset = 0;
bm->table.size = 0;
QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
--
2.31.1
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é <philmd@redhat.com> > --- > 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(&bm->table, sizeof(bm->table)); > + tb = g_memdup2(&bm->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 <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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é <philmd@redhat.com> >> --- >> 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(&bm->table, sizeof(bm->table)); >> + tb = g_memdup2(&bm->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 <eblake@redhat.com> Thanks!
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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(&node, sizeof(void *));
+ target->data = g_memdup2(&node, 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
On Freitag, 3. September 2021 19:44:49 CEST Philippe Mathieu-Daudé wrote:
> Per
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-n
> ow/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é <philmd@redhat.com>
> ---
> 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(&node, sizeof(void *));
> + target->data = g_memdup2(&node, sizeof(void *));
> target->size = sizeof(void *);
> return 0;
> }
That's Ok, trivial change.
> 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,
src->size is actually just 16 bit (fsdev/file-op-9p.h):
struct V9fsPath {
uint16_t size;
char *data;
};
Should (still) be Ok as well as V9fsPath is about file system pathes which are
currently limited to 4k (PATH_MAX).
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Best regards,
Christian Schoenebeck
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é <philmd@redhat.com>
---
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, &error_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
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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(&machine->possible_cpus->cpus[i].props,
- sizeof(*cpu_item->props));
+ cpu_item->props = g_memdup2(&machine->possible_cpus->cpus[i].props,
+ sizeof(*cpu_item->props));
cpu = machine->possible_cpus->cpus[i].cpu;
if (cpu) {
--
2.31.1
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é <philmd@redhat.com>
---
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(&val, sizeof(val)), sizeof(val));
+ g_memdup2(&val, sizeof(val)), sizeof(val));
val = cpu_to_le64(HPPA_TLB_ENTRIES);
fw_cfg_add_file(fw_cfg, "/etc/cpu/tlb_entries",
- g_memdup(&val, sizeof(val)), sizeof(val));
+ g_memdup2(&val, sizeof(val)), sizeof(val));
val = cpu_to_le64(HPPA_BTLB_ENTRIES);
fw_cfg_add_file(fw_cfg, "/etc/cpu/btlb_entries",
- g_memdup(&val, sizeof(val)), sizeof(val));
+ g_memdup2(&val, sizeof(val)), sizeof(val));
val = cpu_to_le64(HPA_POWER_BUTTON);
fw_cfg_add_file(fw_cfg, "/etc/power-button-addr",
- g_memdup(&val, sizeof(val)), sizeof(val));
+ g_memdup2(&val, 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
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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(&vmstate_eepro100, sizeof(vmstate_eepro100));
+ s->vmstate = g_memdup2(&vmstate_eepro100, sizeof(vmstate_eepro100));
s->vmstate->name = qemu_get_queue(s->nic)->model;
vmstate_register(VMSTATE_IF(&pci_dev->qdev), VMSTATE_INSTANCE_ID_ANY,
s->vmstate, s);
--
2.31.1
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é <philmd@redhat.com>
---
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(&bst_le16, sizeof bst_le16), sizeof bst_le16);
+ g_memdup2(&bst_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(&rt_le32, 4), 4);
+ fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup2(&rt_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
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é <philmd@redhat.com>
---
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(&reply, sizeof(MPIMsgSCSITaskMgmtReply));
+ reply_async = g_memdup2(&reply,
+ 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(&reply, sizeof(MPIMsgSCSITaskMgmtReply));
+ reply_async = g_memdup2(&reply, sizeof(MPIMsgSCSITaskMgmtReply));
reply_async->IOCLogInfo = INT_MAX;
count = 0;
--
2.31.1
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é <philmd@redhat.com>
---
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(&sphb->msi_devs[i].key,
- sizeof(sphb->msi_devs[i].key));
- value = g_memdup(&sphb->msi_devs[i].value,
- sizeof(sphb->msi_devs[i].value));
+ key = g_memdup2(&sphb->msi_devs[i].key, sizeof(sphb->msi_devs[i].key));
+ value = g_memdup2(&sphb->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
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é <philmd@redhat.com>
Acked-by: David Gibson <david@gibson.dropber.id.au>
> ---
> 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(&sphb->msi_devs[i].key,
> - sizeof(sphb->msi_devs[i].key));
> - value = g_memdup(&sphb->msi_devs[i].value,
> - sizeof(sphb->msi_devs[i].value));
> + key = g_memdup2(&sphb->msi_devs[i].key, sizeof(sphb->msi_devs[i].key));
> + value = g_memdup2(&sphb->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
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é <philmd@redhat.com>
---
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(&list->lock);
- g_queue_push_tail(list->list, g_memdup(&value, sizeof(value)));
+ g_queue_push_tail(list->list, g_memdup2(&value, sizeof(value)));
qemu_mutex_unlock(&list->lock);
}
--
2.31.1
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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, &ctrl, sizeof(ctrl));
iov_discard_front(&iov, &iov_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, &req, sizeof(req))
--
2.31.1
On Fri, Sep 3, 2021 at 8:11 PM Philippe Mathieu-Daudé <philmd@redhat.com> 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é <philmd@redhat.com>
> ---
> Should we check in_num/out_num in range?
I'd say it is not needed to check: virtqueue_pop fills them by
iterating through the descriptor chain so the range is restricted to
[0, 1024].
Acked-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> 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, &ctrl, sizeof(ctrl));
> iov_discard_front(&iov, &iov_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, &req, sizeof(req))
> --
> 2.31.1
>
>
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com>
---
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(&policy, 1), check_set_count, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_set_limit, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_oneshot, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_periodic, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_on_the_fly_mode_change, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_on_the_fly_period_change, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_on_the_fly_freq_change, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_run_with_period_0, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_run_with_delta_0, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_periodic_with_load_0, g_free);
+ g_memdup2(&policy, 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(&policy, 1), check_oneshot_with_load_0, g_free);
+ g_memdup2(&policy, 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(&iov, &iov_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(&iov_tmp, &iov_cnt_tmp, 0, &undo);
@@ -361,7 +361,7 @@ static void test_discard_front_undo(void)
/* Discard more bytes than vector size */
iov_random(&iov, &iov_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;
size = iov_size(iov, iov_cnt);
@@ -373,7 +373,7 @@ static void test_discard_front_undo(void)
/* Discard entire vector */
iov_random(&iov, &iov_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;
size = iov_size(iov, iov_cnt);
@@ -385,7 +385,7 @@ static void test_discard_front_undo(void)
/* Discard within first element */
iov_random(&iov, &iov_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;
size = g_test_rand_int_range(1, iov->iov_len);
@@ -397,7 +397,7 @@ static void test_discard_front_undo(void)
/* Discard entire first element */
iov_random(&iov, &iov_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(&iov_tmp, &iov_cnt_tmp, iov->iov_len, &undo);
@@ -408,7 +408,7 @@ static void test_discard_front_undo(void)
/* Discard within second element */
iov_random(&iov, &iov_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;
size = iov->iov_len + g_test_rand_int_range(1, iov[1].iov_len);
@@ -499,7 +499,7 @@ static void test_discard_back_undo(void)
/* Discard zero bytes */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
iov_discard_back_undoable(iov, &iov_cnt_tmp, 0, &undo);
iov_discard_undo(&undo);
@@ -509,7 +509,7 @@ static void test_discard_back_undo(void)
/* Discard more bytes than vector size */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
size = iov_size(iov, iov_cnt);
iov_discard_back_undoable(iov, &iov_cnt_tmp, size + 1, &undo);
@@ -520,7 +520,7 @@ static void test_discard_back_undo(void)
/* Discard entire vector */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
size = iov_size(iov, iov_cnt);
iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
@@ -531,7 +531,7 @@ static void test_discard_back_undo(void)
/* Discard within last element */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
size = g_test_rand_int_range(1, iov[iov_cnt - 1].iov_len);
iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
@@ -542,7 +542,7 @@ static void test_discard_back_undo(void)
/* Discard entire last element */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
size = iov[iov_cnt - 1].iov_len;
iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
@@ -553,7 +553,7 @@ static void test_discard_back_undo(void)
/* Discard within second-to-last element */
iov_random(&iov, &iov_cnt);
- iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+ iov_orig = g_memdup2(iov, sizeof(iov[0]) * iov_cnt);
iov_cnt_tmp = iov_cnt;
size = iov[iov_cnt - 1].iov_len +
g_test_rand_int_range(1, iov[iov_cnt - 2].iov_len);
--
2.31.1
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é <philmd@redhat.com>
---
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 ? &default_opts : opts_in),
- sizeof(AHCIOpts));
+ opts = g_memdup2((opts_in == NULL ? &default_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
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é <philmd@redhat.com>
> ---
> 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 ? &default_opts : opts_in),
> - sizeof(AHCIOpts));
> + opts = g_memdup2((opts_in == NULL ? &default_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 <thuth@redhat.com>
Le 03/09/2021 à 19:45, Philippe Mathieu-Daudé a écrit :
> 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é <philmd@redhat.com>
> ---
> 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 ? &default_opts : opts_in),
> - sizeof(AHCIOpts));
> + opts = g_memdup2((opts_in == NULL ? &default_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;
>
Applied to my trivial-patches branch.
Thanks,
Laurent
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é <philmd@redhat.com>
---
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(&a->new_key, sizeof(uint32_t));
+ ARMCPRegInfo *new_reg = g_memdup2(src_reg, sizeof(ARMCPRegInfo));
+ uint32_t *new_key = g_memdup2(&a->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
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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
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é <philmd@redhat.com>
---
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(&divrec, sizeof(divrec)));
+ g_memdup2(&divrec, 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(&gid_ifid,
- sizeof(gid_ifid)), g_memdup(&fd, sizeof(fd)));
+ g_hash_table_insert(server.umad_agent.gid2fd, g_memdup2(&gid_ifid,
+ sizeof(gid_ifid)), g_memdup2(&fd, sizeof(fd)));
pthread_rwlock_unlock(&server.lock);
@@ -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(&gid_ifid,
+ g_hash_table_remove(server.umad_agent.gid2fd, g_memdup2(&gid_ifid,
sizeof(gid_ifid)));
pthread_rwlock_unlock(&server.lock);
@@ -267,8 +267,8 @@ static void hash_tbl_save_fd_comm_id_pair(int fd, uint32_t comm_id,
pthread_rwlock_wrlock(&server.lock);
g_hash_table_insert(server.umad_agent.commid2fd,
- g_memdup(&comm_id, sizeof(comm_id)),
- g_memdup(&fde, sizeof(fde)));
+ g_memdup2(&comm_id, sizeof(comm_id)),
+ g_memdup2(&fde, sizeof(fde)));
pthread_rwlock_unlock(&server.lock);
}
--
2.31.1
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é <philmd@redhat.com>
---
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
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é <philmd@redhat.com> > --- > scripts/checkpatch.pl | 5 +++++ > 1 file changed, 5 insertions(+) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Le 03/09/2021 à 19:45, Philippe Mathieu-Daudé a écrit :
> 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é <philmd@redhat.com>
> ---
> 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);
>
Applied to my trivial-patches branch.
Thanks,
Laurent
© 2016 - 2024 Red Hat, Inc.