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

[Tim Deegan <tim@xxxxxxx>]

At 10:52 +0000 on 14 Jan (1358160739), Jan Beulich wrote:
> >>> On 14.01.13 at 11:23, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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.

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