From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader Date: Fri, 14 Jun 2013 16:29:06 +0100 Message-ID: <51BB36C2.4000709@eu.citrix.com> References: <1371147302-20767-1-git-send-email-ian.jackson@eu.citrix.com> <1371147302-20767-19-git-send-email-ian.jackson@eu.citrix.com> <20923.12455.79753.831636@mariner.uk.xensource.com> <51BB321B.80800@eu.citrix.com> <20923.13137.527678.760500@mariner.uk.xensource.com> <51BB3468.6060004@eu.citrix.com> <20923.13768.315739.369184@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20923.13768.315739.369184@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Andrew Cooper , "xen-devel@lists.xensource.com" , mattjd@gmail.com, security@xen.org List-Id: xen-devel@lists.xenproject.org On 14/06/13 16:24, Ian Jackson wrote: > George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"): >> I think disturbing it in a way that is obviously correct is better than >> disturbing it in a way that looks redundant. > That's an argument. > > How about this then ? > (diff against previous tip followed by revised patch) > > Ian. > > commit 202da02f40b1806138419002c3e29dc8ce0e88c2 > Author: Ian Jackson > Date: Fri Jun 14 16:23:44 2013 +0100 > > Use clearer code for calculating probe_end > > diff --git a/.topmsg b/.topmsg > index c5cff24..1e1b932 100644 > --- a/.topmsg > +++ b/.topmsg > @@ -25,6 +25,8 @@ This is part of the fix to a security issue, XSA-55. > Signed-off-by: Ian Jackson > Reviewed-by: Andrew Cooper > > +v9: Use clearer code for calculating probe_end in find_table. > + > v6: Add a missing `return -EINVAL' (Matthew Daley). > Fix an error in the commit message (Matthew Daley). > > diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c > index 7eaf071..6469a65 100644 > --- a/tools/libxc/xc_dom_binloader.c > +++ b/tools/libxc/xc_dom_binloader.c > @@ -126,10 +126,10 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) > if ( dom->kernel_size < sizeof(*table) ) > return NULL; > probe_ptr = dom->kernel_blob; > - probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > - if ( dom->kernel_size >= 8192 && > - (void*)probe_end > (dom->kernel_blob + 8192) ) > + if ( dom->kernel_size > (8192 + sizeof(*table)) ) > probe_end = dom->kernel_blob + 8192; > + else > + probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > > for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) > { > > > > From: Ian Jackson > Subject: [PATCH] libxc: Add range checking to xc_dom_binloader > > This is a simple binary image loader with its own metadata format. > However, it is too careless with image-supplied values. > > Add the following checks: > > * That the image is bigger than the metadata table; otherwise the > pointer arithmetic to calculate the metadata table location may > yield undefined and dangerous values. > > * When clamping the end of the region to search, that we do not > calculate pointers beyond the end of the image. The C > specification does not permit this and compilers are becoming ever > more determined to miscompile code when they can "prove" various > falsehoods based on assertions from the C spec. > > * That the supplied image is big enough for the text we are allegedly > copying from it. Otherwise we might have a read overrun and copy > the results (perhaps a lot of secret data) into the guest. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson > Reviewed-by: Andrew Cooper This looks good -- thanks: Reviewed-by: George Dunlap