[Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Olaf Hering]

g_malloc0_n is available since glib-2.24. To allow build with older glib
versions use the generic g_malloc0, which is already used in many other
places in the code.

Fixes commit 3284fad728 ("xen-disk: add support for multi-page shared rings")

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_disk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index d42ed7070d..71deec17b0 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1232,7 +1232,7 @@ static int blk_connect(struct XenDevice *xendev)
         return -1;
     }
 
-    domids = g_malloc0_n(blkdev->nr_ring_ref, sizeof(uint32_t));
+    domids = g_malloc0(blkdev->nr_ring_ref * sizeof(uint32_t));
     for (i = 0; i < blkdev->nr_ring_ref; i++) {
         domids[i] = blkdev->xendev.dom;
     }

Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Eric Blake]

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

On 07/28/2017 07:31 AM, Olaf Hering wrote:
> g_malloc0_n is available since glib-2.24. To allow build with older glib
> versions use the generic g_malloc0, which is already used in many other
> places in the code.
> 
> Fixes commit 3284fad728 ("xen-disk: add support for multi-page shared rings")
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  hw/block/xen_disk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index d42ed7070d..71deec17b0 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -1232,7 +1232,7 @@ static int blk_connect(struct XenDevice *xendev)
>          return -1;
>      }
>  
> -    domids = g_malloc0_n(blkdev->nr_ring_ref, sizeof(uint32_t));
> +    domids = g_malloc0(blkdev->nr_ring_ref * sizeof(uint32_t));

This version is prone to multiplication overflow (well, maybe not, but
you have to audit for that).  Wouldn't it be better to use:

domids = g_new0(blkdev->nr_ring_ref, uint32_t)

which preserves the safety of g_malloc0_n?


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Olaf Hering]

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

On Fri, Jul 28, Eric Blake wrote:

> This version is prone to multiplication overflow (well, maybe not, but
> you have to audit for that).  Wouldn't it be better to use:

What could go wrong?
qemu will die either way, I think.

Olaf

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

Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Daniel P. Berrange]

On Fri, Jul 28, 2017 at 07:43:59AM -0500, Eric Blake wrote:
> On 07/28/2017 07:31 AM, Olaf Hering wrote:
> > g_malloc0_n is available since glib-2.24. To allow build with older glib
> > versions use the generic g_malloc0, which is already used in many other
> > places in the code.
> > 
> > Fixes commit 3284fad728 ("xen-disk: add support for multi-page shared rings")
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > ---
> >  hw/block/xen_disk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index d42ed7070d..71deec17b0 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -1232,7 +1232,7 @@ static int blk_connect(struct XenDevice *xendev)
> >          return -1;
> >      }
> >  
> > -    domids = g_malloc0_n(blkdev->nr_ring_ref, sizeof(uint32_t));
> > +    domids = g_malloc0(blkdev->nr_ring_ref * sizeof(uint32_t));
> 
> This version is prone to multiplication overflow (well, maybe not, but
> you have to audit for that).  Wouldn't it be better to use:
> 
> domids = g_new0(blkdev->nr_ring_ref, uint32_t)

You mean   g_new0(uint32_t, blkdev->nr_ring_ref)  but yeah, g_new0 is
better than g_malloc0 pretty much every time.

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 :|

Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Eric Blake]

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

On 07/28/2017 07:48 AM, Olaf Hering wrote:
> On Fri, Jul 28, Eric Blake wrote:
> 
>> This version is prone to multiplication overflow (well, maybe not, but
>> you have to audit for that).  Wouldn't it be better to use:
> 
> What could go wrong?
> qemu will die either way, I think.

Dying immediately due to provable multiplication overflow is MUCH better
than successfully allocating too-little and then having who-knows-what
go wrong down the road because you didn't check for overflow.  The
latter can sometimes be exploited into CVEs.  And maybe you can't
overflow, but having to do a non-local audit to prove that is more time
spent than just using the right interface from the get-go.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build

[Markus Armbruster]

Olaf Hering <olaf@aepfle.de> writes:

> On Fri, Jul 28, Eric Blake wrote:
>
>> This version is prone to multiplication overflow (well, maybe not, but
>> you have to audit for that).  Wouldn't it be better to use:
>
> What could go wrong?
> qemu will die either way, I think.

An overflow in the size argument of malloc(), realloc(), etc. is a heap
overrun waiting to happen.