Re: [Xen-devel] Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Matt Wilson <msw@xxxxxxxxxx>]

On Mon, Dec 03, 2012 at 05:51:44PM +0000, Xen.org security team wrote:
[...]
> ISSUE DESCRIPTION
> =================
> 
> Several HVM control operations do not check the size of their inputs
> and can tie up a physical CPU for extended periods of time.
> 
> In addition dirty video RAM tracking involves clearing the bitmap
> provided by the domain controlling the guest (e.g. dom0 or a
> stubdom). If the size of that bitmap is overly large, an intermediate
> variable on the hypervisor stack may overflow that stack.

Part of the xsa27-4.1.patch, quoted below, might have a bug.

> x86/paging: Don't allocate user-controlled amounts of stack memory.
> 
> This is XSA-27 / CVE-2012-5511.
> 
> Signed-off-by: Tim Deegan <tim@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> v2: Provide definition of GB to fix x86-32 compile.
> 
> Signed-off-by: Jan Beulich <JBeulich@xxxxxxxx>
> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
[...]
> diff -r 5639047d6c9f xen/arch/x86/mm/paging.c
> --- a/xen/arch/x86/mm/paging.c        Mon Nov 19 09:43:48 2012 +0100
> +++ b/xen/arch/x86/mm/paging.c        Mon Nov 19 16:00:33 2012 +0000
> @@ -529,13 +529,18 @@ int paging_log_dirty_range(struct domain
>  
>      if ( !d->arch.paging.log_dirty.fault_count &&
>           !d->arch.paging.log_dirty.dirty_count ) {
> -        int size = (nr + BITS_PER_LONG - 1) / BITS_PER_LONG;
> -        unsigned long zeroes[size];
> -        memset(zeroes, 0x00, size * BYTES_PER_LONG);
> +        static uint8_t zeroes[PAGE_SIZE];
> +        int off, size;
> +
> +        size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
>          rv = 0;
> -        if ( copy_to_guest_offset(dirty_bitmap, 0, (uint8_t *) zeroes,
> -                                  size * BYTES_PER_LONG) != 0 )
> -            rv = -EFAULT;
> +        for ( off = 0; !rv && off < size; off += sizeof(zeroes) )
> +        {
> +            int todo = min(size - off, (int) PAGE_SIZE);
> +            if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
> +                rv = -EFAULT;
> +            off += todo;

off is incremented twice, once as part of the for loop and once
inside. Was that intended?

Credit to Steven Noonan for pointing this out.

Matt

> +        }
>          goto out;
>      }
>      d->arch.paging.log_dirty.fault_count = 0;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel