Re: [Xen-devel] Xen Security Advisory 55 - Multiple vulnerabilities in libelf PV kernel handling

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chuck Anderson writes ("Re: Xen Security Advisory 55 - Multiple 
vulnerabilities> >> * 907abe4 2013-06-07 | libxc: check blob size before 
proceeding...
> >>
> >> @@ -278,6 +278,10 @@ size_t xc_dom_check_gzip(xc_interface *xch, void
> >> *blob, siz
> >> +    if ( ziplen < 6 )
> >> +        /* too small */
> >> +        return 0;
> >>
> >> Add #define for MIN_ZIPLEN and include a comment on why it is 6.  Then:
> >> +    if ( MIN_ZIPLEN < 6 )
> >
> > The code below uses simple integers.  Your proposal would separate the
> > definition of the limit to be applied from the code which is to be
> > protected, so I think it's better the way it is.
> 
> I think it is better to use good programming practices with new code 
> even if existing code doesn't.  In any case I think a comment saying why 
> 6 is used would be a good.  That way a reviewer knows what your 
> assumptions are and can evaluate if it is an appropriate value.

I'll add a comment.

> >> **********
> >>
> >> * 1448048 2013-06-07 | libxc: range checks in xc_dom_p2m_host and...
> >>    OK
> >
> > Thanks.  Should I take this as a formal Reviewed-by and put your name
> > on the patch ?
> 
> Yes.  For all "OK", please add:
> Reviewed-by Chuck Anderson <chuck.anderson@xxxxxxxxxx>

Excellent, thanks.

> >> **********
> >>
> >> * e80dd92 2013-06-07 | libxc: Add range checking to xc_dom_binloader
> >>
> >> @@ -123,9 +123,12 @@ static struct xen_bin_image_table
> >> *find_table(struct xc_dom
> >>        probe_ptr = dom->kernel_blob;
> >>        probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
> >> -    if ( (void*)probe_end > (dom->kernel_blob + 8192) )
> >> +    if ( dom->kernel_size >= 8192 &&
> >> +         (void*)probe_end > (dom->kernel_blob + 8192) )
> >>            probe_end = dom->kernel_blob + 8192;
> >>
> >> probe_end is kernel_blob + dom->kernel_size - metadata table size
> >> You could simplify the code and make it clearer by changing:
> >>
> >> +    if ( dom->kernel_size >= 8192 &&
> >> +         (void*)probe_end > (dom->kernel_blob + 8192) )
> >>            probe_end = dom->kernel_blob + 8192;
> >>
> >> to:
> >>
> >> +    if ( dom->kernel_size > (8192 - sizeof(*table)) )
> >>            probe_end = dom->kernel_blob + 8192;
> >
> > Perhaps, but I think it's probably better to make a smaller change
> > here.  If we were to change it I think the change would be this:
> >
> >   -    probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
> >   +    if ( dom->kernel_size > (8192 - sizeof(*table)) )
> >            probe_end = dom->kernel_blob + 8192;
> >        else
> >   +        probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
> 
> Yep, that works.

I think at this stage of the development of the series, I would prefer
not to change it though.  Thanks for your comment though.

> >> * 8dfa66a 2013-06-07 | libxc: Fix range checking in xc_dom_pfn_to_ptr...
...
> >> Replace the last 6 instructions with:
> >> +    ptr = xc_dom_pfn_to_ptr_retcount(dom, page, 0, &safe_region_count);
> >> +    if ( ptr != NULL )
> >> +        *safe_region_out = (safe_region_count <<
> >> XC_DOM_PAGE_SHIFT(dom)) - offs
> >> et;
> >> +    else
> >> +        *safe_region_out = 0;
> >> +    return ptr;
> >
> > I'm not sure why ?
> 
> - I'm assuming "if ( ptr != NULL )" is the expected path.
>    Handling it first prevents the likely branch mis-prediction.
>    Not a big deal but good to do if practical as is the case here.
> 
> - "*safe_region_out = 0" will be overwritten in the expected code path
>    so do it only when needed.

I don't think this is such a hot path that we should be thinking about
these kinds of optimisations here.  Setting *safe_region_out=0 at the
start is a simple guard against any error exit paths, even though at
the moment there is only one such.

> >>       if ( pfn > dom->total_pages ||    /* multiple checks to avoid
> >> overflows */
> >>
> >> Isn't pfn zero-based?  If so, the test should be:
> >>
> >>       if ( pfn >= dom->total_pages ||    /* multiple checks to avoid
> >> overflows */
> >>
> >> I believe there are similar off-by-one errors in existing code if pfn is
> >> zero-based.
> >
> > If count is nonzero then this will be caught by the third inequality.
> > This code isn't touched by my patch series.
> 
> I was reviewing just the changes in the patch but that chunk of existing 
> code caught my eye.

I see.

> Normally a pfn is zero-based.  If you have N pfns they would be numbered 
> 0, 1, ..., N-1.

Yes.  However as I say if count is nonzero this works correctly I
think.

If count is zero then we are in the other case where we look up an
existing block, which has different length checks.  It says:
            if ( pfn >= phys->first + phys->count )
                continue;
which will mean that pfn must be < phys->first + phys->count ie it
must be strictly within the block.

So this code is currently fine.

Thanks,
Ian.

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