From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matt Wilson <msw@amazon.com>
Subject: Re: Xen Security Advisory 27 (CVE-2012-5511) - several
 HVM operations do not validate the range of their inputs
Date: Sat, 12 Jan 2013 16:35:31 +0100
Message-ID: <20130112153524.GA15043@u109add4315675089e695.ant.amazon.com>
References: <E1TfaBE-00066j-ID@xenbits.xen.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <E1TfaBE-00066j-ID@xenbits.xen.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: "Xen.org security team" <security@xen.org>, xen-devel@lists.xen.org
Cc: Steven Noonan <snoonan@amazon.com>
List-Id: xen-devel@lists.xenproject.org

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@xen.org>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> v2: Provide definition of GB to fix x86-32 compile.
> 
> Signed-off-by: Jan Beulich <JBeulich@suse.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
[...]
> 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;