From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 12/22] libelf: Check pointer references in elf_is_elfbinary Date: Tue, 11 Jun 2013 00:04:45 +0100 Message-ID: <51B65B8D.9070009@citrix.com> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-13-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-13-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: > elf_is_elfbinary didn't take a length parameter and could potentially > access out of range when provided with a very short image. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson > Acked-by: Ian Campbell > Reviewed-by: Konrad Rzeszutek Wilk > > v2: Style fix. > Fix commit message subject. > --- > tools/libxc/xc_dom_elfloader.c | 2 +- > xen/arch/x86/bzimage.c | 4 ++-- > xen/common/libelf/libelf-loader.c | 2 +- > xen/common/libelf/libelf-tools.c | 9 ++++++--- > xen/include/xen/libelf.h | 2 +- > 5 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > index c038d1c..f14b053 100644 > --- a/tools/libxc/xc_dom_elfloader.c > +++ b/tools/libxc/xc_dom_elfloader.c > @@ -93,7 +93,7 @@ static int check_elf_kernel(struct xc_dom_image *dom, int verbose) > return -EINVAL; > } > > - if ( !elf_is_elfbinary(dom->kernel_blob) ) > + if ( !elf_is_elfbinary(dom->kernel_blob, dom->kernel_size) ) > { > if ( verbose ) > xc_dom_panic(dom->xch, > diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c > index c5519d8..58fda16 100644 > --- a/xen/arch/x86/bzimage.c > +++ b/xen/arch/x86/bzimage.c > @@ -220,7 +220,7 @@ unsigned long __init bzimage_headroom(char *image_start, > image_length = hdr->payload_length; > } > > - if ( elf_is_elfbinary(image_start) ) > + if ( elf_is_elfbinary(image_start, image_length) ) > return 0; > > orig_image_len = image_length; > @@ -251,7 +251,7 @@ int __init bzimage_parse(char *image_base, char **image_start, unsigned long *im > *image_len = hdr->payload_length; > } > > - if ( elf_is_elfbinary(*image_start) ) > + if ( elf_is_elfbinary(*image_start, *image_len) ) > return 0; > > BUG_ON(!(image_base < *image_start)); > diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c > index 878552e..6c43c34 100644 > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -29,7 +29,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size) > ELF_HANDLE_DECL(elf_shdr) shdr; > uint64_t i, count, section, offset; > > - if ( !elf_is_elfbinary(image_input) ) > + if ( !elf_is_elfbinary(image_input, size) ) > { > elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__); > return -1; > diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c > index 08ab027..b613593 100644 > --- a/xen/common/libelf/libelf-tools.c > +++ b/xen/common/libelf/libelf-tools.c > @@ -332,11 +332,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL( > > /* ------------------------------------------------------------------------ */ > > -int elf_is_elfbinary(const void *image) > +int elf_is_elfbinary(const void *image_start, size_t image_size) > { > - const Elf32_Ehdr *ehdr = image; > + const Elf32_Ehdr *ehdr = image_start; > > - return IS_ELF(*ehdr); /* fixme unchecked */ > + if ( image_size < sizeof(*ehdr) ) > + return 0; > + > + return IS_ELF(*ehdr); > } sizeof(Elf32_Ehdr) == 52 sizeof(Elf64_Ehdr) == 64 As stated by a trivial C program containing 'printf("32: %zu, 64: %zu\n", sizeof(Elf32_Ehdr), sizeof(Elf64_Ehdr));' Therefore, the sizeof check is insufficient if given a 64bit elf. It needs to check first against sizeof e_ident which is consistent between the two, look up EI_CLASS in the ident, then choose the appropriate structure. ~Andrew > > int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) > diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h > index 783f125..c54c90b 100644 > --- a/xen/include/xen/libelf.h > +++ b/xen/include/xen/libelf.h > @@ -349,7 +349,7 @@ uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), > unsigned int unitsz, unsigned int idx); > ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); > > -int elf_is_elfbinary(const void *image); > +int elf_is_elfbinary(const void *image_start, size_t image_size); > int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); > > /* ------------------------------------------------------------------------ */