Matt Wilson
2013-Jan-12 15:35 UTC
Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs
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;
Ian Campbell
2013-Jan-14 10:23 UTC
Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs
On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote:> > 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?It certainly does seem wrong (or too clever for me). I think either could correctly be removed but the more logical one would be the one in the for loop, I think, since the one inside the body is more accurate (although it only matters for the final iteration and either would cause the loop to exit). Ian.
Jan Beulich
2013-Jan-14 10:52 UTC
Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs
>>> On 14.01.13 at 11:23, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote: >> > 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? > > It certainly does seem wrong (or too clever for me). > > I think either could correctly be removed but the more logical one would > be the one in the for loop, I think, since the one inside the body is > more accurate (although it only matters for the final iteration and > either would cause the loop to exit).I agree, but since it was Tim who had put this on-off together (as a smaller replacement to the fix that went into 4.2 before its release), I''d leave this to him. Jan
Tim Deegan
2013-Jan-14 12:19 UTC
Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs
At 10:52 +0000 on 14 Jan (1358160739), Jan Beulich wrote:> >>> On 14.01.13 at 11:23, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote: > >> > 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? > > > > It certainly does seem wrong (or too clever for me). > > > > I think either could correctly be removed but the more logical one would > > be the one in the for loop, I think, since the one inside the body is > > more accurate (although it only matters for the final iteration and > > either would cause the loop to exit). > > I agree, but since it was Tim who had put this on-off together > (as a smaller replacement to the fix that went into 4.2 before > its release), I''d leave this to him.Yeah, the increment in the loop header shouldn''t be there. I can''t get to xenbits for an up-to-date tree right now but I''ll make a patch once I can. Tim.
Tim Deegan
2013-Jan-17 11:30 UTC
[PATCH 4.1-testing] x86/mm: Fix loop increment in paging_log_dirty_range()
# HG changeset patch
# User Tim Deegan <tim@xen.org>
# Date 1358421452 0
# Node ID 04368044ca5fb9800bfdacf14e883d39cad5c8a6
# Parent 8fe0e86c2ac27e22121aa9c70ddf5eacbb3051d0
x86/mm: Fix loop increment in paging_log_dirty_range()
In 23417:53ef1f35a0f8 (the fix for XSA-27 / CVE-2012-5511), the
loop variable gets incremented twice, so the loop only clears every
second page of the bitmap. This might cause the tools to think that
pages are dirty when they are not.
Reported-by: Steven Noonan <snoonan@amazon.com>
Reported-by: Matt Wilson <msw@amazon.com>
Signed-off-by: Tim Deegan <tim@xen.org>
diff -r 8fe0e86c2ac2 -r 04368044ca5f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Wed Jan 16 14:15:12 2013 +0000
+++ b/xen/arch/x86/mm/paging.c Thu Jan 17 11:17:32 2013 +0000
@@ -534,7 +534,8 @@ int paging_log_dirty_range(struct domain
size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- for ( off = 0; !rv && off < size; off += sizeof zeroes )
+ off = 0;
+ while ( !rv && off < size )
{
int todo = min(size - off, (int) PAGE_SIZE);
if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
Ian Campbell
2013-Jan-17 11:39 UTC
Re: [PATCH 4.1-testing] x86/mm: Fix loop increment in paging_log_dirty_range()
On Thu, 2013-01-17 at 11:30 +0000, Tim Deegan wrote:> # HG changeset patch > # User Tim Deegan <tim@xen.org> > # Date 1358421452 0 > # Node ID 04368044ca5fb9800bfdacf14e883d39cad5c8a6 > # Parent 8fe0e86c2ac27e22121aa9c70ddf5eacbb3051d0 > x86/mm: Fix loop increment in paging_log_dirty_range() > > In 23417:53ef1f35a0f8 (the fix for XSA-27 / CVE-2012-5511), the > loop variable gets incremented twice, so the loop only clears every > second page of the bitmap. This might cause the tools to think that > pages are dirty when they are not. > > Reported-by: Steven Noonan <snoonan@amazon.com> > Reported-by: Matt Wilson <msw@amazon.com> > Signed-off-by: Tim Deegan <tim@xen.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>> diff -r 8fe0e86c2ac2 -r 04368044ca5f xen/arch/x86/mm/paging.c > --- a/xen/arch/x86/mm/paging.c Wed Jan 16 14:15:12 2013 +0000 > +++ b/xen/arch/x86/mm/paging.c Thu Jan 17 11:17:32 2013 +0000 > @@ -534,7 +534,8 @@ int paging_log_dirty_range(struct domain > > size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long); > rv = 0; > - for ( off = 0; !rv && off < size; off += sizeof zeroes ) > + off = 0; > + while ( !rv && off < size ) > { > int todo = min(size - off, (int) PAGE_SIZE); > if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) ) > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel