* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
@ 2017-07-28 12:43 ` Eric Blake
0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2017-07-28 12:43 UTC (permalink / raw)
To: Olaf Hering, Stefano Stabellini, Anthony Perard, Kevin Wolf,
Max Reitz, open list:X86, open list:Block layer core,
open list:All patches CC here
[-- Attachment #1.1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:43 ` Eric Blake
@ 2017-07-28 12:48 ` Olaf Hering
-1 siblings, 0 replies; 12+ messages in thread
From: Olaf Hering @ 2017-07-28 12:48 UTC (permalink / raw)
To: Eric Blake
Cc: Stefano Stabellini, Anthony Perard, Kevin Wolf, Max Reitz,
open list:X86, open list:Block layer core,
open list:All patches CC here
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
@ 2017-07-28 12:48 ` Olaf Hering
0 siblings, 0 replies; 12+ messages in thread
From: Olaf Hering @ 2017-07-28 12:48 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Stefano Stabellini, open list:Block layer core,
open list:All patches CC here, Max Reitz, Anthony Perard,
open list:X86
[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:48 ` Olaf Hering
(?)
@ 2017-07-28 13:00 ` Eric Blake
-1 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2017-07-28 13:00 UTC (permalink / raw)
To: Olaf Hering
Cc: Stefano Stabellini, Anthony Perard, Kevin Wolf, Max Reitz,
open list:X86, open list:Block layer core,
open list:All patches CC here
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:48 ` Olaf Hering
(?)
(?)
@ 2017-07-28 13:00 ` Eric Blake
-1 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2017-07-28 13:00 UTC (permalink / raw)
To: Olaf Hering
Cc: Kevin Wolf, Stefano Stabellini, open list:Block layer core,
open list:All patches CC here, Max Reitz, Anthony Perard,
open list:X86
[-- Attachment #1.1.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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:48 ` Olaf Hering
` (2 preceding siblings ...)
(?)
@ 2017-07-28 16:35 ` Markus Armbruster
-1 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2017-07-28 16:35 UTC (permalink / raw)
To: Olaf Hering
Cc: Kevin Wolf, Stefano Stabellini, open list:Block layer core,
open list:All patches CC here, Max Reitz, Anthony Perard,
open list:X86, Eric Blake
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:48 ` Olaf Hering
` (3 preceding siblings ...)
(?)
@ 2017-07-28 16:35 ` Markus Armbruster
-1 siblings, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2017-07-28 16:35 UTC (permalink / raw)
To: Olaf Hering
Cc: Eric Blake, Kevin Wolf, Stefano Stabellini,
open list:Block layer core, open list:All patches CC here,
Max Reitz, Anthony Perard, open list:X86
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:43 ` Eric Blake
(?)
(?)
@ 2017-07-28 12:52 ` Daniel P. Berrange
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrange @ 2017-07-28 12:52 UTC (permalink / raw)
To: Eric Blake
Cc: Olaf Hering, Stefano Stabellini, Anthony Perard, Kevin Wolf,
Max Reitz, open list:X86, open list:Block layer core,
open list:All patches CC here
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH] xen-disk: use g_malloc0 to fix build
2017-07-28 12:43 ` Eric Blake
` (2 preceding siblings ...)
(?)
@ 2017-07-28 12:52 ` Daniel P. Berrange
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrange @ 2017-07-28 12:52 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Olaf Hering, Stefano Stabellini,
open list:Block layer core, open list:All patches CC here,
Max Reitz, Anthony Perard, open list:X86
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 :|
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread