From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 18/22] libxc: Add range checking to xc_dom_binloader Date: Mon, 10 Jun 2013 22:37:55 +0100 Message-ID: <51B64733.8010909@citrix.com> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-19-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370629642-6990-19-git-send-email-ian.jackson@eu.citrix.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: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org On 07/06/13 19:27, Ian Jackson wrote: > 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 > > v6: Add a missing `return -EINVAL' (Matthew Daley). > Fix an error in the commit message (Matthew Daley). > > v5: This patch is new in this version of the series. > --- > tools/libxc/xc_dom_binloader.c | 13 ++++++++++++- > 1 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c > index d2de04c..7eaf071 100644 > --- a/tools/libxc/xc_dom_binloader.c > +++ b/tools/libxc/xc_dom_binloader.c > @@ -123,9 +123,12 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) > uint32_t *probe_ptr; > uint32_t *probe_end; > > + if ( dom->kernel_size < sizeof(*table) ) > + return NULL; > 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; More void pointer arithmetic. If I am reading the above correctly, it appears to be a glorified probe_end = dom->kernel_blob + (uintptr_t)min(dom->kernel_size - sizeof(*table), 8192); which looks to be a more simple representation? ~Andrew > > for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) > @@ -282,6 +285,14 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) > return -EINVAL; > } > > + if ( image_size < skip || > + image_size - skip < text_size ) > + { > + DOMPRINTF("%s: image is too small for declared text size", > + __FUNCTION__); > + return -EINVAL; > + } > + > memcpy(dest, image + skip, text_size); > memset(dest + text_size, 0, bss_size); >