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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.



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