[libvirt] [PATCH] util: Fix crash of libvirtd when running numatune with invalid nodeset

[Peter Krempa]

On 08/16/13 09:47, Alex Jia wrote:
> This issue is introduced by commit 0fc8909, the virBitmapIsSet() needs caller
> to ensure 'b < bitmap->max_bit', but it's lost in the virBitmapParse() caller,
> this will cause crash of libvirtd, with the patch, libvirtd no crash and can
> get a expected error "Failed to parse nodeset".
> 
> How to reproduce?
> 
> # virsh numatune foo --nodeset 1000000000
> Actual result:
> error: Unable to change numa parameters
> error: End of file while reading data: Input/output error
> error: One or more references were leaked after disconnect from the hypervisor
> error: Failed to reconnect to the hypervisor
> 
> GDB backtrace:
> 

....

> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=997367
> 
> Signed-off-by: Alex Jia <ajia at redhat.com>
> ---
> The caller virBitmapGetBit() can make sure 'b < bitmap->max_bit', so don't
> need to worry about higher caller for the virBitmapGetBit(), but the
> virBitmapParse() is called by many XML parser function, not sure which one
> can crash libvirtd with read-only client then probably require a CVE, I haven't
> a good way to check them now and only manually check them one by one.
> 
>  src/util/virbitmap.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index 7e1cd02..edbfb30 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -337,6 +337,9 @@ virBitmapParse(const char *str,
>          if (start < 0)
>              goto parse_error;
>  
> +        if ((*bitmap)->max_bit <= start)
> +            goto parse_error;
> +

This will check only the start value of a range. When you use a range
that starts with a valid number "1-1000000000000" for example, then the
loop that is setting bits from the range will triger the crash too.

IMO the correct fix here is to get rid of the weird construction:

if (!virBitmapIsSet(*bitmap, i)) {
    ignore_value(virBitmapSetBit(*bitmap, i));
    ret++;
}

with

if (virBitmapSetBit(*bitmap, i) < 0)
    goto error;

and at the end count the enabled bits as
ret = virBitmapCountBits(*bitmap);

instead of the code that is present.

I'll post an updated version of this patch containing the suggested changes.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130816/a9ef8993/attachment-0001.sig>