* [PATCH 01/21] libelf: abolish libelf-relocate.c
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 02/21] libxc: introduce xc_dom_seg_to_ptr_pages Ian Jackson
` (19 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
This file is not actually used. It's not built in Xen's instance of
libelf; in libxc's it's built but nothing in it is called. Do not
compile it in libxc, and delete it.
This reduces the amount of work we need to do in forthcoming patches
to libelf (particularly since as libelf-relocate.c is not used it is
probably full of bugs).
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxc/Makefile | 2 +-
xen/common/libelf/libelf-relocate.c | 372 -----------------------------------
2 files changed, 1 insertions(+), 373 deletions(-)
delete mode 100644 xen/common/libelf/libelf-relocate.c
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 9942c3a..6f5c307 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -50,7 +50,7 @@ vpath %.c ../../xen/common/libelf
CFLAGS += -I../../xen/common/libelf
GUEST_SRCS-y += libelf-tools.c libelf-loader.c
-GUEST_SRCS-y += libelf-dominfo.c libelf-relocate.c
+GUEST_SRCS-y += libelf-dominfo.c
# new domain builder
GUEST_SRCS-y += xc_dom_core.c xc_dom_boot.c
diff --git a/xen/common/libelf/libelf-relocate.c b/xen/common/libelf/libelf-relocate.c
deleted file mode 100644
index 7ef4b01..0000000
--- a/xen/common/libelf/libelf-relocate.c
+++ /dev/null
@@ -1,372 +0,0 @@
-/*
- * ELF relocation code (not used by xen kernel right now).
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation;
- * version 2.1 of the License.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#include "libelf-private.h"
-
-/* ------------------------------------------------------------------------ */
-
-static const char *rel_names_i386[] = {
- "R_386_NONE",
- "R_386_32",
- "R_386_PC32",
- "R_386_GOT32",
- "R_386_PLT32",
- "R_386_COPY",
- "R_386_GLOB_DAT",
- "R_386_JMP_SLOT",
- "R_386_RELATIVE",
- "R_386_GOTOFF",
- "R_386_GOTPC",
- "R_386_32PLT",
- "R_386_TLS_TPOFF",
- "R_386_TLS_IE",
- "R_386_TLS_GOTIE",
- "R_386_TLS_LE",
- "R_386_TLS_GD",
- "R_386_TLS_LDM",
- "R_386_16",
- "R_386_PC16",
- "R_386_8",
- "R_386_PC8",
- "R_386_TLS_GD_32",
- "R_386_TLS_GD_PUSH",
- "R_386_TLS_GD_CALL",
- "R_386_TLS_GD_POP",
- "R_386_TLS_LDM_32",
- "R_386_TLS_LDM_PUSH",
- "R_386_TLS_LDM_CALL",
- "R_386_TLS_LDM_POP",
- "R_386_TLS_LDO_32",
- "R_386_TLS_IE_32",
- "R_386_TLS_LE_32",
- "R_386_TLS_DTPMOD32",
- "R_386_TLS_DTPOFF32",
- "R_386_TLS_TPOFF32",
-};
-
-static int elf_reloc_i386(struct elf_binary *elf, int type,
- uint64_t addr, uint64_t value)
-{
- void *ptr = elf_get_ptr(elf, addr);
- uint32_t *u32;
-
- switch ( type )
- {
- case 1 /* R_386_32 */ :
- u32 = ptr;
- *u32 += elf->reloc_offset;
- break;
- case 2 /* R_386_PC32 */ :
- /* nothing */
- break;
- default:
- return -1;
- }
- return 0;
-}
-
-/* ------------------------------------------------------------------------ */
-
-static const char *rel_names_x86_64[] = {
- "R_X86_64_NONE",
- "R_X86_64_64",
- "R_X86_64_PC32",
- "R_X86_64_GOT32",
- "R_X86_64_PLT32",
- "R_X86_64_COPY",
- "R_X86_64_GLOB_DAT",
- "R_X86_64_JUMP_SLOT",
- "R_X86_64_RELATIVE",
- "R_X86_64_GOTPCREL",
- "R_X86_64_32",
- "R_X86_64_32S",
- "R_X86_64_16",
- "R_X86_64_PC16",
- "R_X86_64_8",
- "R_X86_64_PC8",
- "R_X86_64_DTPMOD64",
- "R_X86_64_DTPOFF64",
- "R_X86_64_TPOFF64",
- "R_X86_64_TLSGD",
- "R_X86_64_TLSLD",
- "R_X86_64_DTPOFF32",
- "R_X86_64_GOTTPOFF",
- "R_X86_64_TPOFF32",
-};
-
-static int elf_reloc_x86_64(struct elf_binary *elf, int type,
- uint64_t addr, uint64_t value)
-{
- void *ptr = elf_get_ptr(elf, addr);
- uint64_t *u64;
- uint32_t *u32;
- int32_t *s32;
-
- switch ( type )
- {
- case 1 /* R_X86_64_64 */ :
- u64 = ptr;
- value += elf->reloc_offset;
- *u64 = value;
- break;
- case 2 /* R_X86_64_PC32 */ :
- u32 = ptr;
- *u32 = value - addr;
- if ( *u32 != (uint32_t)(value - addr) )
- {
- elf_err(elf, "R_X86_64_PC32 overflow: 0x%" PRIx32
- " != 0x%" PRIx32 "\n",
- *u32, (uint32_t) (value - addr));
- return -1;
- }
- break;
- case 10 /* R_X86_64_32 */ :
- u32 = ptr;
- value += elf->reloc_offset;
- *u32 = value;
- if ( *u32 != value )
- {
- elf_err(elf, "R_X86_64_32 overflow: 0x%" PRIx32
- " != 0x%" PRIx64 "\n",
- *u32, value);
- return -1;
- }
- break;
- case 11 /* R_X86_64_32S */ :
- s32 = ptr;
- value += elf->reloc_offset;
- *s32 = value;
- if ( *s32 != (int64_t) value )
- {
- elf_err(elf, "R_X86_64_32S overflow: 0x%" PRIx32
- " != 0x%" PRIx64 "\n",
- *s32, (int64_t) value);
- return -1;
- }
- break;
- default:
- return -1;
- }
- return 0;
-}
-
-/* ------------------------------------------------------------------------ */
-
-static struct relocs {
- const char **names;
- int count;
- int (*func) (struct elf_binary * elf, int type, uint64_t addr,
- uint64_t value);
-} relocs[] =
-/* *INDENT-OFF* */
-{
- [EM_386] = {
- .names = rel_names_i386,
- .count = sizeof(rel_names_i386) / sizeof(rel_names_i386[0]),
- .func = elf_reloc_i386,
- },
- [EM_X86_64] = {
- .names = rel_names_x86_64,
- .count = sizeof(rel_names_x86_64) / sizeof(rel_names_x86_64[0]),
- .func = elf_reloc_x86_64,
- }
-};
-/* *INDENT-ON* */
-
-/* ------------------------------------------------------------------------ */
-
-static const char *rela_name(int machine, int type)
-{
- if ( machine > sizeof(relocs) / sizeof(relocs[0]) )
- return "unknown mach";
- if ( !relocs[machine].names )
- return "unknown mach";
- if ( type > relocs[machine].count )
- return "unknown rela";
- return relocs[machine].names[type];
-}
-
-static int elf_reloc_section(struct elf_binary *elf,
- const elf_shdr * rels,
- const elf_shdr * sect, const elf_shdr * syms)
-{
- const void *ptr, *end;
- const elf_shdr *shdr;
- const elf_rela *rela;
- const elf_rel *rel;
- const elf_sym *sym;
- uint64_t s_type;
- uint64_t r_offset;
- uint64_t r_info;
- uint64_t r_addend;
- int r_type, r_sym;
- size_t rsize;
- uint64_t shndx, sbase, addr, value;
- const char *sname;
- int machine;
-
- machine = elf_uval(elf, elf->ehdr, e_machine);
- if ( (machine >= (sizeof(relocs) / sizeof(relocs[0]))) ||
- (relocs[machine].func == NULL) )
- {
- elf_err(elf, "%s: can't handle machine %d\n",
- __FUNCTION__, machine);
- return -1;
- }
- if ( elf_swap(elf) )
- {
- elf_err(elf, "%s: non-native byte order, relocation not supported\n",
- __FUNCTION__);
- return -1;
- }
-
- s_type = elf_uval(elf, rels, sh_type);
- rsize = (SHT_REL == s_type) ? elf_size(elf, rel) : elf_size(elf, rela);
- ptr = elf_section_start(elf, rels);
- end = elf_section_end(elf, rels);
-
- for ( ; ptr < end; ptr += rsize )
- {
- switch ( s_type )
- {
- case SHT_REL:
- rel = ptr;
- r_offset = elf_uval(elf, rel, r_offset);
- r_info = elf_uval(elf, rel, r_info);
- r_addend = 0;
- break;
- case SHT_RELA:
- rela = ptr;
- r_offset = elf_uval(elf, rela, r_offset);
- r_info = elf_uval(elf, rela, r_info);
- r_addend = elf_uval(elf, rela, r_addend);
- break;
- default:
- /* can't happen */
- return -1;
- }
- if ( elf_64bit(elf) )
- {
- r_type = ELF64_R_TYPE(r_info);
- r_sym = ELF64_R_SYM(r_info);
- }
- else
- {
- r_type = ELF32_R_TYPE(r_info);
- r_sym = ELF32_R_SYM(r_info);
- }
-
- sym = elf_sym_by_index(elf, r_sym);
- shndx = elf_uval(elf, sym, st_shndx);
- switch ( shndx )
- {
- case SHN_UNDEF:
- sname = "*UNDEF*";
- sbase = 0;
- break;
- case SHN_COMMON:
- elf_err(elf, "%s: invalid section: %" PRId64 "\n",
- __FUNCTION__, shndx);
- return -1;
- case SHN_ABS:
- sname = "*ABS*";
- sbase = 0;
- break;
- default:
- shdr = elf_shdr_by_index(elf, shndx);
- if ( shdr == NULL )
- {
- elf_err(elf, "%s: invalid section: %" PRId64 "\n",
- __FUNCTION__, shndx);
- return -1;
- }
- sname = elf_section_name(elf, shdr);
- sbase = elf_uval(elf, shdr, sh_addr);
- }
-
- addr = r_offset;
- value = elf_uval(elf, sym, st_value);
- value += r_addend;
-
- if ( elf->log_callback && (elf->verbose > 1) )
- {
- uint64_t st_name = elf_uval(elf, sym, st_name);
- const char *name = st_name ? elf->sym_strtab + st_name : "*NONE*";
-
- elf_msg(elf,
- "%s: type %s [%d], off 0x%" PRIx64 ", add 0x%" PRIx64 ","
- " sym %s [0x%" PRIx64 "], sec %s [0x%" PRIx64 "]"
- " -> addr 0x%" PRIx64 " value 0x%" PRIx64 "\n",
- __FUNCTION__, rela_name(machine, r_type), r_type, r_offset,
- r_addend, name, elf_uval(elf, sym, st_value), sname, sbase,
- addr, value);
- }
-
- if ( relocs[machine].func(elf, r_type, addr, value) == -1 )
- {
- elf_err(elf, "%s: unknown/unsupported reloc type %s [%d]\n",
- __FUNCTION__, rela_name(machine, r_type), r_type);
- return -1;
- }
- }
- return 0;
-}
-
-int elf_reloc(struct elf_binary *elf)
-{
- const elf_shdr *rels, *sect, *syms;
- uint64_t i, count, type;
-
- count = elf_shdr_count(elf);
- for ( i = 0; i < count; i++ )
- {
- rels = elf_shdr_by_index(elf, i);
- type = elf_uval(elf, rels, sh_type);
- if ( (type != SHT_REL) && (type != SHT_RELA) )
- continue;
-
- sect = elf_shdr_by_index(elf, elf_uval(elf, rels, sh_info));
- syms = elf_shdr_by_index(elf, elf_uval(elf, rels, sh_link));
- if ( NULL == sect || NULL == syms )
- continue;
-
- if ( !(elf_uval(elf, sect, sh_flags) & SHF_ALLOC) )
- {
- elf_msg(elf, "%s: relocations for %s, skipping\n",
- __FUNCTION__, elf_section_name(elf, sect));
- continue;
- }
-
- elf_msg(elf, "%s: relocations for %s @ 0x%" PRIx64 "\n",
- __FUNCTION__, elf_section_name(elf, sect),
- elf_uval(elf, sect, sh_addr));
- if ( elf_reloc_section(elf, rels, sect, syms) != 0 )
- return -1;
- }
- return 0;
-}
-
-/*
- * Local variables:
- * mode: C
- * c-set-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 02/21] libxc: introduce xc_dom_seg_to_ptr_pages
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
2013-06-12 16:00 ` [PATCH 01/21] libelf: abolish libelf-relocate.c Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 03/21] libxc: Fix range checking in xc_dom_pfn_to_ptr etc Ian Jackson
` (18 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Provide a version of xc_dom_seg_to_ptr which returns the number of
guest pages it has actually mapped. This is useful for callers who
want to do range checking; we will use this later in this series.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxc/xc_dom.h | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 7043f96..19a10f4 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -262,14 +262,27 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first,
void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn);
void xc_dom_unmap_all(struct xc_dom_image *dom);
-static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
- struct xc_dom_seg *seg)
+static inline void *xc_dom_seg_to_ptr_pages(struct xc_dom_image *dom,
+ struct xc_dom_seg *seg,
+ xen_pfn_t *pages_out)
{
xen_vaddr_t segsize = seg->vend - seg->vstart;
unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
xen_pfn_t pages = (segsize + page_size - 1) / page_size;
+ void *retval;
+
+ retval = xc_dom_pfn_to_ptr(dom, seg->pfn, pages);
+
+ *pages_out = retval ? pages : 0;
+ return retval;
+}
+
+static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
+ struct xc_dom_seg *seg)
+{
+ xen_pfn_t dummy;
- return xc_dom_pfn_to_ptr(dom, seg->pfn, pages);
+ return xc_dom_seg_to_ptr_pages(dom, seg, &dummy);
}
static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 03/21] libxc: Fix range checking in xc_dom_pfn_to_ptr etc.
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
2013-06-12 16:00 ` [PATCH 01/21] libelf: abolish libelf-relocate.c Ian Jackson
2013-06-12 16:00 ` [PATCH 02/21] libxc: introduce xc_dom_seg_to_ptr_pages Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 04/21] libelf: abolish elf_sval and elf_access_signed Ian Jackson
` (17 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
* Ensure that xc_dom_pfn_to_ptr (when called with count==0) does not
return a previously-allocated block which is entirely before the
requested pfn (!)
* Provide a version of xc_dom_pfn_to_ptr, xc_dom_pfn_to_ptr_retcount,
which provides the length of the mapped region via an out parameter.
* Change xc_dom_vaddr_to_ptr to always provide the length of the
mapped region and change the call site in xc_dom_binloader.c to
check it. The call site in xc_dom_load_elf_symtab will be corrected
in a forthcoming patch, and for now ignores the returned length.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
v5: This patch is new in v5 of the series.
---
tools/libxc/xc_dom.h | 16 +++++++++++++---
tools/libxc/xc_dom_binloader.c | 11 ++++++++++-
tools/libxc/xc_dom_core.c | 13 +++++++++++++
tools/libxc/xc_dom_elfloader.c | 3 ++-
4 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 19a10f4..6b118ad 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -259,6 +259,8 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first,
xen_pfn_t count);
+void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t first,
+ xen_pfn_t count, xen_pfn_t *count_out);
void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn);
void xc_dom_unmap_all(struct xc_dom_image *dom);
@@ -286,13 +288,21 @@ static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
}
static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom,
- xen_vaddr_t vaddr)
+ xen_vaddr_t vaddr,
+ size_t *safe_region_out)
{
unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
xen_pfn_t page = (vaddr - dom->parms.virt_base) / page_size;
unsigned int offset = (vaddr - dom->parms.virt_base) % page_size;
- void *ptr = xc_dom_pfn_to_ptr(dom, page, 0);
- return (ptr ? (ptr + offset) : NULL);
+ xen_pfn_t safe_region_count;
+ void *ptr;
+
+ *safe_region_out = 0;
+ ptr = xc_dom_pfn_to_ptr_retcount(dom, page, 0, &safe_region_count);
+ if ( ptr == NULL )
+ return ptr;
+ *safe_region_out = (safe_region_count << XC_DOM_PAGE_SHIFT(dom)) - offset;
+ return ptr;
}
static inline int xc_dom_feature_translated(struct xc_dom_image *dom)
diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index 769e97d..bde93f7 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -249,6 +249,7 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom)
char *image = dom->kernel_blob;
char *dest;
size_t image_size = dom->kernel_size;
+ size_t dest_size;
uint32_t start_addr;
uint32_t load_end_addr;
uint32_t bss_end_addr;
@@ -272,7 +273,15 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom)
DOMPRINTF(" text_size: 0x%" PRIx32 "", text_size);
DOMPRINTF(" bss_size: 0x%" PRIx32 "", bss_size);
- dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart);
+ dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size);
+
+ if ( dest_size < text_size ||
+ dest_size - text_size < bss_size )
+ {
+ DOMPRINTF("%s: mapped region is too small for image", __FUNCTION__);
+ return -EINVAL;
+ }
+
memcpy(dest, image + skip, text_size);
memset(dest + text_size, 0, bss_size);
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 2a01d7c..8913e41 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -351,10 +351,19 @@ int xc_dom_try_gunzip(struct xc_dom_image *dom, void **blob, size_t * size)
void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn,
xen_pfn_t count)
{
+ xen_pfn_t count_out_dummy;
+ return xc_dom_pfn_to_ptr_retcount(dom, pfn, count, &count_out_dummy);
+}
+
+void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn,
+ xen_pfn_t count, xen_pfn_t *count_out)
+{
struct xc_dom_phys *phys;
unsigned int page_shift = XC_DOM_PAGE_SHIFT(dom);
char *mode = "unset";
+ *count_out = 0;
+
if ( pfn > dom->total_pages || /* multiple checks to avoid overflows */
count > dom->total_pages ||
pfn > dom->total_pages - count )
@@ -384,6 +393,7 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn,
phys->count);
return NULL;
}
+ *count_out = count;
}
else
{
@@ -391,6 +401,9 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn,
just hand out a pointer to it */
if ( pfn < phys->first )
continue;
+ if ( pfn >= phys->first + phys->count )
+ continue;
+ *count_out = phys->count - (pfn - phys->first);
}
return phys->ptr + ((pfn - phys->first) << page_shift);
}
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 9114bfb..afecded 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -130,10 +130,11 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
if ( load )
{
+ size_t allow_size; /* will be used in a forthcoming XSA-55 patch */
if ( !dom->bsd_symtab_start )
return 0;
size = dom->kernel_seg.vend - dom->bsd_symtab_start;
- hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start);
+ hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
*(int *)hdr = size - sizeof(int);
}
else
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 04/21] libelf: abolish elf_sval and elf_access_signed
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (2 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 03/21] libxc: Fix range checking in xc_dom_pfn_to_ptr etc Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 05/21] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised Ian Jackson
` (16 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
These are not used anywhere.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
xen/common/libelf/libelf-tools.c | 28 ----------------------------
xen/include/xen/libelf.h | 11 -----------
2 files changed, 0 insertions(+), 39 deletions(-)
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 885d5e8..a78174d 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -48,34 +48,6 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, const void *ptr,
}
}
-int64_t elf_access_signed(struct elf_binary *elf, const void *ptr,
- uint64_t offset, size_t size)
-{
- int need_swap = elf_swap(elf);
- const int8_t *s8;
- const int16_t *s16;
- const int32_t *s32;
- const int64_t *s64;
-
- switch ( size )
- {
- case 1:
- s8 = ptr + offset;
- return *s8;
- case 2:
- s16 = ptr + offset;
- return need_swap ? bswap_16(*s16) : *s16;
- case 4:
- s32 = ptr + offset;
- return need_swap ? bswap_32(*s32) : *s32;
- case 8:
- s64 = ptr + offset;
- return need_swap ? bswap_64(*s64) : *s64;
- default:
- return 0;
- }
-}
-
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
{
int elf_round = (elf_64bit(elf) ? 8 : 4) - 1;
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 584d8b3..c114021 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -136,23 +136,12 @@ struct elf_binary {
offsetof(typeof(*(str)),e32.elem), \
sizeof((str)->e32.elem)))
-#define elf_sval(elf, str, elem) \
- ((ELFCLASS64 == (elf)->class) \
- ? elf_access_signed((elf), (str), \
- offsetof(typeof(*(str)),e64.elem), \
- sizeof((str)->e64.elem)) \
- : elf_access_signed((elf), (str), \
- offsetof(typeof(*(str)),e32.elem), \
- sizeof((str)->e32.elem)))
-
#define elf_size(elf, str) \
((ELFCLASS64 == (elf)->class) \
? sizeof((str)->e64) : sizeof((str)->e32))
uint64_t elf_access_unsigned(struct elf_binary *elf, const void *ptr,
uint64_t offset, size_t size);
-int64_t elf_access_signed(struct elf_binary *elf, const void *ptr,
- uint64_t offset, size_t size);
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 05/21] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (3 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 04/21] libelf: abolish elf_sval and elf_access_signed Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 06/21] libelf: introduce macros for memory access and pointer handling Ian Jackson
` (15 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
xc_dom_load_elf_symtab (with load==0) calls elf_round_up, but it
mistakenly used the uninitialised variable "syms" when calculating
dom->bsd_symtab_start. This should be a reference to "elf".
This change might have the effect of rounding the value differently.
Previously if the uninitialised value (a single byte on the stack) was
ELFCLASS64 (ie, 2), the alignment would be to 8 bytes, otherwise to 4.
However, the value is calculated from dom->kernel_seg.vend so this
could only make a difference if that value wasn't already aligned to 8
bytes.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index afecded..0fc35e3 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -144,7 +144,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
hdr = xc_dom_malloc(dom, size);
if ( hdr == NULL )
return 0;
- dom->bsd_symtab_start = elf_round_up(&syms, dom->kernel_seg.vend);
+ dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
}
memcpy(hdr + sizeof(int),
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 06/21] libelf: introduce macros for memory access and pointer handling
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (4 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 05/21] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 07/21] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note Ian Jackson
` (14 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
We introduce a collection of macros which abstract away all the
pointer arithmetic and dereferences used for accessing the input ELF
and the output area(s). We use the new macros everywhere.
For now, these macros are semantically identical to the code they
replace, so this patch has no functional change.
elf_is_elfbinary is an exception: since it doesn't take an elf*, we
need to handle it differently. In a future patch we will change it to
take, and check, a length parameter. For now we just mark it with a
fixme.
Nontrivial differences in the 4.1 backport:
* We need to provide our own elf_uintptr_t since Xen doesn't.
* We see some additional differences in our verification diff.
* The "function-filter" needs to massage additional symbol names.
Conflicts:
* In xc_dom_load_elf_symtab the old code used
*(Elf64_Word*)(&shdr->e64.sh_name) and the new Elf32_Word
but in fact the type in the struct has changed too so the
new code using elf_store_field is still correct.
* loadelfimage, elf_load_image etc. don't exist and are done
directly with memcpy/memset; patch adjusted appropriately.
* elf_note_numeric_array doesn't exist in 4.1.
That this patch has no functional change can be verified as follows:
0. Copy the scripts "comparison-generate" and "function-filter"
out of this commit message.
1. Check out the tree before this patch.
2. Run the script ../comparison-generate .... ../before
3. Check out the tree after this patch.
4. Run the script ../comparison-generate .... ../after
5. diff --exclude=\*.[soi] -ruN before/ after/ |less
Expect these differences:
* stubdom/zlib-x86_64/ztest*.s2
The filename of this test file apparently contains the pid.
* stubdom/grub/kexec.s2:
Large differences following ".section .debug_info" (which
the 4.1 build system erroneously fails to suppress).
* tools/libxc/xc_dom_x86.s2:
Two trivial code generation differences with no semantic
import.
* xen/common/version.s2
The xen build timestamp appears in two diff hunks.
Verification that this is all that's needed:
In a completely built xen.git,
find * -name .*.d -type f | xargs grep -l libelf\.h
Expect results in:
xen/arch/x86: Checked above.
tools/libxc: Checked above.
tools/xcutils/readnotes: Checked above.
tools/xenstore: Checked above.
xen/common/libelf:
This is the build for the hypervisor; checked in B above.
stubdom:
We have one stubdom which reads ELFs using our libelf,
pvgrub, which is checked above.
I have not done this verification for ARM.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
-8<- comparison-generate -8<-
#!/bin/bash
# usage:
# cd xen.git
# .../comparison-generate OUR-CONFIG BUILD-RUNE-PREFIX ../before|../after
# eg:
# .../comparison-generate ~/work/.config 'schroot -pc64 --' ../before
set -ex
test $# = 3 || need-exactly-three-arguments
our_config=$1
build_rune_prefix=$2
result_dir=$3
git clean -x -d -f
cp "$our_config" .
cat <<END >>.config
debug_symbols=n
CFLAGS += -save-temps
END
perl -i~ -pe 's/ -g / -g0 / if m/^CFLAGS/' xen/Rules.mk
if [ -f ./configure ]; then
$build_rune_prefix ./configure
fi
$build_rune_prefix make -C xen
$build_rune_prefix make -C tools/include
$build_rune_prefix make -C stubdom grub
$build_rune_prefix make -C tools/libxc
$build_rune_prefix make -C tools/xenstore
$build_rune_prefix make -C tools/xcutils
rm -rf "$result_dir"
mkdir "$result_dir"
set +x
for f in `find xen tools stubdom -name \*.[soi]`; do
mkdir -p "$result_dir"/`dirname $f`
cp $f "$result_dir"/${f}
case $f in
*.s)
../function-filter <$f >"$result_dir"/${f}2
;;
esac
done
echo ok.
-8<-
-8<- function-filter -8<-
#!/usr/bin/perl -w
# function-filter
# script for massaging gcc-generated labels to be consistent
use strict;
our @lines;
my $sedderybody = "sub seddery () {\n";
while (<>) {
push @lines, $_;
if (m/^(__FUNCTION__|__func__|_ctx|note_desc|types|last_order|memflags|mutex|d\d_cpu_last|write_count|wall_last|__PRETTY_FUNCTION__)\.(\d+)\:/ ||
m/^\s+\.local\s+(_ctx|write_count|d\d_cpu_last|wall_last|mutex)\.(\d+)\s*$/) {
$sedderybody .= " s/\\b$1\\.$2\\b/__XSA55MANGLED__$1.$./g;\n";
}
}
$sedderybody .= "}\n1;\n";
eval $sedderybody or die $@;
foreach (@lines) {
seddery();
print or die $!;
}
-8<-
---
tools/libxc/xc_dom_elfloader.c | 30 +++---
tools/xcutils/readnotes.c | 26 +++---
xen/common/libelf/libelf-dominfo.c | 51 +++++-----
xen/common/libelf/libelf-loader.c | 70 +++++++------
xen/common/libelf/libelf-tools.c | 90 +++++++++--------
xen/include/xen/libelf.h | 196 +++++++++++++++++++++++++++++++-----
6 files changed, 310 insertions(+), 153 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 0fc35e3..0fd8c42 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -115,9 +115,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
struct elf_binary *elf, int load)
{
struct elf_binary syms;
- const elf_shdr *shdr, *shdr2;
+ ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
xen_vaddr_t symtab, maxaddr;
- char *hdr;
+ ELF_PTRVAL_CHAR hdr;
size_t size;
int h, count, type, i, tables = 0;
@@ -147,11 +147,11 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
}
- memcpy(hdr + sizeof(int),
- elf->image,
+ elf_memcpy_safe(elf, hdr + sizeof(int),
+ ELF_IMAGE_BASE(elf),
elf_size(elf, elf->ehdr));
- memcpy(hdr + sizeof(int) + elf_size(elf, elf->ehdr),
- elf->image + elf_uval(elf, elf->ehdr, e_shoff),
+ elf_memcpy_safe(elf, hdr + sizeof(int) + elf_size(elf, elf->ehdr),
+ ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
elf_shdr_count(elf) * elf_size(elf, shdr));
if ( elf_64bit(elf) )
{
@@ -189,7 +189,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
count = elf_shdr_count(&syms);
for ( h = 0; h < count; h++ )
{
- shdr = elf_shdr_by_index(&syms, h);
+ shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h);
type = elf_uval(&syms, shdr, sh_type);
if ( type == SHT_STRTAB )
{
@@ -205,9 +205,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
if ( i == count )
{
if ( elf_64bit(&syms) )
- *(Elf64_Off*)(&shdr->e64.sh_offset) = 0;
+ elf_store_field(elf, shdr, e64.sh_offset, 0);
else
- *(Elf32_Off*)(&shdr->e32.sh_offset) = 0;
+ elf_store_field(elf, shdr, e32.sh_offset, 0);
continue;
}
}
@@ -216,9 +216,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
{
/* Mangled to be based on ELF header location. */
if ( elf_64bit(&syms) )
- *(Elf64_Off*)(&shdr->e64.sh_offset) = maxaddr - symtab;
+ elf_store_field(elf, shdr, e64.sh_offset, maxaddr - symtab);
else
- *(Elf32_Off*)(&shdr->e32.sh_offset) = maxaddr - symtab;
+ elf_store_field(elf, shdr, e32.sh_offset, maxaddr - symtab);
size = elf_uval(&syms, shdr, sh_size);
maxaddr = elf_round_up(&syms, maxaddr + size);
tables++;
@@ -230,7 +230,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
if ( load )
{
shdr2 = elf_shdr_by_index(elf, h);
- memcpy((void*)elf_section_start(&syms, shdr),
+ elf_memcpy_safe(elf, ELF_OBSOLETE_VOIDP_CAST elf_section_start(&syms, shdr),
elf_section_start(elf, shdr2),
size);
}
@@ -238,9 +238,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
/* Name is NULL. */
if ( elf_64bit(&syms) )
- *(Elf64_Half*)(&shdr->e64.sh_name) = 0;
+ elf_store_field(elf, shdr, e64.sh_name, 0);
else
- *(Elf32_Word*)(&shdr->e32.sh_name) = 0;
+ elf_store_field(elf, shdr, e32.sh_name, 0);
}
if ( tables == 0 )
@@ -275,7 +275,7 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
}
/* Find the section-header strings table. */
- if ( elf->sec_strtab == NULL )
+ if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
{
xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
" has no shstrtab", __FUNCTION__);
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index 2637685..15e5514 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -19,13 +19,13 @@
static xc_interface *xch;
static void print_string_note(const char *prefix, struct elf_binary *elf,
- const elf_note *note)
+ ELF_HANDLE_DECL(elf_note) note)
{
printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note));
}
static void print_numeric_note(const char *prefix, struct elf_binary *elf,
- const elf_note *note)
+ ELF_HANDLE_DECL(elf_note) note)
{
uint64_t value = elf_note_numeric(elf, note);
int descsz = elf_uval(elf, note, descsz);
@@ -56,12 +56,12 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
}
-static int print_notes(struct elf_binary *elf, const elf_note *start, const elf_note *end)
+static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end)
{
- const elf_note *note;
+ ELF_HANDLE_DECL(elf_note) note;
int notes_found = 0;
- for ( note = start; note < end; note = elf_note_next(elf, note) )
+ for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) )
{
if (0 != strcmp(elf_note_name(elf, note), "Xen"))
continue;
@@ -128,7 +128,7 @@ int main(int argc, char **argv)
void *image,*tmp;
struct stat st;
struct elf_binary elf;
- const elf_shdr *shdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
int notes_found = 0;
if (argc != 2)
@@ -180,7 +180,7 @@ int main(int argc, char **argv)
count = elf_phdr_count(&elf);
for ( h=0; h < count; h++)
{
- const elf_phdr *phdr;
+ ELF_HANDLE_DECL(elf_phdr) phdr;
phdr = elf_phdr_by_index(&elf, h);
if (elf_uval(&elf, phdr, p_type) != PT_NOTE)
continue;
@@ -192,8 +192,8 @@ int main(int argc, char **argv)
continue;
notes_found = print_notes(&elf,
- elf_segment_start(&elf, phdr),
- elf_segment_end(&elf, phdr));
+ ELF_MAKE_HANDLE(elf_note, elf_segment_start(&elf, phdr)),
+ ELF_MAKE_HANDLE(elf_note, elf_segment_end(&elf, phdr)));
}
if ( notes_found == 0 )
@@ -201,13 +201,13 @@ int main(int argc, char **argv)
count = elf_shdr_count(&elf);
for ( h=0; h < count; h++)
{
- const elf_shdr *shdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
shdr = elf_shdr_by_index(&elf, h);
if (elf_uval(&elf, shdr, sh_type) != SHT_NOTE)
continue;
notes_found = print_notes(&elf,
- elf_section_start(&elf, shdr),
- elf_section_end(&elf, shdr));
+ ELF_MAKE_HANDLE(elf_note, elf_section_start(&elf, shdr)),
+ ELF_MAKE_HANDLE(elf_note, elf_section_end(&elf, shdr)));
if ( notes_found )
fprintf(stderr, "using notes from SHT_NOTE section\n");
@@ -215,7 +215,7 @@ int main(int argc, char **argv)
}
shdr = elf_shdr_by_name(&elf, "__xen_guest");
- if (shdr)
+ if (ELF_HANDLE_VALID(shdr))
printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr));
return 0;
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 29c3339..1ae57ca 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -43,7 +43,7 @@ int elf_xen_parse_features(const char *features,
for ( pos = 0; features[pos] != '\0'; pos += len )
{
- memset(feature, 0, sizeof(feature));
+ elf_memset_unchecked(feature, 0, sizeof(feature));
for ( len = 0;; len++ )
{
if ( len >= sizeof(feature)-1 )
@@ -94,7 +94,7 @@ int elf_xen_parse_features(const char *features,
int elf_xen_parse_note(struct elf_binary *elf,
struct elf_dom_parms *parms,
- const elf_note *note)
+ ELF_HANDLE_DECL(elf_note) note)
{
/* *INDENT-OFF* */
static const struct {
@@ -205,15 +205,16 @@ int elf_xen_parse_note(struct elf_binary *elf,
static int elf_xen_parse_notes(struct elf_binary *elf,
struct elf_dom_parms *parms,
- const void *start, const void *end)
+ ELF_PTRVAL_CONST_VOID start,
+ ELF_PTRVAL_CONST_VOID end)
{
int xen_elfnotes = 0;
- const elf_note *note;
+ ELF_HANDLE_DECL(elf_note) note;
parms->elf_note_start = start;
parms->elf_note_end = end;
- for ( note = parms->elf_note_start;
- (void *)note < parms->elf_note_end;
+ for ( note = ELF_MAKE_HANDLE(elf_note, parms->elf_note_start);
+ ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
note = elf_note_next(elf, note) )
{
if ( strcmp(elf_note_name(elf, note), "Xen") )
@@ -231,45 +232,46 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
int elf_xen_parse_guest_info(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
- const char *h;
+ ELF_PTRVAL_CONST_CHAR h;
char name[32], value[128];
int len;
h = parms->guest_info;
- while ( *h )
+#define STAR(h) (*(h))
+ while ( STAR(h) )
{
- memset(name, 0, sizeof(name));
- memset(value, 0, sizeof(value));
+ elf_memset_unchecked(name, 0, sizeof(name));
+ elf_memset_unchecked(value, 0, sizeof(value));
for ( len = 0;; len++, h++ )
{
if ( len >= sizeof(name)-1 )
break;
- if ( *h == '\0' )
+ if ( STAR(h) == '\0' )
break;
- if ( *h == ',' )
+ if ( STAR(h) == ',' )
{
h++;
break;
}
- if ( *h == '=' )
+ if ( STAR(h) == '=' )
{
h++;
for ( len = 0;; len++, h++ )
{
if ( len >= sizeof(value)-1 )
break;
- if ( *h == '\0' )
+ if ( STAR(h) == '\0' )
break;
- if ( *h == ',' )
+ if ( STAR(h) == ',' )
{
h++;
break;
}
- value[len] = *h;
+ value[len] = STAR(h);
}
break;
}
- name[len] = *h;
+ name[len] = STAR(h);
}
elf_msg(elf, "%s: %s=\"%s\"\n", __FUNCTION__, name, value);
@@ -318,7 +320,8 @@ int elf_xen_parse_guest_info(struct elf_binary *elf,
static int elf_xen_note_check(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
- if ( (parms->elf_note_start == NULL) && (parms->guest_info == NULL) )
+ if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) &&
+ (ELF_PTRVAL_INVALID(parms->guest_info)) )
{
int machine = elf_uval(elf, elf->ehdr, e_machine);
if ( (machine == EM_386) || (machine == EM_X86_64) )
@@ -441,12 +444,12 @@ static int elf_xen_addr_calc_check(struct elf_binary *elf,
int elf_xen_parse(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
- const elf_shdr *shdr;
- const elf_phdr *phdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
+ ELF_HANDLE_DECL(elf_phdr) phdr;
int xen_elfnotes = 0;
int i, count, rc;
- memset(parms, 0, sizeof(*parms));
+ elf_memset_unchecked(parms, 0, sizeof(*parms));
parms->virt_base = UNSET_ADDR;
parms->virt_entry = UNSET_ADDR;
parms->virt_hypercall = UNSET_ADDR;
@@ -516,11 +519,11 @@ int elf_xen_parse(struct elf_binary *elf,
for ( i = 0; i < count; i++ )
{
shdr = elf_shdr_by_name(elf, "__xen_guest");
- if ( shdr )
+ if ( ELF_HANDLE_VALID(shdr) )
{
parms->guest_info = elf_section_start(elf, shdr);
- parms->elf_note_start = NULL;
- parms->elf_note_end = NULL;
+ parms->elf_note_start = ELF_INVALID_PTRVAL;
+ parms->elf_note_end = ELF_INVALID_PTRVAL;
elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
parms->guest_info);
elf_xen_parse_guest_info(elf, parms);
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 1ccf7d3..5c27696 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -22,7 +22,7 @@
int elf_init(struct elf_binary *elf, const char *image, size_t size)
{
- const elf_shdr *shdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
uint64_t i, count, section, offset;
if ( !elf_is_elfbinary(image) )
@@ -31,7 +31,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size)
return -1;
}
- memset(elf, 0, sizeof(*elf));
+ elf_memset_unchecked(elf, 0, sizeof(*elf));
elf->image = image;
elf->size = size;
elf->ehdr = (elf_ehdr *)image;
@@ -61,7 +61,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size)
/* Find section string table. */
section = elf_uval(elf, elf->ehdr, e_shstrndx);
shdr = elf_shdr_by_index(elf, section);
- if ( shdr != NULL )
+ if ( ELF_HANDLE_VALID(shdr) )
elf->sec_strtab = elf_section_start(elf, shdr);
/* Find symbol table and symbol string table. */
@@ -73,9 +73,9 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size)
continue;
elf->sym_tab = shdr;
shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link));
- if ( shdr == NULL )
+ if ( !ELF_HANDLE_VALID(shdr) )
{
- elf->sym_tab = NULL;
+ elf->sym_tab = ELF_INVALID_HANDLE(elf_shdr);
continue;
}
elf->sym_strtab = elf_section_start(elf, shdr);
@@ -118,10 +118,10 @@ void elf_set_verbose(struct elf_binary *elf)
void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
{
uint64_t sz;
- const elf_shdr *shdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
int i, type;
- if ( !elf->sym_tab )
+ if ( !ELF_HANDLE_VALID(elf->sym_tab) )
return;
pstart = elf_round_up(elf, pstart);
@@ -138,7 +138,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
shdr = elf_shdr_by_index(elf, i);
- type = elf_uval(elf, (elf_shdr *)shdr, sh_type);
+ type = elf_uval(elf, shdr, sh_type);
if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
}
@@ -149,10 +149,12 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
static void elf_load_bsdsyms(struct elf_binary *elf)
{
- elf_ehdr *sym_ehdr;
+ ELF_HANDLE_DECL_NONCONST(elf_ehdr) sym_ehdr;
unsigned long sz;
- char *maxva, *symbase, *symtab_addr;
- elf_shdr *shdr;
+ ELF_PTRVAL_VOID maxva;
+ ELF_PTRVAL_VOID symbase;
+ ELF_PTRVAL_VOID symtab_addr;
+ ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr;
int i, type;
if ( !elf->bsd_symtab_pstart )
@@ -161,18 +163,18 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
#define elf_hdr_elm(_elf, _hdr, _elm, _val) \
do { \
if ( elf_64bit(_elf) ) \
- (_hdr)->e64._elm = _val; \
+ elf_store_field(_elf, _hdr, e64._elm, _val); \
else \
- (_hdr)->e32._elm = _val; \
+ elf_store_field(_elf, _hdr, e32._elm, _val); \
} while ( 0 )
symbase = elf_get_ptr(elf, elf->bsd_symtab_pstart);
symtab_addr = maxva = symbase + sizeof(uint32_t);
/* Set up Elf header. */
- sym_ehdr = (elf_ehdr *)symtab_addr;
+ sym_ehdr = ELF_MAKE_HANDLE(elf_ehdr, symtab_addr);
sz = elf_uval(elf, elf->ehdr, e_ehsize);
- memcpy(sym_ehdr, elf->ehdr, sz);
+ elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz);
maxva += sz; /* no round up */
elf_hdr_elm(elf, sym_ehdr, e_phoff, 0);
@@ -181,37 +183,39 @@ do { \
elf_hdr_elm(elf, sym_ehdr, e_phnum, 0);
/* Copy Elf section headers. */
- shdr = (elf_shdr *)maxva;
+ shdr = ELF_MAKE_HANDLE(elf_shdr, maxva);
sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize);
- memcpy(shdr, elf->image + elf_uval(elf, elf->ehdr, e_shoff), sz);
- maxva = (char *)(long)elf_round_up(elf, (long)maxva + sz);
+ elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr),
+ ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
+ sz);
+ maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz);
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
type = elf_uval(elf, shdr, sh_type);
if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
{
- elf_msg(elf, "%s: shdr %i at 0x%p -> 0x%p\n", __func__, i,
+ elf_msg(elf, "%s: shdr %i at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", __func__, i,
elf_section_start(elf, shdr), maxva);
sz = elf_uval(elf, shdr, sh_size);
- memcpy(maxva, elf_section_start(elf, shdr), sz);
+ elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz);
/* Mangled to be based on ELF header location. */
elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
- maxva = (char *)(long)elf_round_up(elf, (long)maxva + sz);
+ maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz);
}
- shdr = (elf_shdr *)((long)shdr +
+ shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) +
(long)elf_uval(elf, elf->ehdr, e_shentsize));
}
/* Write down the actual sym size. */
- *(uint32_t *)symbase = maxva - symtab_addr;
+ elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr);
#undef elf_ehdr_elm
}
void elf_parse_binary(struct elf_binary *elf)
{
- const elf_phdr *phdr;
+ ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t low = -1;
uint64_t high = 0;
uint64_t i, count, paddr, memsz;
@@ -239,9 +243,9 @@ void elf_parse_binary(struct elf_binary *elf)
void elf_load_binary(struct elf_binary *elf)
{
- const elf_phdr *phdr;
+ ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t i, count, paddr, offset, filesz, memsz;
- char *dest;
+ ELF_PTRVAL_VOID dest;
count = elf_uval(elf, elf->ehdr, e_phnum);
for ( i = 0; i < count; i++ )
@@ -254,27 +258,27 @@ void elf_load_binary(struct elf_binary *elf)
filesz = elf_uval(elf, phdr, p_filesz);
memsz = elf_uval(elf, phdr, p_memsz);
dest = elf_get_ptr(elf, paddr);
- elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%p -> 0x%p\n",
- __func__, i, dest, dest + filesz);
- memcpy(dest, elf->image + offset, filesz);
- memset(dest + filesz, 0, memsz - filesz);
+ elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n",
+ __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz));
+ elf_memcpy_safe(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz);
+ elf_memset_safe(elf, dest + filesz, 0, memsz - filesz);
}
elf_load_bsdsyms(elf);
}
-void *elf_get_ptr(struct elf_binary *elf, unsigned long addr)
+ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr)
{
return elf->dest + addr - elf->pstart;
}
uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
{
- const elf_sym *sym;
+ ELF_HANDLE_DECL(elf_sym) sym;
uint64_t value;
sym = elf_sym_by_name(elf, symbol);
- if ( sym == NULL )
+ if ( !ELF_HANDLE_VALID(sym) )
{
elf_err(elf, "%s: not found: %s\n", __FUNCTION__, symbol);
return -1;
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index a78174d..af260fa 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -67,10 +67,10 @@ int elf_phdr_count(struct elf_binary *elf)
return elf_uval(elf, elf->ehdr, e_phnum);
}
-const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name)
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name)
{
uint64_t count = elf_shdr_count(elf);
- const elf_shdr *shdr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
const char *sname;
int i;
@@ -81,76 +81,80 @@ const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name)
if ( sname && !strcmp(sname, name) )
return shdr;
}
- return NULL;
+ return ELF_INVALID_HANDLE(elf_shdr);
}
-const elf_shdr *elf_shdr_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index)
{
uint64_t count = elf_shdr_count(elf);
- const void *ptr;
+ ELF_PTRVAL_CONST_VOID ptr;
if ( index >= count )
- return NULL;
+ return ELF_INVALID_HANDLE(elf_shdr);
- ptr = (elf->image
+ ptr = (ELF_IMAGE_BASE(elf)
+ elf_uval(elf, elf->ehdr, e_shoff)
+ elf_uval(elf, elf->ehdr, e_shentsize) * index);
- return ptr;
+ return ELF_MAKE_HANDLE(elf_shdr, ptr);
}
-const elf_phdr *elf_phdr_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index)
{
uint64_t count = elf_uval(elf, elf->ehdr, e_phnum);
- const void *ptr;
+ ELF_PTRVAL_CONST_VOID ptr;
if ( index >= count )
- return NULL;
+ return ELF_INVALID_HANDLE(elf_phdr);
- ptr = (elf->image
+ ptr = (ELF_IMAGE_BASE(elf)
+ elf_uval(elf, elf->ehdr, e_phoff)
+ elf_uval(elf, elf->ehdr, e_phentsize) * index);
- return ptr;
+ return ELF_MAKE_HANDLE(elf_phdr, ptr);
}
-const char *elf_section_name(struct elf_binary *elf, const elf_shdr * shdr)
+
+const char *elf_section_name(struct elf_binary *elf,
+ ELF_HANDLE_DECL(elf_shdr) shdr)
{
- if ( elf->sec_strtab == NULL )
+ if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
return "unknown";
+
return elf->sec_strtab + elf_uval(elf, shdr, sh_name);
}
-const void *elf_section_start(struct elf_binary *elf, const elf_shdr * shdr)
+ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
{
- return elf->image + elf_uval(elf, shdr, sh_offset);
+ return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset);
}
-const void *elf_section_end(struct elf_binary *elf, const elf_shdr * shdr)
+ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
{
- return elf->image
+ return ELF_IMAGE_BASE(elf)
+ elf_uval(elf, shdr, sh_offset) + elf_uval(elf, shdr, sh_size);
}
-const void *elf_segment_start(struct elf_binary *elf, const elf_phdr * phdr)
+ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
- return elf->image + elf_uval(elf, phdr, p_offset);
+ return ELF_IMAGE_BASE(elf)
+ + elf_uval(elf, phdr, p_offset);
}
-const void *elf_segment_end(struct elf_binary *elf, const elf_phdr * phdr)
+ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
- return elf->image
+ return ELF_IMAGE_BASE(elf)
+ elf_uval(elf, phdr, p_offset) + elf_uval(elf, phdr, p_filesz);
}
-const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol)
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol)
{
- const void *ptr = elf_section_start(elf, elf->sym_tab);
- const void *end = elf_section_end(elf, elf->sym_tab);
- const elf_sym *sym;
+ ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab);
+ ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab);
+ ELF_HANDLE_DECL(elf_sym) sym;
uint64_t info, name;
for ( ; ptr < end; ptr += elf_size(elf, sym) )
{
- sym = ptr;
+ sym = ELF_MAKE_HANDLE(elf_sym, ptr);
info = elf_uval(elf, sym, st_info);
name = elf_uval(elf, sym, st_name);
if ( ELF32_ST_BIND(info) != STB_GLOBAL )
@@ -159,33 +163,33 @@ const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol)
continue;
return sym;
}
- return NULL;
+ return ELF_INVALID_HANDLE(elf_sym);
}
-const elf_sym *elf_sym_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index)
{
- const void *ptr = elf_section_start(elf, elf->sym_tab);
- const elf_sym *sym;
+ ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab);
+ ELF_HANDLE_DECL(elf_sym) sym;
- sym = ptr + index * elf_size(elf, sym);
+ sym = ELF_MAKE_HANDLE(elf_sym, ptr + index * elf_size(elf, sym));
return sym;
}
-const char *elf_note_name(struct elf_binary *elf, const elf_note * note)
+const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- return (void *)note + elf_size(elf, note);
+ return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note);
}
-const void *elf_note_desc(struct elf_binary *elf, const elf_note * note)
+ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
int namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
- return (void *)note + elf_size(elf, note) + namesz;
+ return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz;
}
-uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note)
+uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- const void *desc = elf_note_desc(elf, note);
+ ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
int descsz = elf_uval(elf, note, descsz);
switch (descsz)
@@ -199,12 +203,12 @@ uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note)
return 0;
}
}
-const elf_note *elf_note_next(struct elf_binary *elf, const elf_note * note)
+ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
int namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
int descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
- return (void *)note + elf_size(elf, note) + namesz + descsz;
+ return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz);
}
/* ------------------------------------------------------------------------ */
@@ -213,10 +217,10 @@ int elf_is_elfbinary(const void *image)
{
const Elf32_Ehdr *ehdr = image;
- return IS_ELF(*ehdr);
+ return IS_ELF(*ehdr); /* fixme unchecked */
}
-int elf_phdr_is_loadable(struct elf_binary *elf, const elf_phdr * phdr)
+int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
uint64_t p_type = elf_uval(elf, phdr, p_type);
uint64_t p_flags = elf_uval(elf, phdr, p_flags);
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index c114021..f5de8ed 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -48,6 +48,107 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
/* ------------------------------------------------------------------------ */
+/* Macros for accessing the input image and output area. */
+
+/*
+ * We abstract away the pointerness of these pointers, replacing
+ * various void*, char* and struct* with the following:
+ * PTRVAL A pointer to a byte; one can do pointer arithmetic
+ * on this.
+ * This replaces variables which were char*,void*
+ * and their const versions, so we provide four
+ * different declaration macros:
+ * ELF_PTRVAL_{,CONST}{VOID,CHAR}
+ * HANDLE A pointer to a struct. There is one of these types
+ * for each pointer type - that is, for each "structname".
+ * In the arguments to the various HANDLE macros, structname
+ * must be a single identifier which is a typedef.
+ * It is not permitted to do arithmetic on these
+ * pointers. In the current code attempts to do so will
+ * compile, but in the next patch this will become a
+ * compile error.
+ * We provide two declaration macros for const and
+ * non-const pointers.
+ */
+
+#ifdef __XEN__
+# ifdef __i386__
+typedef uint32_t elf_uintptr_t;
+# else
+typedef uint64_t elf_uintptr_t;
+# endif
+#else
+typedef uintptr_t elf_uintptr_t;
+#endif
+
+#define ELF_REALPTR2PTRVAL(realpointer) (realpointer)
+ /* Converts an actual C pointer into a PTRVAL */
+
+#define ELF_HANDLE_DECL_NONCONST(structname) structname *
+#define ELF_HANDLE_DECL(structname) const structname *
+ /* Provides a type declaration for a HANDLE. */
+ /* May only be used to declare ONE variable at a time */
+
+#define ELF_PTRVAL_VOID void *
+#define ELF_PTRVAL_CHAR char *
+#define ELF_PTRVAL_CONST_VOID const void *
+#define ELF_PTRVAL_CONST_CHAR const char *
+ /* Provides a type declaration for a PTRVAL. */
+ /* May only be used to declare ONE variable at a time */
+
+#define ELF_DEFINE_HANDLE(structname) /* empty */
+ /*
+ * This must be invoked for each HANDLE type to define
+ * the actual C type used for that kind of HANDLE.
+ */
+
+#define ELF_PRPTRVAL "p"
+ /* printf format a la PRId... for a PTRVAL */
+
+#define ELF_MAKE_HANDLE(structname, ptrval) (ptrval)
+ /* Converts a PTRVAL to a HANDLE */
+
+#define ELF_IMAGE_BASE(elf) ((elf)->image)
+ /* Returns the base of the image as a PTRVAL. */
+
+#define ELF_HANDLE_PTRVAL(handleval) ((void*)(handleval))
+ /* Converts a HANDLE to a PTRVAL. */
+
+#define ELF_OBSOLETE_VOIDP_CAST (void*)(elf_uintptr_t)
+ /*
+ * In some places the existing code needs to
+ * - cast away const (the existing code uses const a fair
+ * bit but actually sometimes wants to write to its input)
+ * from a PTRVAL.
+ * - convert an integer representing a pointer to a PTRVAL
+ * This macro provides a suitable cast.
+ */
+
+#define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_uintptr_t)(ptrval))
+ /*
+ * Turns a PTRVAL into an actual C pointer. Before this is done
+ * the caller must have ensured that the PTRVAL does in fact point
+ * to a permissible location.
+ */
+
+/* PTRVALs can be INVALID (ie, NULL). */
+#define ELF_INVALID_PTRVAL (NULL) /* returns NULL PTRVAL */
+#define ELF_INVALID_HANDLE(structname) /* returns NULL handle */ \
+ ELF_MAKE_HANDLE(structname, ELF_INVALID_PTRVAL)
+#define ELF_PTRVAL_VALID(ptrval) (ptrval) /* } */
+#define ELF_HANDLE_VALID(handleval) (handleval) /* } predicates */
+#define ELF_PTRVAL_INVALID(ptrval) ((ptrval) == NULL) /* } */
+
+/* For internal use by other macros here */
+#define ELF__HANDLE_FIELD_TYPE(handleval, elm) \
+ typeof((handleval)->elm)
+#define ELF__HANDLE_FIELD_OFFSET(handleval, elm) \
+ offsetof(typeof(*(handleval)),elm)
+
+
+/* ------------------------------------------------------------------------ */
+
+
typedef union {
Elf32_Ehdr e32;
Elf64_Ehdr e64;
@@ -83,6 +184,12 @@ typedef union {
Elf64_Note e64;
} elf_note;
+ELF_DEFINE_HANDLE(elf_ehdr)
+ELF_DEFINE_HANDLE(elf_shdr)
+ELF_DEFINE_HANDLE(elf_phdr)
+ELF_DEFINE_HANDLE(elf_sym)
+ELF_DEFINE_HANDLE(elf_note)
+
struct elf_binary {
/* elf binary */
const char *image;
@@ -90,10 +197,10 @@ struct elf_binary {
char class;
char data;
- const elf_ehdr *ehdr;
- const char *sec_strtab;
- const elf_shdr *sym_tab;
- const char *sym_strtab;
+ ELF_HANDLE_DECL(elf_ehdr) ehdr;
+ ELF_PTRVAL_CONST_CHAR sec_strtab;
+ ELF_HANDLE_DECL(elf_shdr) sym_tab;
+ ELF_PTRVAL_CONST_CHAR sym_strtab;
/* loaded to */
char *dest;
@@ -135,43 +242,70 @@ struct elf_binary {
: elf_access_unsigned((elf), (str), \
offsetof(typeof(*(str)),e32.elem), \
sizeof((str)->e32.elem)))
+ /*
+ * Reads an unsigned field in a header structure in the ELF.
+ * str is a HANDLE, and elem is the field name in it.
+ */
#define elf_size(elf, str) \
((ELFCLASS64 == (elf)->class) \
? sizeof((str)->e64) : sizeof((str)->e32))
+ /*
+ * Returns the size of the substructure for the appropriate 32/64-bitness.
+ * str should be a HANDLE.
+ */
-uint64_t elf_access_unsigned(struct elf_binary *elf, const void *ptr,
+uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr,
uint64_t offset, size_t size);
+ /* Reads a field at arbitrary offset and alignemnt */
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
+
+#define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz))
+#define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz))
+ /*
+ * Versions of memcpy and memset which will (in the next patch)
+ * arrange never to write outside permitted areas.
+ */
+
+#define elf_store_val(elf, type, ptr, val) (*(type*)(ptr) = (val))
+ /* Stores a value at a particular PTRVAL. */
+
+#define elf_store_field(elf, hdr, elm, val) \
+ (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \
+ &((hdr)->elm), \
+ (val)))
+ /* Stores a 32/64-bit field. hdr is a HANDLE and elm is the field name. */
+
+
/* ------------------------------------------------------------------------ */
/* xc_libelf_tools.c */
int elf_shdr_count(struct elf_binary *elf);
int elf_phdr_count(struct elf_binary *elf);
-const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name);
-const elf_shdr *elf_shdr_by_index(struct elf_binary *elf, int index);
-const elf_phdr *elf_phdr_by_index(struct elf_binary *elf, int index);
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name);
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index);
+ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index);
-const char *elf_section_name(struct elf_binary *elf, const elf_shdr * shdr);
-const void *elf_section_start(struct elf_binary *elf, const elf_shdr * shdr);
-const void *elf_section_end(struct elf_binary *elf, const elf_shdr * shdr);
+const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
+ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
+ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
-const void *elf_segment_start(struct elf_binary *elf, const elf_phdr * phdr);
-const void *elf_segment_end(struct elf_binary *elf, const elf_phdr * phdr);
+ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
+ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
-const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol);
-const elf_sym *elf_sym_by_index(struct elf_binary *elf, int index);
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol);
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index);
-const char *elf_note_name(struct elf_binary *elf, const elf_note * note);
-const void *elf_note_desc(struct elf_binary *elf, const elf_note * note);
-uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note);
-const elf_note *elf_note_next(struct elf_binary *elf, const elf_note * note);
+const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
+ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
+uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
+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_phdr_is_loadable(struct elf_binary *elf, const elf_phdr * phdr);
+int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
/* ------------------------------------------------------------------------ */
/* xc_libelf_loader.c */
@@ -187,7 +321,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*,
void elf_parse_binary(struct elf_binary *elf);
void elf_load_binary(struct elf_binary *elf);
-void *elf_get_ptr(struct elf_binary *elf, unsigned long addr);
+ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr);
uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol);
void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */
@@ -219,9 +353,9 @@ struct xen_elfnote {
struct elf_dom_parms {
/* raw */
- const char *guest_info;
- const void *elf_note_start;
- const void *elf_note_end;
+ ELF_PTRVAL_CONST_CHAR guest_info;
+ ELF_PTRVAL_CONST_VOID elf_note_start;
+ ELF_PTRVAL_CONST_VOID elf_note_end;
struct xen_elfnote elf_notes[XEN_ELFNOTE_MAX + 1];
/* parsed */
@@ -260,10 +394,22 @@ int elf_xen_parse_features(const char *features,
uint32_t *required);
int elf_xen_parse_note(struct elf_binary *elf,
struct elf_dom_parms *parms,
- const elf_note *note);
+ ELF_HANDLE_DECL(elf_note) note);
int elf_xen_parse_guest_info(struct elf_binary *elf,
struct elf_dom_parms *parms);
int elf_xen_parse(struct elf_binary *elf,
struct elf_dom_parms *parms);
+#define elf_memcpy_unchecked memcpy
+#define elf_memset_unchecked memset
+ /*
+ * Unsafe versions of memcpy and memset which take actual C
+ * pointers. These are just like real memcpy and memset.
+ */
+
+
+#define ELF_ADVANCE_DEST(elf, amount) elf->dest += (amount)
+ /* Advances past amount bytes of the current destination area. */
+
+
#endif /* __XEN_LIBELF_H__ */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 07/21] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (5 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 06/21] libelf: introduce macros for memory access and pointer handling Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 08/21] libelf: check nul-terminated strings properly Ian Jackson
` (13 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Use the new PTRVAL macros and elf_access_unsigned in
print_l1_mfn_valid_note.
No functional change unless the input is wrong, or we are reading a
file for a different endianness.
Separated out from the previous patch because this change does produce
a difference in the generated code.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/xcutils/readnotes.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index 15e5514..af8ea12 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -35,22 +35,23 @@ static void print_numeric_note(const char *prefix, struct elf_binary *elf,
}
static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
- const elf_note *note)
+ ELF_HANDLE_DECL(elf_note) note)
{
int descsz = elf_uval(elf, note, descsz);
- const uint32_t *desc32 = elf_note_desc(elf, note);
- const uint64_t *desc64 = elf_note_desc(elf, note);
+ ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
/* XXX should be able to cope with a list of values. */
switch ( descsz / 2 )
{
case 8:
printf("%s: mask=%#"PRIx64" value=%#"PRIx64"\n", prefix,
- desc64[0], desc64[1]);
+ elf_access_unsigned(elf, desc, 0, 8),
+ elf_access_unsigned(elf, desc, 8, 8));
break;
case 4:
printf("%s: mask=%#"PRIx32" value=%#"PRIx32"\n", prefix,
- desc32[0],desc32[1]);
+ (uint32_t)elf_access_unsigned(elf, desc, 0, 4),
+ (uint32_t)elf_access_unsigned(elf, desc, 4, 4));
break;
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 08/21] libelf: check nul-terminated strings properly
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (6 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 07/21] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 09/21] libelf: check all pointer accesses Ian Jackson
` (12 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
It is not safe to simply take pointers into the ELF and use them as C
pointers. They might not be properly nul-terminated (and the pointers
might be wild).
So we are going to introduce a new function elf_strval for safely
getting strings. This will check that the addresses are in range and
that there is a proper nul-terminated string. Of course it might
discover that there isn't. In that case, it will be made to fail.
This means that elf_note_name might fail, too.
For the benefit of call sites which are just going to pass the value
to a printf-like function, we provide elf_strfmt which returns
"(invalid)" on failure rather than NULL.
In this patch we introduce dummy definitions of these functions. We
introduce calls to elf_strval and elf_strfmt everywhere, and update
all the call sites with appropriate error checking.
There is not yet any semantic change, since before this patch all the
places where we introduce elf_strval dereferenced the value anyway, so
it mustn't have been NULL.
In future patches, when elf_strval is made able return NULL, when it
does so it will mark the elf "broken" so that an appropriate
diagnostic can be printed.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/xcutils/readnotes.c | 11 ++++++++---
xen/common/libelf/libelf-dominfo.c | 13 ++++++++++---
xen/common/libelf/libelf-tools.c | 10 +++++++---
xen/include/xen/libelf.h | 7 +++++--
4 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index af8ea12..d462f80 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -21,7 +21,7 @@ static xc_interface *xch;
static void print_string_note(const char *prefix, struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note)
{
- printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note));
+ printf("%s: %s\n", prefix, elf_strfmt(elf, elf_note_desc(elf, note)));
}
static void print_numeric_note(const char *prefix, struct elf_binary *elf,
@@ -61,10 +61,14 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
{
ELF_HANDLE_DECL(elf_note) note;
int notes_found = 0;
+ const char *this_note_name;
for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) )
{
- if (0 != strcmp(elf_note_name(elf, note), "Xen"))
+ this_note_name = elf_note_name(elf, note);
+ if (NULL == this_note_name)
+ continue;
+ if (0 != strcmp(this_note_name, "Xen"))
continue;
notes_found++;
@@ -217,7 +221,8 @@ int main(int argc, char **argv)
shdr = elf_shdr_by_name(&elf, "__xen_guest");
if (ELF_HANDLE_VALID(shdr))
- printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr));
+ printf("__xen_guest: %s\n",
+ elf_strfmt(&elf, elf_section_start(&elf, shdr)));
return 0;
}
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 1ae57ca..60673cd 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -133,7 +133,10 @@ int elf_xen_parse_note(struct elf_binary *elf,
if ( note_desc[type].str )
{
- str = elf_note_desc(elf, note);
+ str = elf_strval(elf, elf_note_desc(elf, note));
+ if (str == NULL)
+ /* elf_strval will mark elf broken if it fails so no need to log */
+ return 0;
elf_msg(elf, "%s: %s = \"%s\"\n", __FUNCTION__,
note_desc[type].name, str);
parms->elf_notes[type].type = XEN_ENT_STR;
@@ -210,6 +213,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
{
int xen_elfnotes = 0;
ELF_HANDLE_DECL(elf_note) note;
+ const char *note_name;
parms->elf_note_start = start;
parms->elf_note_end = end;
@@ -217,7 +221,10 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
note = elf_note_next(elf, note) )
{
- if ( strcmp(elf_note_name(elf, note), "Xen") )
+ note_name = elf_note_name(elf, note);
+ if ( note_name == NULL )
+ continue;
+ if ( strcmp(note_name, "Xen") )
continue;
if ( elf_xen_parse_note(elf, parms, note) )
return -1;
@@ -525,7 +532,7 @@ int elf_xen_parse(struct elf_binary *elf,
parms->elf_note_start = ELF_INVALID_PTRVAL;
parms->elf_note_end = ELF_INVALID_PTRVAL;
elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
- parms->guest_info);
+ elf_strfmt(elf, parms->guest_info));
elf_xen_parse_guest_info(elf, parms);
break;
}
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index af260fa..628c159 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -119,7 +119,7 @@ const char *elf_section_name(struct elf_binary *elf,
if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
return "unknown";
- return elf->sec_strtab + elf_uval(elf, shdr, sh_name);
+ return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name));
}
ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
@@ -151,6 +151,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab);
ELF_HANDLE_DECL(elf_sym) sym;
uint64_t info, name;
+ const char *sym_name;
for ( ; ptr < end; ptr += elf_size(elf, sym) )
{
@@ -159,7 +160,10 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
name = elf_uval(elf, sym, st_name);
if ( ELF32_ST_BIND(info) != STB_GLOBAL )
continue;
- if ( strcmp(elf->sym_strtab + name, symbol) )
+ sym_name = elf_strval(elf, elf->sym_strtab + name);
+ if ( sym_name == NULL ) /* out of range, oops */
+ return ELF_INVALID_HANDLE(elf_sym);
+ if ( strcmp(sym_name, symbol) )
continue;
return sym;
}
@@ -177,7 +181,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index)
const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note);
+ return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note));
}
ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index f5de8ed..7101064 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -262,6 +262,9 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr,
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
+#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */
+#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */
+
#define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz))
#define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz))
/*
@@ -289,7 +292,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index);
ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index);
-const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
+const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */
ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
@@ -299,7 +302,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el
ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol);
ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index);
-const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
+const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */
ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/21] libelf: check all pointer accesses
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (7 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 08/21] libelf: check nul-terminated strings properly Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 10/21] libelf: Check pointer references in elf_is_elfbinary Ian Jackson
` (11 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
We change the ELF_PTRVAL and ELF_HANDLE types and associated macros:
* PTRVAL becomes a uintptr_t, for which we provide a typedef
elf_ptrval. This means no arithmetic done on it can overflow so
the compiler cannot do any malicious invalid pointer arithmetic
"optimisations". It also means that any places where we
dereference one of these pointers without using the appropriate
macros or functions become a compilation error.
So we can be sure that we won't miss any memory accesses.
All the PTRVAL variables were previously void* or char*, so
the actual address calculations are unchanged.
* ELF_HANDLE becomes a union, one half of which keeps the pointer
value and the other half of which is just there to record the
type.
The new type is not a pointer type so there can be no address
calculations on it whose meaning would change. Every assignment or
access has to go through one of our macros.
* The distinction between const and non-const pointers and char*s
and void*s in libelf goes away. This was not important (and
anyway libelf tended to cast away const in various places).
* The fields elf->image and elf->dest are renamed. That proves
that we haven't missed any unchecked uses of these actual
pointer values.
* The caller may fill in elf->caller_xdest_base and _size to
specify another range of memory which is safe for libelf to
access, besides the input and output images.
* When accesses fail due to being out of range, we mark the elf
"broken". This will be checked and used for diagnostics in
a following patch.
We do not check for write accesses to the input image. This is
because libelf actually does this in a number of places. So we
simply permit that.
* Each caller of libelf which used to set dest now sets
dest_base and dest_size.
* In xc_dom_load_elf_symtab we provide a new actual-pointer
value hdr_ptr which we get from mapping the guest's kernel
area and use (checking carefully) as the caller_xdest area.
* The STAR(h) macro in libelf-dominfo.c now uses elf_access_unsigned.
* elf-init uses the new elf_uval_3264 accessor to access the 32-bit
fields, rather than an unchecked field access (ie, unchecked
pointer access).
* elf_uval has been reworked to use elf_uval_3264. Both of these
macros are essentially new in this patch (although they are derived
from the old elf_uval) and need careful review.
* ELF_ADVANCE_DEST is now safe in the sense that you can use it to
chop parts off the front of the dest area but if you chop more than
is available, the dest area is simply set to be empty, preventing
future accesses.
* We introduce some #defines for memcpy, memset, memmove and strcpy:
- We provide elf_memcpy_safe and elf_memset_safe which take
PTRVALs and do checking on the supplied pointers.
- Users inside libelf must all be changed to either
elf_mem*_unchecked (which are just like mem*), or
elf_mem*_safe (which take PTRVALs) and are checked. Any
unchanged call sites become compilation errors.
* We do _not_ at this time fix elf_access_unsigned so that it doesn't
make unaligned accesses. We hope that unaligned accesses are OK on
every supported architecture. But it does check the supplied
pointer for validity.
This is part of the fix to a security issue, XSA-55.
Additional change in 4.1 backport:
* ELF_PRPTRVAL needs to be defined oddly on 4.1 and earlier because
Xen's headers provide no definitions of uintptr_t or PRIuPTR.
Conflicts:
* Callers of elf_load_binary don't check its return value in 4.1.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 49 ++++++++--
tools/libxc/xc_hvm_build.c | 10 +-
xen/arch/x86/domain_build.c | 3 +-
xen/common/libelf/libelf-dominfo.c | 2 +-
xen/common/libelf/libelf-loader.c | 16 ++--
xen/common/libelf/libelf-private.h | 13 +++
xen/common/libelf/libelf-tools.c | 106 +++++++++++++++++++-
xen/include/xen/libelf.h | 188 +++++++++++++++++++++++++-----------
8 files changed, 303 insertions(+), 84 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 0fd8c42..b10790a 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -130,20 +130,30 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
if ( load )
{
- size_t allow_size; /* will be used in a forthcoming XSA-55 patch */
+ char *hdr_ptr;
+ size_t allow_size;
+
if ( !dom->bsd_symtab_start )
return 0;
size = dom->kernel_seg.vend - dom->bsd_symtab_start;
- hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
- *(int *)hdr = size - sizeof(int);
+ hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
+ elf->caller_xdest_base = hdr_ptr;
+ elf->caller_xdest_size = allow_size;
+ hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
+ elf_store_val(elf, int, hdr, size - sizeof(int));
}
else
{
+ char *hdr_ptr;
+
size = sizeof(int) + elf_size(elf, elf->ehdr) +
elf_shdr_count(elf) * elf_size(elf, shdr);
- hdr = xc_dom_malloc(dom, size);
- if ( hdr == NULL )
+ hdr_ptr = xc_dom_malloc(dom, size);
+ if ( hdr_ptr == NULL )
return 0;
+ elf->caller_xdest_base = hdr_ptr;
+ elf->caller_xdest_size = size;
+ hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
}
@@ -171,9 +181,32 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
ehdr->e_shoff = elf_size(elf, elf->ehdr);
ehdr->e_shstrndx = SHN_UNDEF;
}
- if ( elf_init(&syms, hdr + sizeof(int), size - sizeof(int)) )
+ if ( elf->caller_xdest_size < sizeof(int) )
+ {
+ DOMPRINTF("%s/%s: header size %"PRIx64" too small",
+ __FUNCTION__, load ? "load" : "parse",
+ (uint64_t)elf->caller_xdest_size);
+ return -1;
+ }
+ if ( elf_init(&syms, elf->caller_xdest_base + sizeof(int),
+ elf->caller_xdest_size - sizeof(int)) )
return -1;
+ /*
+ * The caller_xdest_{base,size} and dest_{base,size} need to
+ * remain valid so long as each struct elf_image does. The
+ * principle we adopt is that these values are set when the
+ * memory is allocated or mapped, and cleared when (and if)
+ * they are unmapped.
+ *
+ * Mappings of the guest are normally undone by xc_dom_unmap_all
+ * (directly or via xc_dom_release). We do not explicitly clear
+ * these because in fact that happens only at the end of
+ * xc_dom_boot_image, at which time all of these ELF loading
+ * functions have returned. No relevant struct elf_binary*
+ * escapes this file.
+ */
+
xc_elf_set_logfile(dom->xch, &syms, 1);
symtab = dom->bsd_symtab_start + sizeof(int);
@@ -304,8 +337,10 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
static int xc_dom_load_elf_kernel(struct xc_dom_image *dom)
{
struct elf_binary *elf = dom->private_loader;
+ xen_pfn_t pages;
- elf->dest = xc_dom_seg_to_ptr(dom, &dom->kernel_seg);
+ elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages);
+ elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
elf_load_binary(elf);
if ( dom->parms.bsd_symtab )
xc_dom_load_elf_symtab(dom, elf, 1);
diff --git a/tools/libxc/xc_hvm_build.c b/tools/libxc/xc_hvm_build.c
index 4af08c7..e3efca9 100644
--- a/tools/libxc/xc_hvm_build.c
+++ b/tools/libxc/xc_hvm_build.c
@@ -99,18 +99,20 @@ static int loadelfimage(
for ( i = 0; i < pages; i++ )
entries[i].mfn = parray[(elf->pstart >> PAGE_SHIFT) + i];
- elf->dest = xc_map_foreign_ranges(
+ elf->dest_base = xc_map_foreign_ranges(
xch, dom, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT,
entries, pages);
- if ( elf->dest == NULL )
+ if ( elf->dest_base == NULL )
goto err;
+ elf->dest_size = pages * PAGE_SIZE;
/* Load the initial elf image. */
elf_load_binary(elf);
rc = 0;
- munmap(elf->dest, pages << PAGE_SHIFT);
- elf->dest = NULL;
+ munmap(elf->dest_base, pages << PAGE_SHIFT);
+ elf->dest_base = NULL;
+ elf->dest_size = 0;
err:
free(entries);
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 93215d2..6467c36 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -895,7 +895,8 @@ int __init construct_dom0(
write_ptbase(v);
/* Copy the OS image and free temporary buffer. */
- elf.dest = (void*)vkern_start;
+ elf.dest_base = (void*)vkern_start;
+ elf.dest_size = vkern_end - vkern_start;
elf_load_binary(&elf);
bootstrap_map(NULL);
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 60673cd..a145360 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -244,7 +244,7 @@ int elf_xen_parse_guest_info(struct elf_binary *elf,
int len;
h = parms->guest_info;
-#define STAR(h) (*(h))
+#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
while ( STAR(h) )
{
elf_memset_unchecked(name, 0, sizeof(name));
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 5c27696..7bf5e33 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -20,23 +20,25 @@
/* ------------------------------------------------------------------------ */
-int elf_init(struct elf_binary *elf, const char *image, size_t size)
+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) )
+ if ( !elf_is_elfbinary(image_input) )
{
elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__);
return -1;
}
elf_memset_unchecked(elf, 0, sizeof(*elf));
- elf->image = image;
+ elf->image_base = image_input;
elf->size = size;
- elf->ehdr = (elf_ehdr *)image;
- elf->class = elf->ehdr->e32.e_ident[EI_CLASS];
- elf->data = elf->ehdr->e32.e_ident[EI_DATA];
+ elf->ehdr = ELF_MAKE_HANDLE(elf_ehdr, (elf_ptrval)image_input);
+ elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
+ elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
+ elf->caller_xdest_base = NULL;
+ elf->caller_xdest_size = 0;
/* Sanity check phdr. */
offset = elf_uval(elf, elf->ehdr, e_phoff) +
@@ -269,7 +271,7 @@ void elf_load_binary(struct elf_binary *elf)
ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr)
{
- return elf->dest + addr - elf->pstart;
+ return ELF_REALPTR2PTRVAL(elf->dest_base) + addr - elf->pstart;
}
uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol)
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 3ef753c..280dfd1 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -86,6 +86,19 @@ do { strncpy((d),(s),sizeof((d))-1); \
#endif
+#undef memcpy
+#undef memset
+#undef memmove
+#undef strcpy
+
+#define memcpy MISTAKE_unspecified_memcpy
+#define memset MISTAKE_unspecified_memset
+#define memmove MISTAKE_unspecified_memmove
+#define strcpy MISTAKE_unspecified_strcpy
+ /* This prevents libelf from using these undecorated versions
+ * of memcpy, memset, memmove and strcpy. Every call site
+ * must either use elf_mem*_unchecked, or elf_mem*_safe. */
+
#endif /* __LIBELF_PRIVATE_H_ */
/*
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 628c159..4a893f7 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -20,28 +20,100 @@
/* ------------------------------------------------------------------------ */
-uint64_t elf_access_unsigned(struct elf_binary * elf, const void *ptr,
- uint64_t offset, size_t size)
+void elf_mark_broken(struct elf_binary *elf, const char *msg)
{
+ if ( elf->broken == NULL )
+ elf->broken = msg;
+}
+
+const char *elf_check_broken(const struct elf_binary *elf)
+{
+ return elf->broken;
+}
+
+static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
+ const void *region, uint64_t regionsize)
+ /*
+ * Returns true if the putative memory area [ptrval,ptrval+size>
+ * is completely inside the region [region,region+regionsize>.
+ *
+ * ptrval and size are the untrusted inputs to be checked.
+ * region and regionsize are trusted and must be correct and valid,
+ * although it is OK for region to perhaps be maliciously NULL
+ * (but not some other malicious value).
+ */
+{
+ elf_ptrval regionp = (elf_ptrval)region;
+
+ if ( (region == NULL) ||
+ (ptrval < regionp) || /* start is before region */
+ (ptrval > regionp + regionsize) || /* start is after region */
+ (size > regionsize - (ptrval - regionp)) ) /* too big */
+ return 0;
+ return 1;
+}
+
+int elf_access_ok(struct elf_binary * elf,
+ uint64_t ptrval, size_t size)
+{
+ if ( elf_ptrval_in_range(ptrval, size, elf->image_base, elf->size) )
+ return 1;
+ if ( elf_ptrval_in_range(ptrval, size, elf->dest_base, elf->dest_size) )
+ return 1;
+ if ( elf_ptrval_in_range(ptrval, size,
+ elf->caller_xdest_base, elf->caller_xdest_size) )
+ return 1;
+ elf_mark_broken(elf, "out of range access");
+ return 0;
+}
+
+void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst,
+ elf_ptrval src, size_t size)
+{
+ if ( elf_access_ok(elf, dst, size) &&
+ elf_access_ok(elf, src, size) )
+ {
+ /* use memmove because these checks do not prove that the
+ * regions don't overlap and overlapping regions grant
+ * permission for compiler malice */
+ elf_memmove_unchecked(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src), size);
+ }
+}
+
+void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size)
+{
+ if ( elf_access_ok(elf, dst, size) )
+ {
+ elf_memset_unchecked(ELF_UNSAFE_PTR(dst), c, size);
+ }
+}
+
+uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
+ uint64_t moreoffset, size_t size)
+{
+ elf_ptrval ptrval = base + moreoffset;
int need_swap = elf_swap(elf);
const uint8_t *u8;
const uint16_t *u16;
const uint32_t *u32;
const uint64_t *u64;
+ if ( !elf_access_ok(elf, ptrval, size) )
+ return 0;
+
switch ( size )
{
case 1:
- u8 = ptr + offset;
+ u8 = (const void*)ptrval;
return *u8;
case 2:
- u16 = ptr + offset;
+ u16 = (const void*)ptrval;
return need_swap ? bswap_16(*u16) : *u16;
case 4:
- u32 = ptr + offset;
+ u32 = (const void*)ptrval;
return need_swap ? bswap_32(*u32) : *u32;
case 8:
- u64 = ptr + offset;
+ u64 = (const void*)ptrval;
return need_swap ? bswap_64(*u64) : *u64;
default:
return 0;
@@ -122,6 +194,28 @@ const char *elf_section_name(struct elf_binary *elf,
return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name));
}
+const char *elf_strval(struct elf_binary *elf, elf_ptrval start)
+{
+ uint64_t length;
+
+ for ( length = 0; ; length++ ) {
+ if ( !elf_access_ok(elf, start + length, 1) )
+ return NULL;
+ if ( !elf_access_unsigned(elf, start, length, 1) )
+ /* ok */
+ return ELF_UNSAFE_PTR(start);
+ }
+}
+
+const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start)
+{
+ const char *str = elf_strval(elf, start);
+
+ if ( str == NULL )
+ return "(invalid)";
+ return str;
+}
+
ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
{
return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset);
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 7101064..827fcfd 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -57,8 +57,9 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
* on this.
* This replaces variables which were char*,void*
* and their const versions, so we provide four
- * different declaration macros:
+ * different obsolete declaration macros:
* ELF_PTRVAL_{,CONST}{VOID,CHAR}
+ * New code can simply use the elf_ptrval typedef.
* HANDLE A pointer to a struct. There is one of these types
* for each pointer type - that is, for each "structname".
* In the arguments to the various HANDLE macros, structname
@@ -67,61 +68,65 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
* pointers. In the current code attempts to do so will
* compile, but in the next patch this will become a
* compile error.
- * We provide two declaration macros for const and
- * non-const pointers.
+ * We also provide a second declaration macro for
+ * pointers which were to const; this is obsolete.
*/
#ifdef __XEN__
# ifdef __i386__
typedef uint32_t elf_uintptr_t;
+# define ELF_PRPTRVAL PRIu32
# else
typedef uint64_t elf_uintptr_t;
+# define ELF_PRPTRVAL PRIu64
# endif
#else
typedef uintptr_t elf_uintptr_t;
+# define ELF_PRPTRVAL PRIuPTR
#endif
-#define ELF_REALPTR2PTRVAL(realpointer) (realpointer)
+typedef elf_uintptr_t elf_ptrval;
+
+#define ELF_REALPTR2PTRVAL(realpointer) ((elf_ptrval)(realpointer))
/* Converts an actual C pointer into a PTRVAL */
-#define ELF_HANDLE_DECL_NONCONST(structname) structname *
-#define ELF_HANDLE_DECL(structname) const structname *
+#define ELF_HANDLE_DECL_NONCONST(structname) structname##_handle /*obsolete*/
+#define ELF_HANDLE_DECL(structname) structname##_handle
/* Provides a type declaration for a HANDLE. */
- /* May only be used to declare ONE variable at a time */
-#define ELF_PTRVAL_VOID void *
-#define ELF_PTRVAL_CHAR char *
-#define ELF_PTRVAL_CONST_VOID const void *
-#define ELF_PTRVAL_CONST_CHAR const char *
- /* Provides a type declaration for a PTRVAL. */
- /* May only be used to declare ONE variable at a time */
+#define ELF_PTRVAL_VOID elf_ptrval /*obsolete*/
+#define ELF_PTRVAL_CHAR elf_ptrval /*obsolete*/
+#define ELF_PTRVAL_CONST_VOID elf_ptrval /*obsolete*/
+#define ELF_PTRVAL_CONST_CHAR elf_ptrval /*obsolete*/
-#define ELF_DEFINE_HANDLE(structname) /* empty */
+#define ELF_DEFINE_HANDLE(structname) \
+ typedef union { \
+ elf_ptrval ptrval; \
+ const structname *typeonly; /* for sizeof, offsetof, &c only */ \
+ } structname##_handle;
/*
* This must be invoked for each HANDLE type to define
* the actual C type used for that kind of HANDLE.
*/
-#define ELF_PRPTRVAL "p"
- /* printf format a la PRId... for a PTRVAL */
-
-#define ELF_MAKE_HANDLE(structname, ptrval) (ptrval)
+#define ELF_MAKE_HANDLE(structname, ptrval) ((structname##_handle){ ptrval })
/* Converts a PTRVAL to a HANDLE */
-#define ELF_IMAGE_BASE(elf) ((elf)->image)
+#define ELF_IMAGE_BASE(elf) ((elf_ptrval)(elf)->image_base)
/* Returns the base of the image as a PTRVAL. */
-#define ELF_HANDLE_PTRVAL(handleval) ((void*)(handleval))
+#define ELF_HANDLE_PTRVAL(handleval) ((handleval).ptrval)
/* Converts a HANDLE to a PTRVAL. */
-#define ELF_OBSOLETE_VOIDP_CAST (void*)(elf_uintptr_t)
+#define ELF_OBSOLETE_VOIDP_CAST /*empty*/
/*
- * In some places the existing code needs to
+ * In some places the old code used to need to
* - cast away const (the existing code uses const a fair
* bit but actually sometimes wants to write to its input)
* from a PTRVAL.
* - convert an integer representing a pointer to a PTRVAL
- * This macro provides a suitable cast.
+ * Nowadays all of these re uintptr_ts so there is no const problem
+ * and no need for any casting.
*/
#define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_uintptr_t)(ptrval))
@@ -132,18 +137,21 @@ typedef uintptr_t elf_uintptr_t;
*/
/* PTRVALs can be INVALID (ie, NULL). */
-#define ELF_INVALID_PTRVAL (NULL) /* returns NULL PTRVAL */
+#define ELF_INVALID_PTRVAL ((elf_ptrval)0) /* returns NULL PTRVAL */
#define ELF_INVALID_HANDLE(structname) /* returns NULL handle */ \
ELF_MAKE_HANDLE(structname, ELF_INVALID_PTRVAL)
-#define ELF_PTRVAL_VALID(ptrval) (ptrval) /* } */
-#define ELF_HANDLE_VALID(handleval) (handleval) /* } predicates */
-#define ELF_PTRVAL_INVALID(ptrval) ((ptrval) == NULL) /* } */
+#define ELF_PTRVAL_VALID(ptrval) (!!(ptrval)) /* } */
+#define ELF_HANDLE_VALID(handleval) (!!(handleval).ptrval) /* } predicates */
+#define ELF_PTRVAL_INVALID(ptrval) (!ELF_PTRVAL_VALID((ptrval))) /* } */
+
+#define ELF_MAX_PTRVAL (~(elf_ptrval)0)
+ /* PTRVAL value guaranteed to compare > to any valid PTRVAL */
/* For internal use by other macros here */
#define ELF__HANDLE_FIELD_TYPE(handleval, elm) \
- typeof((handleval)->elm)
+ typeof((handleval).typeonly->elm)
#define ELF__HANDLE_FIELD_OFFSET(handleval, elm) \
- offsetof(typeof(*(handleval)),elm)
+ offsetof(typeof(*(handleval).typeonly),elm)
/* ------------------------------------------------------------------------ */
@@ -192,7 +200,7 @@ ELF_DEFINE_HANDLE(elf_note)
struct elf_binary {
/* elf binary */
- const char *image;
+ const void *image_base;
size_t size;
char class;
char data;
@@ -200,10 +208,16 @@ struct elf_binary {
ELF_HANDLE_DECL(elf_ehdr) ehdr;
ELF_PTRVAL_CONST_CHAR sec_strtab;
ELF_HANDLE_DECL(elf_shdr) sym_tab;
- ELF_PTRVAL_CONST_CHAR sym_strtab;
+ uint64_t sym_strtab;
/* loaded to */
- char *dest;
+ /*
+ * dest_base and dest_size are trusted and must be correct;
+ * whenever dest_size is not 0, both of these must be valid
+ * so long as the struct elf_binary is in use.
+ */
+ char *dest_base;
+ size_t dest_size;
uint64_t pstart;
uint64_t pend;
uint64_t reloc_offset;
@@ -211,12 +225,22 @@ struct elf_binary {
uint64_t bsd_symtab_pstart;
uint64_t bsd_symtab_pend;
+ /*
+ * caller's other acceptable destination
+ *
+ * Again, these are trusted and must be valid (or 0) so long
+ * as the struct elf_binary is in use.
+ */
+ void *caller_xdest_base;
+ uint64_t caller_xdest_size;
+
#ifndef __XEN__
/* misc */
elf_log_callback *log_callback;
void *log_caller_data;
#endif
int verbose;
+ const char *broken;
};
/* ------------------------------------------------------------------------ */
@@ -234,22 +258,27 @@ struct elf_binary {
#define elf_lsb(elf) (ELFDATA2LSB == (elf)->data)
#define elf_swap(elf) (NATIVE_ELFDATA != (elf)->data)
-#define elf_uval(elf, str, elem) \
- ((ELFCLASS64 == (elf)->class) \
- ? elf_access_unsigned((elf), (str), \
- offsetof(typeof(*(str)),e64.elem), \
- sizeof((str)->e64.elem)) \
- : elf_access_unsigned((elf), (str), \
- offsetof(typeof(*(str)),e32.elem), \
- sizeof((str)->e32.elem)))
+#define elf_uval_3264(elf, handle, elem) \
+ elf_access_unsigned((elf), (handle).ptrval, \
+ offsetof(typeof(*(handle).typeonly),elem), \
+ sizeof((handle).typeonly->elem))
+
+#define elf_uval(elf, handle, elem) \
+ ((ELFCLASS64 == (elf)->class) \
+ ? elf_uval_3264(elf, handle, e64.elem) \
+ : elf_uval_3264(elf, handle, e32.elem))
/*
* Reads an unsigned field in a header structure in the ELF.
* str is a HANDLE, and elem is the field name in it.
*/
-#define elf_size(elf, str) \
+
+#define elf_size(elf, handle_or_handletype) ({ \
+ typeof(handle_or_handletype) elf_size__dummy; \
((ELFCLASS64 == (elf)->class) \
- ? sizeof((str)->e64) : sizeof((str)->e32))
+ ? sizeof(elf_size__dummy.typeonly->e64) \
+ : sizeof(elf_size__dummy.typeonly->e32)); \
+})
/*
* Returns the size of the substructure for the appropriate 32/64-bitness.
* str should be a HANDLE.
@@ -261,23 +290,37 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr,
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
+const char *elf_strval(struct elf_binary *elf, elf_ptrval start);
+ /* may return NULL if the string is out of range etc. */
-#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */
-#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */
+const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start);
+ /* like elf_strval but returns "(invalid)" instead of NULL */
-#define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz))
-#define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz))
+void elf_memcpy_safe(struct elf_binary*, elf_ptrval dst, elf_ptrval src, size_t);
+void elf_memset_safe(struct elf_binary*, elf_ptrval dst, int c, size_t);
/*
- * Versions of memcpy and memset which will (in the next patch)
- * arrange never to write outside permitted areas.
+ * Versions of memcpy and memset which arrange never to write
+ * outside permitted areas.
*/
-#define elf_store_val(elf, type, ptr, val) (*(type*)(ptr) = (val))
+int elf_access_ok(struct elf_binary * elf,
+ uint64_t ptrval, size_t size);
+
+#define elf_store_val(elf, type, ptr, val) \
+ ({ \
+ typeof(type) elf_store__val = (val); \
+ elf_ptrval elf_store__targ = ptr; \
+ if (elf_access_ok((elf), elf_store__targ, \
+ sizeof(elf_store__val))) { \
+ elf_memcpy_unchecked((void*)elf_store__targ, &elf_store__val, \
+ sizeof(elf_store__val)); \
+ } \
+ }) \
/* Stores a value at a particular PTRVAL. */
-#define elf_store_field(elf, hdr, elm, val) \
- (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \
- &((hdr)->elm), \
+#define elf_store_field(elf, hdr, elm, val) \
+ (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \
+ ELF_HANDLE_PTRVAL(hdr) + ELF__HANDLE_FIELD_OFFSET(hdr, elm), \
(val)))
/* Stores a 32/64-bit field. hdr is a HANDLE and elm is the field name. */
@@ -314,6 +357,10 @@ int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
/* xc_libelf_loader.c */
int elf_init(struct elf_binary *elf, const char *image, size_t size);
+ /*
+ * image and size must be correct. They will be recorded in
+ * *elf, and must remain valid while the elf is in use.
+ */
#ifdef __XEN__
void elf_set_verbose(struct elf_binary *elf);
#else
@@ -329,6 +376,9 @@ uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol);
void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */
+void elf_mark_broken(struct elf_binary *elf, const char *msg);
+const char *elf_check_broken(const struct elf_binary *elf); /* NULL means OK */
+
/* ------------------------------------------------------------------------ */
/* xc_libelf_relocate.c */
@@ -403,16 +453,38 @@ int elf_xen_parse_guest_info(struct elf_binary *elf,
int elf_xen_parse(struct elf_binary *elf,
struct elf_dom_parms *parms);
-#define elf_memcpy_unchecked memcpy
-#define elf_memset_unchecked memset
+static inline void *elf_memcpy_unchecked(void *dest, const void *src, size_t n)
+ { return memcpy(dest, src, n); }
+static inline void *elf_memmove_unchecked(void *dest, const void *src, size_t n)
+ { return memmove(dest, src, n); }
+static inline void *elf_memset_unchecked(void *s, int c, size_t n)
+ { return memset(s, c, n); }
/*
- * Unsafe versions of memcpy and memset which take actual C
- * pointers. These are just like real memcpy and memset.
+ * Unsafe versions of memcpy, memmove memset which take actual C
+ * pointers. These are just like the real functions.
+ * We provide these so that in libelf-private.h we can #define
+ * memcpy, memset and memmove to undefined MISTAKE things.
*/
-#define ELF_ADVANCE_DEST(elf, amount) elf->dest += (amount)
- /* Advances past amount bytes of the current destination area. */
+/* Advances past amount bytes of the current destination area. */
+static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount)
+{
+ if ( elf->dest_base == NULL )
+ {
+ elf_mark_broken(elf, "advancing in null image");
+ }
+ else if ( elf->dest_size >= amount )
+ {
+ elf->dest_base += amount;
+ elf->dest_size -= amount;
+ }
+ else
+ {
+ elf->dest_size = 0;
+ elf_mark_broken(elf, "advancing past end (image very short?)");
+ }
+}
#endif /* __XEN_LIBELF_H__ */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 10/21] libelf: Check pointer references in elf_is_elfbinary
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (8 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 09/21] libelf: check all pointer accesses Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 11/21] libelf: Make all callers call elf_check_broken Ian Jackson
` (10 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
elf_is_elfbinary didn't take a length parameter and could potentially
access out of range when provided with a very short image.
We only need to check the size is enough for the actual dereference in
elf_is_elfbinary; callers are just using it to check the magic number
and do their own checks (usually via the new elf_ptrval system) before
dereferencing other parts of the header.
This is part of the fix to a security issue, XSA-55.
Conflicts in 4.1 backport:
* xen/arch/x86/bzimage.c in 4.1 doesn't use elf_is_elfbinary.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxc/xc_dom_elfloader.c | 2 +-
xen/common/libelf/libelf-loader.c | 2 +-
xen/common/libelf/libelf-tools.c | 9 ++++++---
xen/include/xen/libelf.h | 4 +++-
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index b10790a..945df7a 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -95,7 +95,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/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 7bf5e33..96b0fe5 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -25,7 +25,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 4a893f7..3419f0c 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -311,11 +311,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);
}
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 827fcfd..8698f67 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -350,7 +350,9 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_
uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
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);
+/* (Only) checks that the image has the right magic number. */
+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);
/* ------------------------------------------------------------------------ */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/21] libelf: Make all callers call elf_check_broken
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (9 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 10/21] libelf: Check pointer references in elf_is_elfbinary Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 12/21] libelf: use C99 bool for booleans Ian Jackson
` (9 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
This arranges that if the new pointer reference error checking
tripped, we actually get a message about it. In this patch these
messages do not change the actual return values from the various
functions: so pointer reference errors do not prevent loading. This
is for fear that some existing kernels might cause the code to make
these wild references, which would then break, which is not a good
thing in a security patch.
In xen/arch/x86/domain_build.c we have to introduce an "out" label and
change all of the "return rc" beyond the relevant point into "goto
out".
This is part of the fix to a security issue, XSA-55.
Differences in 4.1 backport:
* No xen/arch/arm.
* There was less error handling in xen/arch/x86/domain_build.c
so less need to change it.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 22 +++++++++++++++++++---
tools/libxc/xc_hvm_build.c | 5 +++++
tools/xcutils/readnotes.c | 3 +++
xen/arch/x86/domain_build.c | 23 ++++++++++++++++++-----
4 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 945df7a..e733afd 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -276,6 +276,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
elf_store_field(elf, shdr, e32.sh_name, 0);
}
+ if ( elf_check_broken(&syms) )
+ DOMPRINTF("%s: symbols ELF broken: %s", __FUNCTION__,
+ elf_check_broken(&syms));
+ if ( elf_check_broken(elf) )
+ DOMPRINTF("%s: ELF broken: %s", __FUNCTION__,
+ elf_check_broken(elf));
+
if ( tables == 0 )
{
DOMPRINTF("%s: no symbol table present", __FUNCTION__);
@@ -312,13 +319,16 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
{
xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
" has no shstrtab", __FUNCTION__);
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
/* parse binary and get xen meta info */
elf_parse_binary(elf);
if ( (rc = elf_xen_parse(elf, &dom->parms)) != 0 )
- return rc;
+ {
+ goto out;
+ }
/* find kernel segment */
dom->kernel_seg.vstart = dom->parms.virt_kstart;
@@ -331,7 +341,13 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
__FUNCTION__, dom->guest_type,
dom->kernel_seg.vstart, dom->kernel_seg.vend);
- return 0;
+ rc = 0;
+out:
+ if ( elf_check_broken(elf) )
+ DOMPRINTF("%s: ELF broken: %s", __FUNCTION__,
+ elf_check_broken(elf));
+
+ return rc;
}
static int xc_dom_load_elf_kernel(struct xc_dom_image *dom)
diff --git a/tools/libxc/xc_hvm_build.c b/tools/libxc/xc_hvm_build.c
index e3efca9..6c4e41e 100644
--- a/tools/libxc/xc_hvm_build.c
+++ b/tools/libxc/xc_hvm_build.c
@@ -393,11 +393,16 @@ static int setup_guest(xc_interface *xch,
munmap(page0, PAGE_SIZE);
}
+ if ( elf_check_broken(&elf) )
+ ERROR("HVM ELF broken: %s", elf_check_broken(&elf));
+
free(page_array);
return 0;
error_out:
free(page_array);
+ if ( elf_check_broken(&elf) )
+ ERROR("HVM ELF broken, failing: %s", elf_check_broken(&elf));
return -1;
}
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index d462f80..b940a36 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -224,6 +224,9 @@ int main(int argc, char **argv)
printf("__xen_guest: %s\n",
elf_strfmt(&elf, elf_section_start(&elf, shdr)));
+ if (elf_check_broken(&elf))
+ printf("warning: broken ELF: %s\n", elf_check_broken(&elf));
+
return 0;
}
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 6467c36..49bb6a5 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -374,7 +374,7 @@ int __init construct_dom0(
#endif
elf_parse_binary(&elf);
if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
- return rc;
+ goto out;
/* compatibility check */
compatible = 0;
@@ -413,7 +413,8 @@ int __init construct_dom0(
if ( !compatible )
{
printk("Mismatch between Xen and DOM0 kernel\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
#if defined(__x86_64__)
@@ -727,7 +728,8 @@ int __init construct_dom0(
(v_end > HYPERVISOR_COMPAT_VIRT_START(d)) )
{
printk("DOM0 image overlaps with Xen private area.\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
if ( is_pv_32on64_domain(d) )
@@ -907,7 +909,8 @@ int __init construct_dom0(
{
write_ptbase(current);
printk("Invalid HYPERCALL_PAGE field in ELF notes.\n");
- return -1;
+ rc = -1;
+ goto out;
}
hypercall_page_initialise(
d, (void *)(unsigned long)parms.virt_hypercall);
@@ -1254,9 +1257,19 @@ int __init construct_dom0(
BUG_ON(rc != 0);
- iommu_dom0_init(dom0);
+ if ( elf_check_broken(&elf) )
+ printk(" Xen warning: dom0 kernel broken ELF: %s\n",
+ elf_check_broken(&elf));
+ iommu_dom0_init(dom0);
return 0;
+
+out:
+ if ( elf_check_broken(&elf) )
+ printk(" Xen dom0 kernel broken ELF: %s\n",
+ elf_check_broken(&elf));
+
+ return rc;
}
/*
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 12/21] libelf: use C99 bool for booleans
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (10 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 11/21] libelf: Make all callers call elf_check_broken Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 13/21] libelf: use only unsigned integers Ian Jackson
` (8 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
We want to remove uses of "int" because signed integers have
undesirable undefined behaviours on overflow. Malicious compilers can
turn apparently-correct code into code with security vulnerabilities
etc.
In this patch we change all the booleans in libelf to C99 bool,
from <stdbool.h>.
For the one visible libelf boolean in libxc's public interface we
retain the use of int to avoid changing the ABI; libxc converts it to
a bool for consumption by libelf.
It is OK to change all values only ever used as booleans to _Bool
(bool) because conversion from any scalar type to a _Bool works the
same as the boolean test in if() or ?: and is always defined (C99
6.3.1.2). But we do need to check that all these variables really are
only ever used that way. (It is theoretically possible that the old
code truncated some 64-bit values to 32-bit ints which might become
zero depending on the value, which would mean a behavioural change in
this patch, but it seems implausible that treating 0x????????00000000
as false could have been intended.)
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 8 ++++----
xen/common/libelf/libelf-dominfo.c | 2 +-
xen/common/libelf/libelf-loader.c | 4 ++--
xen/common/libelf/libelf-private.h | 2 +-
xen/common/libelf/libelf-tools.c | 10 +++++-----
xen/include/xen/libelf.h | 18 ++++++++++--------
6 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index e733afd..c3da5b9 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -34,7 +34,7 @@
/* ------------------------------------------------------------------------ */
static void log_callback(struct elf_binary *elf, void *caller_data,
- int iserr, const char *fmt, va_list al) {
+ bool iserr, const char *fmt, va_list al) {
xc_interface *xch = caller_data;
xc_reportv(xch,
@@ -46,7 +46,7 @@ static void log_callback(struct elf_binary *elf, void *caller_data,
void xc_elf_set_logfile(xc_interface *xch, struct elf_binary *elf,
int verbose) {
- elf_set_log(elf, log_callback, xch, verbose);
+ elf_set_log(elf, log_callback, xch, verbose /* convert to bool */);
}
/* ------------------------------------------------------------------------ */
@@ -84,7 +84,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
/* ------------------------------------------------------------------------ */
/* parse elf binary */
-static int check_elf_kernel(struct xc_dom_image *dom, int verbose)
+static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
{
if ( dom->kernel_blob == NULL )
{
@@ -112,7 +112,7 @@ static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
}
static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
- struct elf_binary *elf, int load)
+ struct elf_binary *elf, bool load)
{
struct elf_binary syms;
ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index a145360..3a0db85 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -99,7 +99,7 @@ int elf_xen_parse_note(struct elf_binary *elf,
/* *INDENT-OFF* */
static const struct {
char *name;
- int str;
+ bool str;
} note_desc[] = {
[XEN_ELFNOTE_ENTRY] = { "ENTRY", 0},
[XEN_ELFNOTE_HYPERCALL_PAGE] = { "HYPERCALL_PAGE", 0},
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 96b0fe5..5895a6b 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -88,7 +88,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size)
}
#ifndef __XEN__
-void elf_call_log_callback(struct elf_binary *elf, int iserr,
+void elf_call_log_callback(struct elf_binary *elf, bool iserr,
const char *fmt,...) {
va_list al;
@@ -103,7 +103,7 @@ void elf_call_log_callback(struct elf_binary *elf, int iserr,
}
void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback,
- void *log_caller_data, int verbose)
+ void *log_caller_data, bool verbose)
{
elf->log_callback = log_callback;
elf->log_caller_data = log_caller_data;
diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
index 280dfd1..277be04 100644
--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -77,7 +77,7 @@
#define elf_err(elf, fmt, args ... ) \
elf_call_log_callback(elf, 1, fmt , ## args );
-void elf_call_log_callback(struct elf_binary*, int iserr, const char *fmt,...);
+void elf_call_log_callback(struct elf_binary*, bool iserr, const char *fmt,...);
#define safe_strcpy(d,s) \
do { strncpy((d),(s),sizeof((d))-1); \
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 3419f0c..78b2f99 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -31,7 +31,7 @@ const char *elf_check_broken(const struct elf_binary *elf)
return elf->broken;
}
-static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
+static bool elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
const void *region, uint64_t regionsize)
/*
* Returns true if the putative memory area [ptrval,ptrval+size>
@@ -53,7 +53,7 @@ static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
return 1;
}
-int elf_access_ok(struct elf_binary * elf,
+bool elf_access_ok(struct elf_binary * elf,
uint64_t ptrval, size_t size)
{
if ( elf_ptrval_in_range(ptrval, size, elf->image_base, elf->size) )
@@ -92,7 +92,7 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
uint64_t moreoffset, size_t size)
{
elf_ptrval ptrval = base + moreoffset;
- int need_swap = elf_swap(elf);
+ bool need_swap = elf_swap(elf);
const uint8_t *u8;
const uint16_t *u16;
const uint32_t *u32;
@@ -311,7 +311,7 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
/* ------------------------------------------------------------------------ */
-int elf_is_elfbinary(const void *image_start, size_t image_size)
+bool elf_is_elfbinary(const void *image_start, size_t image_size)
{
const Elf32_Ehdr *ehdr = image_start;
@@ -321,7 +321,7 @@ int elf_is_elfbinary(const void *image_start, size_t image_size)
return IS_ELF(*ehdr);
}
-int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
+bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
uint64_t p_type = elf_uval(elf, phdr, p_type);
uint64_t p_flags = elf_uval(elf, phdr, p_flags);
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 8698f67..152a33d 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -29,6 +29,8 @@
#error define architectural endianness
#endif
+#include <stdbool.h>
+
#undef ELFSIZE
#include "elfstructs.h"
#ifdef __XEN__
@@ -42,7 +44,7 @@
struct elf_binary;
typedef void elf_log_callback(struct elf_binary*, void *caller_data,
- int iserr, const char *fmt, va_list al);
+ bool iserr, const char *fmt, va_list al);
#endif
@@ -239,7 +241,7 @@ struct elf_binary {
elf_log_callback *log_callback;
void *log_caller_data;
#endif
- int verbose;
+ bool verbose;
const char *broken;
};
@@ -303,8 +305,8 @@ void elf_memset_safe(struct elf_binary*, elf_ptrval dst, int c, size_t);
* outside permitted areas.
*/
-int elf_access_ok(struct elf_binary * elf,
- uint64_t ptrval, size_t size);
+bool elf_access_ok(struct elf_binary * elf,
+ uint64_t ptrval, size_t size);
#define elf_store_val(elf, type, ptr, val) \
({ \
@@ -351,9 +353,9 @@ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
/* (Only) checks that the image has the right magic number. */
-int elf_is_elfbinary(const void *image_start, size_t image_size);
+bool 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);
+bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
/* ------------------------------------------------------------------------ */
/* xc_libelf_loader.c */
@@ -367,7 +369,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size);
void elf_set_verbose(struct elf_binary *elf);
#else
void elf_set_log(struct elf_binary *elf, elf_log_callback*,
- void *log_caller_pointer, int verbose);
+ void *log_caller_pointer, bool verbose);
#endif
void elf_parse_binary(struct elf_binary *elf);
@@ -419,7 +421,7 @@ struct elf_dom_parms {
char xen_ver[16];
char loader[16];
int pae;
- int bsd_symtab;
+ bool bsd_symtab;
uint64_t virt_base;
uint64_t virt_entry;
uint64_t virt_hypercall;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 13/21] libelf: use only unsigned integers
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (11 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 12/21] libelf: use C99 bool for booleans Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:29 ` George Dunlap
2013-06-12 16:00 ` [PATCH 14/21] libxc: Introduce xc_bitops.h Ian Jackson
` (7 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Signed integers have undesirable undefined behaviours on overflow.
Malicious compilers can turn apparently-correct code into code with
security vulnerabilities etc.
So use only unsigned integers. Exceptions are booleans (which we have
already changed) and error codes.
We _do_ change all the chars which aren't fixed constants from our own
text segment, but not the char*s. This is because it is safe to
access an arbitrary byte through a char*, but not necessarily safe to
convert an arbitrary value to a char.
As a consequence we need to compile libelf with -Wno-pointer-sign.
It is OK to change all the signed integers to unsigned because all the
inequalities in libelf are in contexts where we don't "expect"
negative numbers.
In libelf-dominfo.c:elf_xen_parse we rename a variable "rc" to
"more_notes" as it actually contains a note count derived from the
input image. The "error" return value from elf_xen_parse_notes is
changed from -1 to ~0U.
grepping shows only one occurrence of "PRId" or "%d" or "%ld" in
libelf and xc_dom_elfloader.c (a "%d" which becomes "%u").
This is part of the fix to a security issue, XSA-55.
Conflicts in 4.1 series:
* xc_dom_load_elf_kernel has no rc variable to change.
* elf_load_image doesn't exist.
For those concerned about unintentional functional changes, the
following rune produces a version of the patch which is much smaller
and eliminates only non-functional changes:
GIT_EXTERNAL_DIFF=.../unsigned-differ git-diff <before>..<after>
where <before> and <after> are git refs for the code before and after
this patch, and unsigned-differ is this shell script:
#!/bin/bash
set -e
seddery () {
perl -pe 's/\b(?:elf_errorstatus|elf_negerrnoval)\b/int/g'
}
path="$1"
in="$2"
out="$5"
set +e
diff -pu --label "$path~" <(seddery <"$in") --label "$path" <(seddery <"$out")
rc=$?
set -e
if [ $rc = 1 ]; then rc=0; fi
exit $rc
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/Makefile | 9 +++++-
tools/libxc/xc_dom.h | 7 +++--
tools/libxc/xc_dom_elfloader.c | 40 +++++++++++++++------------
tools/xcutils/readnotes.c | 15 +++++-----
xen/common/libelf/Makefile | 2 +
xen/common/libelf/libelf-dominfo.c | 52 ++++++++++++++++++-----------------
xen/common/libelf/libelf-loader.c | 12 ++++----
xen/common/libelf/libelf-tools.c | 22 +++++++-------
xen/include/xen/libelf.h | 19 +++++++-----
9 files changed, 98 insertions(+), 80 deletions(-)
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 6f5c307..139b791 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -49,8 +49,13 @@ GUEST_SRCS-$(CONFIG_HVM) += xc_hvm_build.c
vpath %.c ../../xen/common/libelf
CFLAGS += -I../../xen/common/libelf
-GUEST_SRCS-y += libelf-tools.c libelf-loader.c
-GUEST_SRCS-y += libelf-dominfo.c
+ELF_SRCS-y += libelf-tools.c libelf-loader.c
+ELF_SRCS-y += libelf-dominfo.c
+
+GUEST_SRCS-y += $(ELF_SRCS-y)
+
+$(patsubst %.c,%.o,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
+$(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
# new domain builder
GUEST_SRCS-y += xc_dom_core.c xc_dom_boot.c
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 6b118ad..9dffbe6 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -135,9 +135,10 @@ struct xc_dom_image {
struct xc_dom_loader {
char *name;
- int (*probe) (struct xc_dom_image * dom);
- int (*parser) (struct xc_dom_image * dom);
- int (*loader) (struct xc_dom_image * dom);
+ /* Sadly the error returns from these functions are not consistent: */
+ elf_negerrnoval (*probe) (struct xc_dom_image * dom);
+ elf_negerrnoval (*parser) (struct xc_dom_image * dom);
+ elf_errorstatus (*loader) (struct xc_dom_image * dom);
struct xc_dom_loader *next;
};
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index c3da5b9..2d25824 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -84,7 +84,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
/* ------------------------------------------------------------------------ */
/* parse elf binary */
-static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
+static elf_negerrnoval check_elf_kernel(struct xc_dom_image *dom, bool verbose)
{
if ( dom->kernel_blob == NULL )
{
@@ -106,12 +106,12 @@ static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
return 0;
}
-static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
+static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
{
return check_elf_kernel(dom, 0);
}
-static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
+static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
struct elf_binary *elf, bool load)
{
struct elf_binary syms;
@@ -119,7 +119,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
xen_vaddr_t symtab, maxaddr;
ELF_PTRVAL_CHAR hdr;
size_t size;
- int h, count, type, i, tables = 0;
+ unsigned h, count, type, i, tables = 0;
if ( elf_swap(elf) )
{
@@ -140,13 +140,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
elf->caller_xdest_base = hdr_ptr;
elf->caller_xdest_size = allow_size;
hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
- elf_store_val(elf, int, hdr, size - sizeof(int));
+ elf_store_val(elf, unsigned, hdr, size - sizeof(unsigned));
}
else
{
char *hdr_ptr;
- size = sizeof(int) + elf_size(elf, elf->ehdr) +
+ size = sizeof(unsigned) + elf_size(elf, elf->ehdr) +
elf_shdr_count(elf) * elf_size(elf, shdr);
hdr_ptr = xc_dom_malloc(dom, size);
if ( hdr_ptr == NULL )
@@ -157,15 +157,15 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
}
- elf_memcpy_safe(elf, hdr + sizeof(int),
+ elf_memcpy_safe(elf, hdr + sizeof(unsigned),
ELF_IMAGE_BASE(elf),
elf_size(elf, elf->ehdr));
- elf_memcpy_safe(elf, hdr + sizeof(int) + elf_size(elf, elf->ehdr),
+ elf_memcpy_safe(elf, hdr + sizeof(unsigned) + elf_size(elf, elf->ehdr),
ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
elf_shdr_count(elf) * elf_size(elf, shdr));
if ( elf_64bit(elf) )
{
- Elf64_Ehdr *ehdr = (Elf64_Ehdr *)(hdr + sizeof(int));
+ Elf64_Ehdr *ehdr = (Elf64_Ehdr *)(hdr + sizeof(unsigned));
ehdr->e_phoff = 0;
ehdr->e_phentsize = 0;
ehdr->e_phnum = 0;
@@ -174,22 +174,22 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
}
else
{
- Elf32_Ehdr *ehdr = (Elf32_Ehdr *)(hdr + sizeof(int));
+ Elf32_Ehdr *ehdr = (Elf32_Ehdr *)(hdr + sizeof(unsigned));
ehdr->e_phoff = 0;
ehdr->e_phentsize = 0;
ehdr->e_phnum = 0;
ehdr->e_shoff = elf_size(elf, elf->ehdr);
ehdr->e_shstrndx = SHN_UNDEF;
}
- if ( elf->caller_xdest_size < sizeof(int) )
+ if ( elf->caller_xdest_size < sizeof(unsigned) )
{
DOMPRINTF("%s/%s: header size %"PRIx64" too small",
__FUNCTION__, load ? "load" : "parse",
(uint64_t)elf->caller_xdest_size);
return -1;
}
- if ( elf_init(&syms, elf->caller_xdest_base + sizeof(int),
- elf->caller_xdest_size - sizeof(int)) )
+ if ( elf_init(&syms, elf->caller_xdest_base + sizeof(unsigned),
+ elf->caller_xdest_size - sizeof(unsigned)) )
return -1;
/*
@@ -209,7 +209,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
xc_elf_set_logfile(dom->xch, &syms, 1);
- symtab = dom->bsd_symtab_start + sizeof(int);
+ symtab = dom->bsd_symtab_start + sizeof(unsigned);
maxaddr = elf_round_up(&syms, symtab + elf_size(&syms, syms.ehdr) +
elf_shdr_count(&syms) * elf_size(&syms, shdr));
@@ -255,7 +255,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
size = elf_uval(&syms, shdr, sh_size);
maxaddr = elf_round_up(&syms, maxaddr + size);
tables++;
- DOMPRINTF("%s: h=%d %s, size=0x%zx, maxaddr=0x%" PRIx64 "",
+ DOMPRINTF("%s: h=%u %s, size=0x%zx, maxaddr=0x%" PRIx64 "",
__FUNCTION__, h,
type == SHT_SYMTAB ? "symtab" : "strtab",
size, maxaddr);
@@ -294,10 +294,14 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
return 0;
}
-static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
+static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
+ /*
+ * This function sometimes returns -1 for error and sometimes
+ * an errno value. WTF?
+ */
{
struct elf_binary *elf;
- int rc;
+ elf_errorstatus rc;
rc = check_elf_kernel(dom, 1);
if ( rc != 0 )
@@ -350,7 +354,7 @@ out:
return rc;
}
-static int xc_dom_load_elf_kernel(struct xc_dom_image *dom)
+static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom)
{
struct elf_binary *elf = dom->private_loader;
xen_pfn_t pages;
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index b940a36..543e0f5 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -28,7 +28,7 @@ static void print_numeric_note(const char *prefix, struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note)
{
uint64_t value = elf_note_numeric(elf, note);
- int descsz = elf_uval(elf, note, descsz);
+ unsigned descsz = elf_uval(elf, note, descsz);
printf("%s: %#*" PRIx64 " (%d bytes)\n",
prefix, 2+2*descsz, value, descsz);
@@ -37,7 +37,7 @@ static void print_numeric_note(const char *prefix, struct elf_binary *elf,
static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note)
{
- int descsz = elf_uval(elf, note, descsz);
+ unsigned descsz = elf_uval(elf, note, descsz);
ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
/* XXX should be able to cope with a list of values. */
@@ -57,10 +57,10 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
}
-static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end)
+static unsigned print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end)
{
ELF_HANDLE_DECL(elf_note) note;
- int notes_found = 0;
+ unsigned notes_found = 0;
const char *this_note_name;
for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) )
@@ -119,7 +119,7 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
break;
default:
printf("unknown note type %#x\n",
- (int)elf_uval(elf, note, type));
+ (unsigned)elf_uval(elf, note, type));
break;
}
}
@@ -129,12 +129,13 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
int main(int argc, char **argv)
{
const char *f;
- int fd,h,size,usize,count;
+ int fd;
+ unsigned h,size,usize,count;
void *image,*tmp;
struct stat st;
struct elf_binary elf;
ELF_HANDLE_DECL(elf_shdr) shdr;
- int notes_found = 0;
+ unsigned notes_found = 0;
if (argc != 2)
{
diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile
index 854e738..e788705 100644
--- a/xen/common/libelf/Makefile
+++ b/xen/common/libelf/Makefile
@@ -2,6 +2,8 @@ obj-y := libelf.o
SECTIONS := text data rodata $(foreach n,1 2 4 8,rodata.str1.$(n)) $(foreach r,rel rel.ro,data.$(r) data.$(r).local)
+CFLAGS += -Wno-pointer-sign
+
libelf.o: libelf-temp.o Makefile
$(OBJCOPY) $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 3a0db85..6054e40 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -28,15 +28,15 @@ static const char *const elf_xen_feature_names[] = {
[XENFEAT_supervisor_mode_kernel] = "supervisor_mode_kernel",
[XENFEAT_pae_pgdir_above_4gb] = "pae_pgdir_above_4gb"
};
-static const int elf_xen_features =
+static const unsigned elf_xen_features =
sizeof(elf_xen_feature_names) / sizeof(elf_xen_feature_names[0]);
-int elf_xen_parse_features(const char *features,
+elf_errorstatus elf_xen_parse_features(const char *features,
uint32_t *supported,
uint32_t *required)
{
- char feature[64];
- int pos, len, i;
+ unsigned char feature[64];
+ unsigned pos, len, i;
if ( features == NULL )
return 0;
@@ -92,7 +92,7 @@ int elf_xen_parse_features(const char *features,
/* ------------------------------------------------------------------------ */
/* xen elf notes */
-int elf_xen_parse_note(struct elf_binary *elf,
+elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
struct elf_dom_parms *parms,
ELF_HANDLE_DECL(elf_note) note)
{
@@ -121,7 +121,7 @@ int elf_xen_parse_note(struct elf_binary *elf,
const char *str = NULL;
uint64_t val = 0;
- int type = elf_uval(elf, note, type);
+ unsigned type = elf_uval(elf, note, type);
if ( (type >= sizeof(note_desc) / sizeof(note_desc[0])) ||
(note_desc[type].name == NULL) )
@@ -206,12 +206,14 @@ int elf_xen_parse_note(struct elf_binary *elf,
return 0;
}
-static int elf_xen_parse_notes(struct elf_binary *elf,
+#define ELF_NOTE_INVALID (~0U)
+
+static unsigned elf_xen_parse_notes(struct elf_binary *elf,
struct elf_dom_parms *parms,
ELF_PTRVAL_CONST_VOID start,
ELF_PTRVAL_CONST_VOID end)
{
- int xen_elfnotes = 0;
+ unsigned xen_elfnotes = 0;
ELF_HANDLE_DECL(elf_note) note;
const char *note_name;
@@ -227,7 +229,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
if ( strcmp(note_name, "Xen") )
continue;
if ( elf_xen_parse_note(elf, parms, note) )
- return -1;
+ return ELF_NOTE_INVALID;
xen_elfnotes++;
}
return xen_elfnotes;
@@ -236,12 +238,12 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
/* ------------------------------------------------------------------------ */
/* __xen_guest section */
-int elf_xen_parse_guest_info(struct elf_binary *elf,
+elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
ELF_PTRVAL_CONST_CHAR h;
- char name[32], value[128];
- int len;
+ unsigned char name[32], value[128];
+ unsigned len;
h = parms->guest_info;
#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1))
@@ -324,13 +326,13 @@ int elf_xen_parse_guest_info(struct elf_binary *elf,
/* ------------------------------------------------------------------------ */
/* sanity checks */
-static int elf_xen_note_check(struct elf_binary *elf,
+static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) &&
(ELF_PTRVAL_INVALID(parms->guest_info)) )
{
- int machine = elf_uval(elf, elf->ehdr, e_machine);
+ unsigned machine = elf_uval(elf, elf->ehdr, e_machine);
if ( (machine == EM_386) || (machine == EM_X86_64) )
{
elf_err(elf, "%s: ERROR: Not a Xen-ELF image: "
@@ -362,7 +364,7 @@ static int elf_xen_note_check(struct elf_binary *elf,
return 0;
}
-static int elf_xen_addr_calc_check(struct elf_binary *elf,
+static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
if ( (parms->elf_paddr_offset != UNSET_ADDR) &&
@@ -448,13 +450,13 @@ static int elf_xen_addr_calc_check(struct elf_binary *elf,
/* ------------------------------------------------------------------------ */
/* glue it all together ... */
-int elf_xen_parse(struct elf_binary *elf,
+elf_errorstatus elf_xen_parse(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
ELF_HANDLE_DECL(elf_shdr) shdr;
ELF_HANDLE_DECL(elf_phdr) phdr;
- int xen_elfnotes = 0;
- int i, count, rc;
+ unsigned xen_elfnotes = 0;
+ unsigned i, count, more_notes;
elf_memset_unchecked(parms, 0, sizeof(*parms));
parms->virt_base = UNSET_ADDR;
@@ -479,13 +481,13 @@ int elf_xen_parse(struct elf_binary *elf,
if (elf_uval(elf, phdr, p_offset) == 0)
continue;
- rc = elf_xen_parse_notes(elf, parms,
+ more_notes = elf_xen_parse_notes(elf, parms,
elf_segment_start(elf, phdr),
elf_segment_end(elf, phdr));
- if ( rc == -1 )
+ if ( more_notes == ELF_NOTE_INVALID )
return -1;
- xen_elfnotes += rc;
+ xen_elfnotes += more_notes;
}
/*
@@ -502,17 +504,17 @@ int elf_xen_parse(struct elf_binary *elf,
if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
continue;
- rc = elf_xen_parse_notes(elf, parms,
+ more_notes = elf_xen_parse_notes(elf, parms,
elf_section_start(elf, shdr),
elf_section_end(elf, shdr));
- if ( rc == -1 )
+ if ( more_notes == ELF_NOTE_INVALID )
return -1;
- if ( xen_elfnotes == 0 && rc > 0 )
+ if ( xen_elfnotes == 0 && more_notes > 0 )
elf_msg(elf, "%s: using notes from SHT_NOTE section\n", __FUNCTION__);
- xen_elfnotes += rc;
+ xen_elfnotes += more_notes;
}
}
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 5895a6b..459c973 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -20,7 +20,7 @@
/* ------------------------------------------------------------------------ */
-int elf_init(struct elf_binary *elf, const char *image_input, size_t size)
+elf_errorstatus 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;
@@ -121,7 +121,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
{
uint64_t sz;
ELF_HANDLE_DECL(elf_shdr) shdr;
- int i, type;
+ unsigned i, type;
if ( !ELF_HANDLE_VALID(elf->sym_tab) )
return;
@@ -157,7 +157,7 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
ELF_PTRVAL_VOID symbase;
ELF_PTRVAL_VOID symtab_addr;
ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr;
- int i, type;
+ unsigned i, type;
if ( !elf->bsd_symtab_pstart )
return;
@@ -190,7 +190,7 @@ do { \
elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr),
ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
sz);
- maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz);
+ maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz);
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
@@ -203,10 +203,10 @@ do { \
elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz);
/* Mangled to be based on ELF header location. */
elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
- maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz);
+ maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz);
}
shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) +
- (long)elf_uval(elf, elf->ehdr, e_shentsize));
+ (unsigned long)elf_uval(elf, elf->ehdr, e_shentsize));
}
/* Write down the actual sym size. */
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 78b2f99..4fb2d38 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -122,19 +122,19 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
{
- int elf_round = (elf_64bit(elf) ? 8 : 4) - 1;
+ uint64_t elf_round = (elf_64bit(elf) ? 8 : 4) - 1;
return (addr + elf_round) & ~elf_round;
}
/* ------------------------------------------------------------------------ */
-int elf_shdr_count(struct elf_binary *elf)
+unsigned elf_shdr_count(struct elf_binary *elf)
{
return elf_uval(elf, elf->ehdr, e_shnum);
}
-int elf_phdr_count(struct elf_binary *elf)
+unsigned elf_phdr_count(struct elf_binary *elf)
{
return elf_uval(elf, elf->ehdr, e_phnum);
}
@@ -144,7 +144,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
uint64_t count = elf_shdr_count(elf);
ELF_HANDLE_DECL(elf_shdr) shdr;
const char *sname;
- int i;
+ unsigned i;
for ( i = 0; i < count; i++ )
{
@@ -156,7 +156,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
return ELF_INVALID_HANDLE(elf_shdr);
}
-ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index)
{
uint64_t count = elf_shdr_count(elf);
ELF_PTRVAL_CONST_VOID ptr;
@@ -170,7 +170,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index)
return ELF_MAKE_HANDLE(elf_shdr, ptr);
}
-ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index)
{
uint64_t count = elf_uval(elf, elf->ehdr, e_phnum);
ELF_PTRVAL_CONST_VOID ptr;
@@ -264,7 +264,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
return ELF_INVALID_HANDLE(elf_sym);
}
-ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index)
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index)
{
ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab);
ELF_HANDLE_DECL(elf_sym) sym;
@@ -280,7 +280,7 @@ const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note
ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- int namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
+ unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz;
}
@@ -288,7 +288,7 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_
uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
- int descsz = elf_uval(elf, note, descsz);
+ unsigned descsz = elf_uval(elf, note, descsz);
switch (descsz)
{
@@ -303,8 +303,8 @@ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note
}
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- int namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
- int descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
+ unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
+ unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz);
}
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 152a33d..9e709c2 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -31,6 +31,9 @@
#include <stdbool.h>
+typedef int elf_errorstatus; /* 0: ok; -ve (normally -1): error */
+typedef int elf_negerrnoval; /* 0: ok; -EFOO: error */
+
#undef ELFSIZE
#include "elfstructs.h"
#ifdef __XEN__
@@ -330,12 +333,12 @@ bool elf_access_ok(struct elf_binary * elf,
/* ------------------------------------------------------------------------ */
/* xc_libelf_tools.c */
-int elf_shdr_count(struct elf_binary *elf);
-int elf_phdr_count(struct elf_binary *elf);
+unsigned elf_shdr_count(struct elf_binary *elf);
+unsigned elf_phdr_count(struct elf_binary *elf);
ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name);
-ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index);
-ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index);
+ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index);
+ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index);
const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */
ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
@@ -345,7 +348,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(
ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol);
-ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index);
+ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index);
const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */
ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
@@ -360,7 +363,7 @@ bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr
/* ------------------------------------------------------------------------ */
/* xc_libelf_loader.c */
-int elf_init(struct elf_binary *elf, const char *image, size_t size);
+elf_errorstatus elf_init(struct elf_binary *elf, const char *image, size_t size);
/*
* image and size must be correct. They will be recorded in
* *elf, and must remain valid while the elf is in use.
@@ -386,7 +389,7 @@ const char *elf_check_broken(const struct elf_binary *elf); /* NULL means OK */
/* ------------------------------------------------------------------------ */
/* xc_libelf_relocate.c */
-int elf_reloc(struct elf_binary *elf);
+elf_errorstatus elf_reloc(struct elf_binary *elf);
/* ------------------------------------------------------------------------ */
/* xc_libelf_dominfo.c */
@@ -420,7 +423,7 @@ struct elf_dom_parms {
char guest_ver[16];
char xen_ver[16];
char loader[16];
- int pae;
+ int pae; /* some kind of enum apparently */
bool bsd_symtab;
uint64_t virt_base;
uint64_t virt_entry;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 13/21] libelf: use only unsigned integers
2013-06-12 16:00 ` [PATCH 13/21] libelf: use only unsigned integers Ian Jackson
@ 2013-06-12 16:29 ` George Dunlap
2013-06-12 17:48 ` AL13N
0 siblings, 1 reply; 32+ messages in thread
From: George Dunlap @ 2013-06-12 16:29 UTC (permalink / raw)
To: Ian Jackson
Cc: Andrew Cooper, xen-devel@lists.xensource.com, mattjd, security
On Wed, Jun 12, 2013 at 5:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> +static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> + /*
> + * This function sometimes returns -1 for error and sometimes
> + * an errno value. WTF?
> + */
It just occured to me that technically, the 'F' in "WTF" is a swear
word; as such someone might consider it unprofessional to have in the
source code.
Either way (with or without the comment changed):
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 13/21] libelf: use only unsigned integers
2013-06-12 16:29 ` George Dunlap
@ 2013-06-12 17:48 ` AL13N
2013-06-13 9:51 ` George Dunlap
2013-06-13 13:33 ` Ian Jackson
0 siblings, 2 replies; 32+ messages in thread
From: AL13N @ 2013-06-12 17:48 UTC (permalink / raw)
To: xen-devel
Cc: xen-devel@lists.xensource.com, George Dunlap, Andrew Cooper,
mattjd, Ian Jackson, security
Op woensdag 12 juni 2013 17:29:43 schreef George Dunlap:
> It just occured to me that technically, the 'F' in "WTF" is a swear
> word; as such someone might consider it unprofessional to have in the
> source code.
lol, have you ever seen the wordcount of the linux kernel?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 13/21] libelf: use only unsigned integers
2013-06-12 17:48 ` AL13N
@ 2013-06-13 9:51 ` George Dunlap
2013-06-13 13:33 ` Ian Jackson
1 sibling, 0 replies; 32+ messages in thread
From: George Dunlap @ 2013-06-13 9:51 UTC (permalink / raw)
To: AL13N
Cc: xen-devel@lists.xensource.com, Andrew Cooper, mattjd, Ian Jackson,
xen-devel@lists.xen.org, security
On Wed, Jun 12, 2013 at 6:48 PM, AL13N <alien@rmail.be> wrote:
> Op woensdag 12 juni 2013 17:29:43 schreef George Dunlap:
>> It just occured to me that technically, the 'F' in "WTF" is a swear
>> word; as such someone might consider it unprofessional to have in the
>> source code.
>
> lol, have you ever seen the wordcount of the linux kernel?
Each project has to set its own standard. :-) In any case, I just
wanted to make sure that we were consciously choosing the standard,
not doing it by accident.
I have a mild preference for s/WTF?/?!?/;, but I'm not super-fussed about it.
-George
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 13/21] libelf: use only unsigned integers
2013-06-12 17:48 ` AL13N
2013-06-13 9:51 ` George Dunlap
@ 2013-06-13 13:33 ` Ian Jackson
1 sibling, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-13 13:33 UTC (permalink / raw)
To: AL13N
Cc: xen-devel@lists.xensource.com, George Dunlap, Andrew Cooper,
mattjd, xen-devel, security
AL13N writes ("Re: [Xen-devel] [PATCH 13/21] libelf: use only unsigned integers"):
> Op woensdag 12 juni 2013 17:29:43 schreef George Dunlap:
> > It just occured to me that technically, the 'F' in "WTF" is a swear
> > word; as such someone might consider it unprofessional to have in the
> > source code.
>
> lol, have you ever seen the wordcount of the linux kernel?
I'm not sure that we necessarily want to regard lkml's "community
standards" on this kind of question as exemplary. I will change the
comment.
Ian.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 14/21] libxc: Introduce xc_bitops.h
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (12 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 13/21] libelf: use only unsigned integers Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 15/21] libelf: check loops for running away Ian Jackson
` (6 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Copy the one file tools/libxc/xc_bitops.h from xen.git#aa1355f9.
We will need this for the next patch, which calls for a bitmap in
libxc.
xc_bitops.h was introduced to unify various existing sets of bitmap
operations. In this patch we backport only the introduction, not the
replacement of the other instances. So we introduce another instance
Sorry :-/.
This is part of the fix to a security issue, XSA-55.
This patch is unique to the Xen 4.1 version of the XSA-55 series.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_bitops.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 63 insertions(+), 0 deletions(-)
create mode 100644 tools/libxc/xc_bitops.h
diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
new file mode 100644
index 0000000..d8e0c16
--- /dev/null
+++ b/tools/libxc/xc_bitops.h
@@ -0,0 +1,63 @@
+#ifndef XC_BITOPS_H
+#define XC_BITOPS_H 1
+
+/* bitmap operations for single threaded access */
+
+#include <stdlib.h>
+#include <string.h>
+
+#define BITS_PER_LONG (sizeof(unsigned long) * 8)
+#define ORDER_LONG (sizeof(unsigned long) == 4 ? 5 : 6)
+
+#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
+#define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
+
+/* calculate required space for number of longs needed to hold nr_bits */
+static inline int bitmap_size(int nr_bits)
+{
+ int nr_long, nr_bytes;
+ nr_long = (nr_bits + BITS_PER_LONG - 1) >> ORDER_LONG;
+ nr_bytes = nr_long * sizeof(unsigned long);
+ return nr_bytes;
+}
+
+static inline unsigned long *bitmap_alloc(int nr_bits)
+{
+ return calloc(1, bitmap_size(nr_bits));
+}
+
+static inline void bitmap_clear(unsigned long *addr, int nr_bits)
+{
+ memset(addr, 0, bitmap_size(nr_bits));
+}
+
+static inline int test_bit(int nr, unsigned long *addr)
+{
+ return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1;
+}
+
+static inline void clear_bit(int nr, unsigned long *addr)
+{
+ BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr));
+}
+
+static inline void set_bit(int nr, unsigned long *addr)
+{
+ BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr));
+}
+
+static inline int test_and_clear_bit(int nr, unsigned long *addr)
+{
+ int oldbit = test_bit(nr, addr);
+ clear_bit(nr, addr);
+ return oldbit;
+}
+
+static inline int test_and_set_bit(int nr, unsigned long *addr)
+{
+ int oldbit = test_bit(nr, addr);
+ set_bit(nr, addr);
+ return oldbit;
+}
+
+#endif /* XC_BITOPS_H */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 15/21] libelf: check loops for running away
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (13 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 14/21] libxc: Introduce xc_bitops.h Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-13 16:47 ` George Dunlap
2013-06-12 16:00 ` [PATCH 16/21] libelf: abolish obsolete macros Ian Jackson
` (5 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Ensure that libelf does not have any loops which can run away
indefinitely even if the input is bogus. (Grepped for \bfor, \bwhile
and \bgoto in libelf and xc_dom_*loader*.c.)
Changes needed:
* elf_note_next uses the note's unchecked alleged length, which might
wrap round. If it does, return ELF_MAX_PTRVAL (0xfff..fff) instead,
which will be beyond the end of the section and so terminate the
caller's loop.
* In various loops over section and program headers, check that the
calculated header pointer is still within the image, and quit the
loop if it isn't.
We have not changed loops which might, in principle, iterate over the
whole image - even if they might do so one byte at a time with a
nontrivial access check function in the middle.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 33 ++++++++++++++++++------
xen/common/libelf/libelf-dominfo.c | 48 ++++++++++++++++++++++++------------
xen/common/libelf/libelf-loader.c | 47 +++++++++++++++++++++++++++++++++-
xen/common/libelf/libelf-tools.c | 28 +++++++++++++++++++-
xen/include/xen/libelf.h | 3 ++
5 files changed, 130 insertions(+), 29 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 2d25824..b7da0b5 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -28,6 +28,7 @@
#include "xg_private.h"
#include "xc_dom.h"
+#include "xc_bitops.h"
#define XEN_VER "xen-3.0"
@@ -120,6 +121,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
ELF_PTRVAL_CHAR hdr;
size_t size;
unsigned h, count, type, i, tables = 0;
+ unsigned long *strtab_referenced = NULL;
if ( elf_swap(elf) )
{
@@ -220,22 +222,35 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
symtab, maxaddr);
count = elf_shdr_count(&syms);
+ /* elf_shdr_count guarantees that count is reasonable */
+
+ strtab_referenced = xc_dom_malloc(dom, bitmap_size(count));
+ if ( strtab_referenced == NULL )
+ return -1;
+ bitmap_clear(strtab_referenced, count);
+ /* Note the symtabs @h linked to by any strtab @i. */
+ for ( i = 0; i < count; i++ )
+ {
+ shdr2 = elf_shdr_by_index(&syms, i);
+ if ( elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB )
+ {
+ h = elf_uval(&syms, shdr2, sh_link);
+ if (h < count)
+ set_bit(h, strtab_referenced);
+ }
+ }
+
for ( h = 0; h < count; h++ )
{
shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+ /* input has an insane section header count field */
+ break;
type = elf_uval(&syms, shdr, sh_type);
if ( type == SHT_STRTAB )
{
- /* Look for a strtab @i linked to symtab @h. */
- for ( i = 0; i < count; i++ )
- {
- shdr2 = elf_shdr_by_index(&syms, i);
- if ( (elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB) &&
- (elf_uval(&syms, shdr2, sh_link) == h) )
- break;
- }
/* Skip symtab @h if we found no corresponding strtab @i. */
- if ( i == count )
+ if ( !test_bit(h, strtab_referenced) )
{
if ( elf_64bit(&syms) )
elf_store_field(elf, shdr, e64.sh_offset, 0);
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 6054e40..8f6bdec 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -211,7 +211,8 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
static unsigned elf_xen_parse_notes(struct elf_binary *elf,
struct elf_dom_parms *parms,
ELF_PTRVAL_CONST_VOID start,
- ELF_PTRVAL_CONST_VOID end)
+ ELF_PTRVAL_CONST_VOID end,
+ unsigned *total_note_count)
{
unsigned xen_elfnotes = 0;
ELF_HANDLE_DECL(elf_note) note;
@@ -223,6 +224,12 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
note = elf_note_next(elf, note) )
{
+ if ( *total_note_count >= ELF_MAX_TOTAL_NOTE_COUNT )
+ {
+ elf_mark_broken(elf, "too many ELF notes");
+ break;
+ }
+ (*total_note_count)++;
note_name = elf_note_name(elf, note);
if ( note_name == NULL )
continue;
@@ -457,6 +464,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_phdr) phdr;
unsigned xen_elfnotes = 0;
unsigned i, count, more_notes;
+ unsigned total_note_count = 0;
elf_memset_unchecked(parms, 0, sizeof(*parms));
parms->virt_base = UNSET_ADDR;
@@ -471,6 +479,13 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
for ( i = 0; i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
+ /*
+ * This test also arranges for the loop to terminate if the
+ * input file has a ridiculous value for the header count: The
+ * first putative header outside the input image will appear
+ * to have type 0 (since out-of-range accesses read as 0) and
+ * PT_NOTE != 0.
+ */
if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
continue;
@@ -483,7 +498,8 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
more_notes = elf_xen_parse_notes(elf, parms,
elf_segment_start(elf, phdr),
- elf_segment_end(elf, phdr));
+ elf_segment_end(elf, phdr),
+ &total_note_count);
if ( more_notes == ELF_NOTE_INVALID )
return -1;
@@ -501,12 +517,17 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
{
shdr = elf_shdr_by_index(elf, i);
+ /*
+ * See above re guarantee of loop termination.
+ * SHT_NOTE != 0.
+ */
if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
continue;
more_notes = elf_xen_parse_notes(elf, parms,
elf_section_start(elf, shdr),
- elf_section_end(elf, shdr));
+ elf_section_end(elf, shdr),
+ &total_note_count);
if ( more_notes == ELF_NOTE_INVALID )
return -1;
@@ -524,20 +545,15 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
*/
if ( xen_elfnotes == 0 )
{
- count = elf_shdr_count(elf);
- for ( i = 0; i < count; i++ )
+ shdr = elf_shdr_by_name(elf, "__xen_guest");
+ if ( ELF_HANDLE_VALID(shdr) )
{
- shdr = elf_shdr_by_name(elf, "__xen_guest");
- if ( ELF_HANDLE_VALID(shdr) )
- {
- parms->guest_info = elf_section_start(elf, shdr);
- parms->elf_note_start = ELF_INVALID_PTRVAL;
- parms->elf_note_end = ELF_INVALID_PTRVAL;
- elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
- elf_strfmt(elf, parms->guest_info));
- elf_xen_parse_guest_info(elf, parms);
- break;
- }
+ parms->guest_info = elf_section_start(elf, shdr);
+ parms->elf_note_start = ELF_INVALID_PTRVAL;
+ parms->elf_note_end = ELF_INVALID_PTRVAL;
+ elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
+ elf_strfmt(elf, parms->guest_info));
+ elf_xen_parse_guest_info(elf, parms);
}
}
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 459c973..118d5aa 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -71,6 +71,9 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t
for ( i = 0; i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+ /* input has an insane section header count field */
+ break;
if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB )
continue;
elf->sym_tab = shdr;
@@ -140,6 +143,9 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
shdr = elf_shdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+ /* input has an insane section header count field */
+ break;
type = elf_uval(elf, shdr, sh_type);
if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
@@ -194,6 +200,9 @@ do { \
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
+ elf_ptrval old_shdr_p;
+ elf_ptrval new_shdr_p;
+
type = elf_uval(elf, shdr, sh_type);
if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
{
@@ -205,8 +214,16 @@ do { \
elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz);
}
- shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) +
- (unsigned long)elf_uval(elf, elf->ehdr, e_shentsize));
+ old_shdr_p = ELF_HANDLE_PTRVAL(shdr);
+ new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize);
+ if ( new_shdr_p <= old_shdr_p ) /* wrapped or stuck */
+ {
+ elf_mark_broken(elf, "bad section header length");
+ break;
+ }
+ if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */
+ break;
+ shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p);
}
/* Write down the actual sym size. */
@@ -226,6 +243,9 @@ void elf_parse_binary(struct elf_binary *elf)
for ( i = 0; i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
+ /* input has an insane program header count field */
+ break;
if ( !elf_phdr_is_loadable(elf, phdr) )
continue;
paddr = elf_uval(elf, phdr, p_paddr);
@@ -248,11 +268,20 @@ void elf_load_binary(struct elf_binary *elf)
ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t i, count, paddr, offset, filesz, memsz;
ELF_PTRVAL_VOID dest;
+ /*
+ * Let bizarre ELFs write the output image up to twice; this
+ * calculation is just to ensure our copying loop is no worse than
+ * O(domain_size).
+ */
+ uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
count = elf_uval(elf, elf->ehdr, e_phnum);
for ( i = 0; i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
+ /* input has an insane program header count field */
+ break;
if ( !elf_phdr_is_loadable(elf, phdr) )
continue;
paddr = elf_uval(elf, phdr, p_paddr);
@@ -260,6 +289,20 @@ void elf_load_binary(struct elf_binary *elf)
filesz = elf_uval(elf, phdr, p_filesz);
memsz = elf_uval(elf, phdr, p_memsz);
dest = elf_get_ptr(elf, paddr);
+
+ /*
+ * We need to check that the input image doesn't have us copy
+ * the whole image zillions of times, as that could lead to
+ * O(n^2) time behaviour and possible DoS by a malicous ELF.
+ */
+ if ( remain_allow_copy < memsz )
+ {
+ elf_mark_broken(elf, "program segments total to more"
+ " than the input image size");
+ break;
+ }
+ remain_allow_copy -= memsz;
+
elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n",
__func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz));
elf_memcpy_safe(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz);
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 4fb2d38..238262b 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -131,7 +131,16 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr)
unsigned elf_shdr_count(struct elf_binary *elf)
{
- return elf_uval(elf, elf->ehdr, e_shnum);
+ unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
+ uint64_t max = elf->size / sizeof(Elf32_Shdr);
+ if (max > ~(unsigned)0)
+ max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
+ if (count > max)
+ {
+ elf_mark_broken(elf, "far too many section headers");
+ count = max;
+ }
+ return count;
}
unsigned elf_phdr_count(struct elf_binary *elf)
@@ -149,6 +158,9 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
for ( i = 0; i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+ /* input has an insane section header count field */
+ break;
sname = elf_section_name(elf, shdr);
if ( sname && !strcmp(sname, name) )
return shdr;
@@ -204,6 +216,11 @@ const char *elf_strval(struct elf_binary *elf, elf_ptrval start)
if ( !elf_access_unsigned(elf, start, length, 1) )
/* ok */
return ELF_UNSAFE_PTR(start);
+ if ( length >= ELF_MAX_STRING_LENGTH )
+ {
+ elf_mark_broken(elf, "excessively long string");
+ return NULL;
+ }
}
}
@@ -306,7 +323,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
- return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz);
+ elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note)
+ + elf_size(elf, note) + namesz + descsz;
+
+ if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */
+ !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) )
+ ptrval = ELF_MAX_PTRVAL; /* terminate caller's loop */
+
+ return ELF_MAKE_HANDLE(elf_note, ptrval);
}
/* ------------------------------------------------------------------------ */
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 9e709c2..738bf0a 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -51,6 +51,9 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
#endif
+#define ELF_MAX_STRING_LENGTH 4096
+#define ELF_MAX_TOTAL_NOTE_COUNT 65536
+
/* ------------------------------------------------------------------------ */
/* Macros for accessing the input image and output area. */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 15/21] libelf: check loops for running away
2013-06-12 16:00 ` [PATCH 15/21] libelf: check loops for running away Ian Jackson
@ 2013-06-13 16:47 ` George Dunlap
2013-06-13 17:34 ` Ian Jackson
0 siblings, 1 reply; 32+ messages in thread
From: George Dunlap @ 2013-06-13 16:47 UTC (permalink / raw)
To: Ian Jackson
Cc: Andrew Cooper, xen-devel@lists.xensource.com, mattjd, security
On Wed, Jun 12, 2013 at 5:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> Ensure that libelf does not have any loops which can run away
> indefinitely even if the input is bogus. (Grepped for \bfor, \bwhile
> and \bgoto in libelf and xc_dom_*loader*.c.)
>
> Changes needed:
> * elf_note_next uses the note's unchecked alleged length, which might
> wrap round. If it does, return ELF_MAX_PTRVAL (0xfff..fff) instead,
> which will be beyond the end of the section and so terminate the
> caller's loop.
> * In various loops over section and program headers, check that the
> calculated header pointer is still within the image, and quit the
> loop if it isn't.
>
> We have not changed loops which might, in principle, iterate over the
> whole image - even if they might do so one byte at a time with a
> nontrivial access check function in the middle.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> tools/libxc/xc_dom_elfloader.c | 33 ++++++++++++++++++------
> xen/common/libelf/libelf-dominfo.c | 48 ++++++++++++++++++++++++------------
> xen/common/libelf/libelf-loader.c | 47 +++++++++++++++++++++++++++++++++-
> xen/common/libelf/libelf-tools.c | 28 +++++++++++++++++++-
> xen/include/xen/libelf.h | 3 ++
> 5 files changed, 130 insertions(+), 29 deletions(-)
>
[snip]
> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
> index 6054e40..8f6bdec 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
[snip]
> @@ -471,6 +479,13 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
> for ( i = 0; i < count; i++ )
> {
> phdr = elf_phdr_by_index(elf, i);
> + /*
> + * This test also arranges for the loop to terminate if the
> + * input file has a ridiculous value for the header count: The
> + * first putative header outside the input image will appear
> + * to have type 0 (since out-of-range accesses read as 0) and
> + * PT_NOTE != 0.
> + */
> if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
> continue;
Sorry, run that by me again? It looks like it will call "continue" in
that case, not "break".
This looks inconsistent with the changes you made in libelf-loader.c
and xc_dom_elfloader.c: There we have similar types of reads with
continues, but you do an access check anyway.
Is there something special about phdr vs shdr?
>
> @@ -483,7 +498,8 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
>
> more_notes = elf_xen_parse_notes(elf, parms,
> elf_segment_start(elf, phdr),
> - elf_segment_end(elf, phdr));
> + elf_segment_end(elf, phdr),
> + &total_note_count);
> if ( more_notes == ELF_NOTE_INVALID )
> return -1;
>
> @@ -501,12 +517,17 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
> {
> shdr = elf_shdr_by_index(elf, i);
>
> + /*
> + * See above re guarantee of loop termination.
> + * SHT_NOTE != 0.
> + */
> if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
> continue;
Same as above.
> @@ -524,20 +545,15 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
> */
> if ( xen_elfnotes == 0 )
> {
> - count = elf_shdr_count(elf);
> - for ( i = 0; i < count; i++ )
> + shdr = elf_shdr_by_name(elf, "__xen_guest");
> + if ( ELF_HANDLE_VALID(shdr) )
> {
> - shdr = elf_shdr_by_name(elf, "__xen_guest");
> - if ( ELF_HANDLE_VALID(shdr) )
> - {
> - parms->guest_info = elf_section_start(elf, shdr);
> - parms->elf_note_start = ELF_INVALID_PTRVAL;
> - parms->elf_note_end = ELF_INVALID_PTRVAL;
> - elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> - elf_strfmt(elf, parms->guest_info));
> - elf_xen_parse_guest_info(elf, parms);
> - break;
> - }
> + parms->guest_info = elf_section_start(elf, shdr);
> + parms->elf_note_start = ELF_INVALID_PTRVAL;
> + parms->elf_note_end = ELF_INVALID_PTRVAL;
> + elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> + elf_strfmt(elf, parms->guest_info));
> + elf_xen_parse_guest_info(elf, parms);
> }
> }
Wow -- nice loop. Better check that one thing N times, just to be sure... :-)
> @@ -306,7 +323,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
> unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
> unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
>
> - return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz);
> + elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note)
> + + elf_size(elf, note) + namesz + descsz;
> +
> + if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */
> + !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) )
> + ptrval = ELF_MAX_PTRVAL; /* terminate caller's loop */
Is this an official part of the interface, or do you just "know" that
the two callers will happen to break out of their loop when this
happens?
If it's the second, this seems like a future bug waiting to happen...
Other than that, it looks like all the changes do what they say on the tin.
-George
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 15/21] libelf: check loops for running away
2013-06-13 16:47 ` George Dunlap
@ 2013-06-13 17:34 ` Ian Jackson
2013-06-13 17:54 ` Ian Jackson
0 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2013-06-13 17:34 UTC (permalink / raw)
To: George Dunlap
Cc: Andrew Cooper, xen-devel@lists.xensource.com, mattjd, security
George Dunlap writes ("Re: [Xen-devel] [PATCH 15/21] libelf: check loops for running away"):
> On Wed, Jun 12, 2013 at 5:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> > + /*
> > + * This test also arranges for the loop to terminate if the
> > + * input file has a ridiculous value for the header count: The
> > + * first putative header outside the input image will appear
> > + * to have type 0 (since out-of-range accesses read as 0) and
> > + * PT_NOTE != 0.
> > + */
> > if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
> > continue;
>
> Sorry, run that by me again? It looks like it will call "continue" in
> that case, not "break".
>
> This looks inconsistent with the changes you made in libelf-loader.c
> and xc_dom_elfloader.c: There we have similar types of reads with
> continues, but you do an access check anyway.
>
> Is there something special about phdr vs shdr?
You are entirely correct. I must have misread "continue" as "break".
> > + /*
> > + * See above re guarantee of loop termination.
> > + * SHT_NOTE != 0.
> > + */
> > if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
> > continue;
>
> Same as above.
Again.
I will fix these by changing them to use the same checks as elsewhere.
> > @@ -306,7 +323,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
> > unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
> > unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
> >
> > - return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz);
> > + elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note)
> > + + elf_size(elf, note) + namesz + descsz;
> > +
> > + if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */
> > + !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) )
> > + ptrval = ELF_MAX_PTRVAL; /* terminate caller's loop */
>
> Is this an official part of the interface, or do you just "know" that
> the two callers will happen to break out of their loop when this
> happens?
>
> If it's the second, this seems like a future bug waiting to happen...
I will add a comment, making it part of the declared interface.
Ian.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 15/21] libelf: check loops for running away
2013-06-13 17:34 ` Ian Jackson
@ 2013-06-13 17:54 ` Ian Jackson
0 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-13 17:54 UTC (permalink / raw)
To: George Dunlap, xen-devel@lists.xensource.com, Andrew Cooper,
mattjd, security
Ian Jackson writes ("Re: [Xen-devel] [PATCH 15/21] libelf: check loops for running away"):
> George Dunlap writes ("Re: [Xen-devel] [PATCH 15/21] libelf: check loops for running away"):
> > Is there something special about phdr vs shdr?
>
> You are entirely correct. I must have misread "continue" as "break".
...
> I will fix these by changing them to use the same checks as elsewhere.
Here's that diff, which I will fold into patch 15 in v8.
Ian.
commit 1c0f8cb92c604d92db9037b5aeadfeadf2197a05
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu Jun 13 18:47:46 2013 +0100
libelf-dominfo fix two loops
diff --git a/.topmsg b/.topmsg
index eed7c13..405c54f 100644
--- a/.topmsg
+++ b/.topmsg
@@ -32,6 +32,10 @@ This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
+v8: Fix the two loops in libelf-dominfo.c; the comment about
+ PT_NOTE and SHT_NOTE wasn't true because the checks did
+ "continue", not "break".
+
v5: Fix regression due to wrong image size loop limit calculation.
Check return value from xc_dom_malloc.
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index b0ba4d8..8ca2a33 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -495,13 +495,9 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
for ( i = 0; i < count; i++ )
{
phdr = elf_phdr_by_index(elf, i);
- /*
- * This test also arranges for the loop to terminate if the
- * input file has a ridiculous value for the header count: The
- * first putative header outside the input image will appear
- * to have type 0 (since out-of-range accesses read as 0) and
- * PT_NOTE != 0.
- */
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) )
+ /* input has an insane program header count field */
+ break;
if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
continue;
@@ -532,11 +528,10 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
for ( i = 0; i < count; i++ )
{
shdr = elf_shdr_by_index(elf, i);
+ if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+ /* input has an insane section header count field */
+ break;
- /*
- * See above re guarantee of loop termination.
- * SHT_NOTE != 0.
- */
if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
continue;
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 16/21] libelf: abolish obsolete macros
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (14 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 15/21] libelf: check loops for running away Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 17/21] libxc: Add range checking to xc_dom_binloader Ian Jackson
` (4 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
Abolish ELF_PTRVAL_[CONST_]{CHAR,VOID}; change uses to elf_ptrval.
Abolish ELF_HANDLE_DECL_NONCONST; change uses to ELF_HANDLE_DECL.
Abolish ELF_OBSOLETE_VOIDP_CAST; simply remove all uses.
No functional change. (Verified by diffing assembler output.)
This is part of the fix to a security issue, XSA-55.
Conflicts in the 4.1 backport:
* elf_load_image is not in 4.1.
* elf_note_numeric_array is not in 4.1.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
tools/libxc/xc_dom_elfloader.c | 8 +++---
tools/xcutils/readnotes.c | 2 +-
xen/common/libelf/libelf-dominfo.c | 6 ++--
xen/common/libelf/libelf-loader.c | 20 +++++++-------
xen/common/libelf/libelf-tools.c | 23 +++++++++--------
xen/include/xen/libelf.h | 48 +++++++++---------------------------
6 files changed, 42 insertions(+), 65 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index b7da0b5..8f2497d 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -116,9 +116,9 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
struct elf_binary *elf, bool load)
{
struct elf_binary syms;
- ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
+ ELF_HANDLE_DECL(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
xen_vaddr_t symtab, maxaddr;
- ELF_PTRVAL_CHAR hdr;
+ elf_ptrval hdr;
size_t size;
unsigned h, count, type, i, tables = 0;
unsigned long *strtab_referenced = NULL;
@@ -242,7 +242,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
for ( h = 0; h < count; h++ )
{
- shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h);
+ shdr = elf_shdr_by_index(&syms, h);
if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
/* input has an insane section header count field */
break;
@@ -278,7 +278,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
if ( load )
{
shdr2 = elf_shdr_by_index(elf, h);
- elf_memcpy_safe(elf, ELF_OBSOLETE_VOIDP_CAST elf_section_start(&syms, shdr),
+ elf_memcpy_safe(elf, elf_section_start(&syms, shdr),
elf_section_start(elf, shdr2),
size);
}
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index 543e0f5..b534001 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -38,7 +38,7 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note)
{
unsigned descsz = elf_uval(elf, note, descsz);
- ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
+ elf_ptrval desc = elf_note_desc(elf, note);
/* XXX should be able to cope with a list of values. */
switch ( descsz / 2 )
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 8f6bdec..2675777 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -210,8 +210,8 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
static unsigned elf_xen_parse_notes(struct elf_binary *elf,
struct elf_dom_parms *parms,
- ELF_PTRVAL_CONST_VOID start,
- ELF_PTRVAL_CONST_VOID end,
+ elf_ptrval start,
+ elf_ptrval end,
unsigned *total_note_count)
{
unsigned xen_elfnotes = 0;
@@ -248,7 +248,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf,
elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf,
struct elf_dom_parms *parms)
{
- ELF_PTRVAL_CONST_CHAR h;
+ elf_ptrval h;
unsigned char name[32], value[128];
unsigned len;
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 118d5aa..2698d44 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -157,12 +157,12 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
static void elf_load_bsdsyms(struct elf_binary *elf)
{
- ELF_HANDLE_DECL_NONCONST(elf_ehdr) sym_ehdr;
+ ELF_HANDLE_DECL(elf_ehdr) sym_ehdr;
unsigned long sz;
- ELF_PTRVAL_VOID maxva;
- ELF_PTRVAL_VOID symbase;
- ELF_PTRVAL_VOID symtab_addr;
- ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr;
+ elf_ptrval maxva;
+ elf_ptrval symbase;
+ elf_ptrval symtab_addr;
+ ELF_HANDLE_DECL(elf_shdr) shdr;
unsigned i, type;
if ( !elf->bsd_symtab_pstart )
@@ -196,7 +196,7 @@ do { \
elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr),
ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
sz);
- maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz);
+ maxva = elf_round_up(elf, (unsigned long)maxva + sz);
for ( i = 0; i < elf_shdr_count(elf); i++ )
{
@@ -212,7 +212,7 @@ do { \
elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz);
/* Mangled to be based on ELF header location. */
elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
- maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz);
+ maxva = elf_round_up(elf, (unsigned long)maxva + sz);
}
old_shdr_p = ELF_HANDLE_PTRVAL(shdr);
new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize);
@@ -267,7 +267,7 @@ void elf_load_binary(struct elf_binary *elf)
{
ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t i, count, paddr, offset, filesz, memsz;
- ELF_PTRVAL_VOID dest;
+ elf_ptrval dest;
/*
* Let bizarre ELFs write the output image up to twice; this
* calculation is just to ensure our copying loop is no worse than
@@ -304,7 +304,7 @@ void elf_load_binary(struct elf_binary *elf)
remain_allow_copy -= memsz;
elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n",
- __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz));
+ __func__, i, dest, (elf_ptrval)(dest + filesz));
elf_memcpy_safe(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz);
elf_memset_safe(elf, dest + filesz, 0, memsz - filesz);
}
@@ -312,7 +312,7 @@ void elf_load_binary(struct elf_binary *elf)
elf_load_bsdsyms(elf);
}
-ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr)
+elf_ptrval elf_get_ptr(struct elf_binary *elf, unsigned long addr)
{
return ELF_REALPTR2PTRVAL(elf->dest_base) + addr - elf->pstart;
}
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 238262b..c765fbc 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -171,7 +171,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n
ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index)
{
uint64_t count = elf_shdr_count(elf);
- ELF_PTRVAL_CONST_VOID ptr;
+ elf_ptrval ptr;
if ( index >= count )
return ELF_INVALID_HANDLE(elf_shdr);
@@ -185,7 +185,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned ind
ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index)
{
uint64_t count = elf_uval(elf, elf->ehdr, e_phnum);
- ELF_PTRVAL_CONST_VOID ptr;
+ elf_ptrval ptr;
if ( index >= count )
return ELF_INVALID_HANDLE(elf_phdr);
@@ -233,24 +233,24 @@ const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start)
return str;
}
-ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
+elf_ptrval elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
{
return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset);
}
-ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
+elf_ptrval elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr)
{
return ELF_IMAGE_BASE(elf)
+ elf_uval(elf, shdr, sh_offset) + elf_uval(elf, shdr, sh_size);
}
-ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
+elf_ptrval elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
return ELF_IMAGE_BASE(elf)
+ elf_uval(elf, phdr, p_offset);
}
-ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
+elf_ptrval elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
{
return ELF_IMAGE_BASE(elf)
+ elf_uval(elf, phdr, p_offset) + elf_uval(elf, phdr, p_filesz);
@@ -258,8 +258,8 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el
ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol)
{
- ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab);
- ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab);
+ elf_ptrval ptr = elf_section_start(elf, elf->sym_tab);
+ elf_ptrval end = elf_section_end(elf, elf->sym_tab);
ELF_HANDLE_DECL(elf_sym) sym;
uint64_t info, name;
const char *sym_name;
@@ -283,7 +283,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym
ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index)
{
- ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab);
+ elf_ptrval ptr = elf_section_start(elf, elf->sym_tab);
ELF_HANDLE_DECL(elf_sym) sym;
sym = ELF_MAKE_HANDLE(elf_sym, ptr + index * elf_size(elf, sym));
@@ -295,7 +295,7 @@ const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note
return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note));
}
-ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
+elf_ptrval elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
@@ -304,7 +304,7 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_
uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
- ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
+ elf_ptrval desc = elf_note_desc(elf, note);
unsigned descsz = elf_uval(elf, note, descsz);
switch (descsz)
@@ -318,6 +318,7 @@ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note
return 0;
}
}
+
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note)
{
unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 738bf0a..9fec7b4 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -61,13 +61,8 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
/*
* We abstract away the pointerness of these pointers, replacing
* various void*, char* and struct* with the following:
- * PTRVAL A pointer to a byte; one can do pointer arithmetic
+ * elf_ptrval A pointer to a byte; one can do pointer arithmetic
* on this.
- * This replaces variables which were char*,void*
- * and their const versions, so we provide four
- * different obsolete declaration macros:
- * ELF_PTRVAL_{,CONST}{VOID,CHAR}
- * New code can simply use the elf_ptrval typedef.
* HANDLE A pointer to a struct. There is one of these types
* for each pointer type - that is, for each "structname".
* In the arguments to the various HANDLE macros, structname
@@ -76,8 +71,6 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data,
* pointers. In the current code attempts to do so will
* compile, but in the next patch this will become a
* compile error.
- * We also provide a second declaration macro for
- * pointers which were to const; this is obsolete.
*/
#ifdef __XEN__
@@ -98,15 +91,9 @@ typedef elf_uintptr_t elf_ptrval;
#define ELF_REALPTR2PTRVAL(realpointer) ((elf_ptrval)(realpointer))
/* Converts an actual C pointer into a PTRVAL */
-#define ELF_HANDLE_DECL_NONCONST(structname) structname##_handle /*obsolete*/
#define ELF_HANDLE_DECL(structname) structname##_handle
/* Provides a type declaration for a HANDLE. */
-#define ELF_PTRVAL_VOID elf_ptrval /*obsolete*/
-#define ELF_PTRVAL_CHAR elf_ptrval /*obsolete*/
-#define ELF_PTRVAL_CONST_VOID elf_ptrval /*obsolete*/
-#define ELF_PTRVAL_CONST_CHAR elf_ptrval /*obsolete*/
-
#define ELF_DEFINE_HANDLE(structname) \
typedef union { \
elf_ptrval ptrval; \
@@ -126,17 +113,6 @@ typedef elf_uintptr_t elf_ptrval;
#define ELF_HANDLE_PTRVAL(handleval) ((handleval).ptrval)
/* Converts a HANDLE to a PTRVAL. */
-#define ELF_OBSOLETE_VOIDP_CAST /*empty*/
- /*
- * In some places the old code used to need to
- * - cast away const (the existing code uses const a fair
- * bit but actually sometimes wants to write to its input)
- * from a PTRVAL.
- * - convert an integer representing a pointer to a PTRVAL
- * Nowadays all of these re uintptr_ts so there is no const problem
- * and no need for any casting.
- */
-
#define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_uintptr_t)(ptrval))
/*
* Turns a PTRVAL into an actual C pointer. Before this is done
@@ -214,7 +190,7 @@ struct elf_binary {
char data;
ELF_HANDLE_DECL(elf_ehdr) ehdr;
- ELF_PTRVAL_CONST_CHAR sec_strtab;
+ elf_ptrval sec_strtab;
ELF_HANDLE_DECL(elf_shdr) sym_tab;
uint64_t sym_strtab;
@@ -292,7 +268,7 @@ struct elf_binary {
* str should be a HANDLE.
*/
-uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr,
+uint64_t elf_access_unsigned(struct elf_binary *elf, elf_ptrval ptr,
uint64_t offset, size_t size);
/* Reads a field at arbitrary offset and alignemnt */
@@ -344,17 +320,17 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned ind
ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index);
const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */
-ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
-ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
+elf_ptrval elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
+elf_ptrval elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr);
-ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
-ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
+elf_ptrval elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
+elf_ptrval elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol);
ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index);
const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */
-ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
+elf_ptrval elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
@@ -381,7 +357,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*,
void elf_parse_binary(struct elf_binary *elf);
void elf_load_binary(struct elf_binary *elf);
-ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr);
+elf_ptrval elf_get_ptr(struct elf_binary *elf, unsigned long addr);
uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol);
void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */
@@ -416,9 +392,9 @@ struct xen_elfnote {
struct elf_dom_parms {
/* raw */
- ELF_PTRVAL_CONST_CHAR guest_info;
- ELF_PTRVAL_CONST_VOID elf_note_start;
- ELF_PTRVAL_CONST_VOID elf_note_end;
+ elf_ptrval guest_info;
+ elf_ptrval elf_note_start;
+ elf_ptrval elf_note_end;
struct xen_elfnote elf_notes[XEN_ELFNOTE_MAX + 1];
/* parsed */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 17/21] libxc: Add range checking to xc_dom_binloader
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (15 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 16/21] libelf: abolish obsolete macros Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
` (3 subsequent siblings)
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
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 <ian.jackson@eu.citrix.com>
---
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 bde93f7..bb904b3 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;
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);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (16 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 17/21] libxc: Add range checking to xc_dom_binloader Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-13 15:54 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 19/21] DRAFT libxc: check return values from malloc Ian Jackson
` (2 subsequent siblings)
20 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
*** DRAFT ONLY ***
This is a direct backport of the 4.2 patch. I have not rerun the
search yet, and need to do so.
The return values from xc_dom_*_to_ptr and xc_map_foreign_range are
sometimes dereferenced, or subjected to pointer arithmetic, without
checking whether the relevant function failed and returned NULL.
Add an appropriate error check at every call site.
Changes in the 4.2 backport of this series:
* Fix tools/libxc/xc_dom_x86.c:setup_pgtables_x86_32.
* Fix tools/libxc/xc_dom_ia64.c:start_info_ia64.
* Fix tools/libxc/ia64/xc_ia64_dom_fwloader.c:xc_dom_load_fw_kernel.
Conflicts in the 4.1 backport of this series:
* xc_dom_load_elf_kernel has less error handling in 4.1.
* the VM generation ID code is not in 4.1.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/ia64/xc_ia64_dom_fwloader.c | 2 +
tools/libxc/xc_dom_binloader.c | 6 +++
tools/libxc/xc_dom_core.c | 6 +++
tools/libxc/xc_dom_elfloader.c | 12 +++++++
tools/libxc/xc_dom_ia64.c | 6 +++
tools/libxc/xc_dom_x86.c | 55 +++++++++++++++++++++++++++++++
tools/libxc/xc_domain_restore.c | 21 ++++++++++++
7 files changed, 108 insertions(+), 0 deletions(-)
diff --git a/tools/libxc/ia64/xc_ia64_dom_fwloader.c b/tools/libxc/ia64/xc_ia64_dom_fwloader.c
index cdf3333..dbd3349 100644
--- a/tools/libxc/ia64/xc_ia64_dom_fwloader.c
+++ b/tools/libxc/ia64/xc_ia64_dom_fwloader.c
@@ -60,6 +60,8 @@ static int xc_dom_load_fw_kernel(struct xc_dom_image *dom)
unsigned long i;
dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart);
+ if ( dest == NULL )
+ return -1;
memcpy(dest, dom->kernel_blob, FW_SIZE);
/* Synchronize cache. */
diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index bb904b3..e3ecdaf 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -277,6 +277,12 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom)
DOMPRINTF(" bss_size: 0x%" PRIx32 "", bss_size);
dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size);
+ if ( dest == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart)"
+ " => NULL", __FUNCTION__);
+ return -EINVAL;
+ }
if ( dest_size < text_size ||
dest_size - text_size < bss_size )
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 8913e41..a54ddae 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -868,6 +868,12 @@ int xc_dom_build_image(struct xc_dom_image *dom)
ramdisklen) != 0 )
goto err;
ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg);
+ if ( ramdiskmap == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg) => NULL",
+ __FUNCTION__);
+ goto err;
+ }
if ( unziplen )
{
if ( xc_dom_do_gunzip(dom->xch,
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 8f2497d..a0d1980 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -139,6 +139,12 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
return 0;
size = dom->kernel_seg.vend - dom->bsd_symtab_start;
hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
+ if ( hdr_ptr == NULL )
+ {
+ DOMPRINTF("%s/load: xc_dom_vaddr_to_ptr(dom,dom->bsd_symtab_start"
+ " => NULL", __FUNCTION__);
+ return -1;
+ }
elf->caller_xdest_base = hdr_ptr;
elf->caller_xdest_size = allow_size;
hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
@@ -375,6 +381,12 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom)
xen_pfn_t pages;
elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages);
+ if ( elf->dest_base == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom,dom->kernel_seg)"
+ " => NULL", __FUNCTION__);
+ return -1;
+ }
elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
elf_load_binary(elf);
if ( dom->parms.bsd_symtab )
diff --git a/tools/libxc/xc_dom_ia64.c b/tools/libxc/xc_dom_ia64.c
index dcd1523..7c0eff1 100644
--- a/tools/libxc/xc_dom_ia64.c
+++ b/tools/libxc/xc_dom_ia64.c
@@ -60,6 +60,12 @@ int start_info_ia64(struct xc_dom_image *dom)
DOMPRINTF_CALLED(dom->xch);
+ if ( start_info == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", __FUNCTION__);
+ return -1; /* our caller throws away our return value :-/ */
+ }
+
memset(start_info, 0, sizeof(*start_info));
sprintf(start_info->magic, dom->guest_type);
start_info->flags = dom->flags;
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0cf1687..75d6b83 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -144,6 +144,9 @@ static int setup_pgtables_x86_32(struct xc_dom_image *dom)
xen_vaddr_t addr;
xen_pfn_t pgpfn;
+ if ( l2tab == NULL )
+ goto pfn_error;
+
for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end;
addr += PAGE_SIZE_X86 )
{
@@ -151,6 +154,8 @@ static int setup_pgtables_x86_32(struct xc_dom_image *dom)
{
/* get L1 tab, make L2 entry */
l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
+ if ( l1tab == NULL )
+ goto pfn_error;
l2off = l2_table_offset_i386(addr);
l2tab[l2off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
@@ -169,6 +174,11 @@ static int setup_pgtables_x86_32(struct xc_dom_image *dom)
l1tab = NULL;
}
return 0;
+
+pfn_error:
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__);
+ return -EINVAL;
}
/*
@@ -219,6 +229,12 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom,
goto out;
l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
+ if ( l3tab == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_pfn_to_ptr(dom, l3pfn, 1) => NULL",
+ __FUNCTION__);
+ return l3mfn; /* our one call site will call xc_dom_panic and fail */
+ }
memset(l3tab, 0, XC_DOM_PAGE_SIZE(dom));
DOMPRINTF("%s: successfully relocated L3 below 4G. "
@@ -262,6 +278,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
}
l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
+ if ( l3tab == NULL )
+ goto pfn_error;
for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end;
addr += PAGE_SIZE_X86 )
@@ -270,6 +288,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
{
/* get L2 tab, make L3 entry */
l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
+ if ( l2tab == NULL )
+ goto pfn_error;
l3off = l3_table_offset_pae(addr);
l3tab[l3off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
@@ -280,6 +300,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
{
/* get L1 tab, make L2 entry */
l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
+ if ( l1tab == NULL )
+ goto pfn_error;
l2off = l2_table_offset_pae(addr);
l2tab[l2off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
@@ -306,6 +328,11 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom)
l3tab[3] = pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
}
return 0;
+
+pfn_error:
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__);
+ return -EINVAL;
}
#undef L1_PROT
@@ -344,6 +371,9 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
uint64_t addr;
xen_pfn_t pgpfn;
+ if ( l4tab == NULL )
+ goto pfn_error;
+
for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end;
addr += PAGE_SIZE_X86 )
{
@@ -351,6 +381,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
{
/* get L3 tab, make L4 entry */
l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
+ if ( l3tab == NULL )
+ goto pfn_error;
l4off = l4_table_offset_x86_64(addr);
l4tab[l4off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l3pfn)) | L4_PROT;
@@ -361,6 +393,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
{
/* get L2 tab, make L3 entry */
l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
+ if ( l2tab == NULL )
+ goto pfn_error;
l3off = l3_table_offset_x86_64(addr);
l3tab[l3off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
@@ -373,6 +407,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
{
/* get L1 tab, make L2 entry */
l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
+ if ( l1tab == NULL )
+ goto pfn_error;
l2off = l2_table_offset_x86_64(addr);
l2tab[l2off] =
pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
@@ -393,6 +429,11 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
l1tab = NULL;
}
return 0;
+
+pfn_error:
+ xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+ "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__);
+ return -EINVAL;
}
#undef L1_PROT
@@ -410,6 +451,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach", 0, p2m_size) )
return -1;
dom->p2m_guest = xc_dom_seg_to_ptr(dom, &dom->p2m_seg);
+ if ( dom->p2m_guest == NULL )
+ return -1;
/* allocate special pages */
dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
@@ -434,6 +477,12 @@ static int start_info_x86_32(struct xc_dom_image *dom)
DOMPRINTF_CALLED(dom->xch);
+ if ( start_info == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", __FUNCTION__);
+ return -1; /* our caller throws away our return value :-/ */
+ }
+
memset(start_info, 0, sizeof(*start_info));
strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic));
start_info->magic[sizeof(start_info->magic) - 1] = '\0';
@@ -474,6 +523,12 @@ static int start_info_x86_64(struct xc_dom_image *dom)
DOMPRINTF_CALLED(dom->xch);
+ if ( start_info == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", __FUNCTION__);
+ return -1; /* our caller throws away our return value :-/ */
+ }
+
memset(start_info, 0, sizeof(*start_info));
strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic));
start_info->magic[sizeof(start_info->magic) - 1] = '\0';
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 9564480..29af52b 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -1434,6 +1434,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
l3tab = (uint64_t *)
xc_map_foreign_range(xch, dom, PAGE_SIZE,
PROT_READ, ctx->p2m[i]);
+ if ( l3tab == NULL )
+ {
+ PERROR("xc_map_foreign_range failed (for l3tab)");
+ goto out;
+ }
for ( j = 0; j < 4; j++ )
l3ptes[j] = l3tab[j];
@@ -1460,6 +1465,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
l3tab = (uint64_t *)
xc_map_foreign_range(xch, dom, PAGE_SIZE,
PROT_READ | PROT_WRITE, ctx->p2m[i]);
+ if ( l3tab == NULL )
+ {
+ PERROR("xc_map_foreign_range failed (for l3tab, 2nd)");
+ goto out;
+ }
for ( j = 0; j < 4; j++ )
l3tab[j] = l3ptes[j];
@@ -1630,6 +1640,12 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
SET_FIELD(ctxt, user_regs.edx, mfn);
start_info = xc_map_foreign_range(
xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, mfn);
+ if ( start_info == NULL )
+ {
+ PERROR("xc_map_foreign_range failed (for start_info)");
+ goto out;
+ }
+
SET_FIELD(start_info, nr_pages, dinfo->p2m_size);
SET_FIELD(start_info, shared_info, shared_info_frame<<PAGE_SHIFT);
SET_FIELD(start_info, flags, 0);
@@ -1765,6 +1781,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
/* Restore contents of shared-info page. No checking needed. */
new_shared_info = xc_map_foreign_range(
xch, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame);
+ if ( new_shared_info == NULL )
+ {
+ PERROR("xc_map_foreign_range failed (for new_shared_info)");
+ goto out;
+ }
/* restore saved vcpu_info and arch specific info */
MEMCPY_FIELD(new_shared_info, old_shared_info, vcpu_info);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
2013-06-12 16:00 ` [PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
@ 2013-06-13 15:54 ` Ian Jackson
0 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-13 15:54 UTC (permalink / raw)
To: xen-devel, mattjd, security, andrew.cooper3
Ian Jackson writes ("[PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range"):
> *** DRAFT ONLY ***
> This is a direct backport of the 4.2 patch. I have not rerun the
> search yet, and need to do so.
I have rerun this search and found nothing further that needed to be
added in the 4.1 version.
However, the change to xc_map_foreign_page very likely needs to be
included here, so I will backport it to 4.2 and 4.1 and include it in
the new versions of those series.
Ian.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 19/21] DRAFT libxc: check return values from malloc
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (17 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 18/21] DRAFT libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-13 15:53 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 20/21] libxc: range checks in xc_dom_p2m_host and _guest Ian Jackson
2013-06-12 16:00 ` [PATCH 21/21] libxc: check blob size before proceeding in xc_dom_check_gzip Ian Jackson
20 siblings, 1 reply; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
*** DRAFT ONLY ***
This is a direct backport of the 4.2 patch. I have not rerun the
search yet, and need to do so.
A sufficiently malformed input to libxc (such as a malformed input ELF
or other guest-controlled data) might cause one of libxc's malloc() to
fail. In this case we need to make sure we don't dereference or do
pointer arithmetic on the result.
Search for all occurrences of \b(m|c|re)alloc in libxc, and all
functions which call them, and add appropriate error checking where
missing.
This includes the functions xc_dom_malloc*, which now print a message
when they fail so that callers don't have to do so.
The function xc_cpuid_to_str wasn't provided with a sane return value
and has a pretty strange API, which now becomes a little stranger.
There are no in-tree callers.
Changes in the Xen 4.2 version of this series:
* No need to fix code relating to ARM.
* No need to fix code relating to superpage support.
* Additionally fix `dom->p2m_host = xc_dom_malloc...' in xc_dom_ia64.c.
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_cpuid_x86.c | 20 ++++++++++++++++++--
tools/libxc/xc_dom_core.c | 13 +++++++++++++
tools/libxc/xc_dom_elfloader.c | 2 ++
tools/libxc/xc_dom_ia64.c | 6 ++++++
tools/libxc/xc_dom_x86.c | 3 +++
tools/libxc/xc_domain_restore.c | 5 +++++
tools/libxc/xc_linux_osdep.c | 4 ++++
tools/libxc/xc_offline_page.c | 5 +++++
tools/libxc/xc_private.c | 2 ++
tools/libxc/xenctrl.h | 2 +-
10 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index f61308a..5adf2d9 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -515,6 +515,8 @@ static int xc_cpuid_do_domctl(
static char *alloc_str(void)
{
char *s = malloc(33);
+ if ( s == NULL )
+ return s;
memset(s, 0, 33);
return s;
}
@@ -526,6 +528,8 @@ void xc_cpuid_to_str(const unsigned int *regs, char **strs)
for ( i = 0; i < 4; i++ )
{
strs[i] = alloc_str();
+ if ( strs[i] == NULL )
+ continue;
for ( j = 0; j < 32; j++ )
strs[i][j] = !!((regs[i] & (1U << (31 - j)))) ? '1' : '0';
}
@@ -599,7 +603,7 @@ int xc_cpuid_check(
const char **config,
char **config_transformed)
{
- int i, j;
+ int i, j, rc;
unsigned int regs[4];
memset(config_transformed, 0, 4 * sizeof(*config_transformed));
@@ -611,6 +615,11 @@ int xc_cpuid_check(
if ( config[i] == NULL )
continue;
config_transformed[i] = alloc_str();
+ if ( config_transformed[i] == NULL )
+ {
+ rc = -ENOMEM;
+ goto fail_rc;
+ }
for ( j = 0; j < 32; j++ )
{
unsigned char val = !!((regs[i] & (1U << (31 - j))));
@@ -627,12 +636,14 @@ int xc_cpuid_check(
return 0;
fail:
+ rc = -EPERM;
+ fail_rc:
for ( i = 0; i < 4; i++ )
{
free(config_transformed[i]);
config_transformed[i] = NULL;
}
- return -EPERM;
+ return rc;
}
/*
@@ -677,6 +688,11 @@ int xc_cpuid_set(
}
config_transformed[i] = alloc_str();
+ if ( config_transformed[i] == NULL )
+ {
+ rc = -ENOMEM;
+ goto fail;
+ }
for ( j = 0; j < 32; j++ )
{
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index a54ddae..3cbf9f7 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -120,9 +120,17 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size)
{
struct xc_dom_mem *block;
+ if ( size > SIZE_MAX - sizeof(*block) )
+ {
+ DOMPRINTF("%s: unreasonable allocation size", __FUNCTION__);
+ return NULL;
+ }
block = malloc(sizeof(*block) + size);
if ( block == NULL )
+ {
+ DOMPRINTF("%s: allocation failed", __FUNCTION__);
return NULL;
+ }
memset(block, 0, sizeof(*block) + size);
block->next = dom->memblocks;
dom->memblocks = block;
@@ -138,7 +146,10 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
block = malloc(sizeof(*block));
if ( block == NULL )
+ {
+ DOMPRINTF("%s: allocation failed", __FUNCTION__);
return NULL;
+ }
memset(block, 0, sizeof(*block));
block->mmap_len = size;
block->mmap_ptr = mmap(NULL, block->mmap_len,
@@ -146,6 +157,7 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size)
-1, 0);
if ( block->mmap_ptr == MAP_FAILED )
{
+ DOMPRINTF("%s: mmap failed", __FUNCTION__);
free(block);
return NULL;
}
@@ -202,6 +214,7 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
close(fd);
if ( block != NULL )
free(block);
+ DOMPRINTF("%s: failed (on file `%s')", __FUNCTION__, filename);
return NULL;
}
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index a0d1980..dcc4e8d 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -329,6 +329,8 @@ static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
return rc;
elf = xc_dom_malloc(dom, sizeof(*elf));
+ if ( elf == NULL )
+ return -1;
dom->private_loader = elf;
rc = elf_init(elf, dom->kernel_blob, dom->kernel_size);
xc_elf_set_logfile(dom->xch, elf, 1);
diff --git a/tools/libxc/xc_dom_ia64.c b/tools/libxc/xc_dom_ia64.c
index 7c0eff1..076821c 100644
--- a/tools/libxc/xc_dom_ia64.c
+++ b/tools/libxc/xc_dom_ia64.c
@@ -188,6 +188,12 @@ int arch_setup_meminit(struct xc_dom_image *dom)
/* setup initial p2m */
dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * nbr);
+ if ( dom->p2m_host == NULL )
+ {
+ DOMPRINTF("%s: xc_dom_malloc failed for p2m_host",
+ __FUNCTION__);
+ return -1;
+ }
for ( pfn = 0; pfn < nbr; pfn++ )
dom->p2m_host[pfn] = start + pfn;
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 75d6b83..448d9a1 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -780,6 +780,9 @@ int arch_setup_meminit(struct xc_dom_image *dom)
}
dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
+ if ( dom->p2m_host == NULL )
+ return -EINVAL;
+
if ( dom->superpages )
{
int count = dom->total_pages >> SUPERPAGE_PFN_SHIFT;
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 29af52b..0f1e2d1 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -967,6 +967,11 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
/* Map relevant mfns */
pfn_err = calloc(j, sizeof(*pfn_err));
+ if ( pfn_err == NULL )
+ {
+ PERROR("allocation for pfn_err failed");
+ return -1;
+ }
region_base = xc_map_foreign_bulk(
xch, dom, PROT_WRITE, region_mfn, pfn_err, j);
diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index 6477ad8..fa7bb7c 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -294,6 +294,8 @@ static void *linux_privcmd_map_foreign_range(xc_interface *xch, xc_osdep_handle
num = (size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT;
arr = calloc(num, sizeof(xen_pfn_t));
+ if ( arr == NULL )
+ return NULL;
for ( i = 0; i < num; i++ )
arr[i] = mfn + i;
@@ -318,6 +320,8 @@ static void *linux_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handle
num_per_entry = chunksize >> XC_PAGE_SHIFT;
num = num_per_entry * nentries;
arr = calloc(num, sizeof(xen_pfn_t));
+ if ( arr == NULL )
+ return NULL;
for ( i = 0; i < nentries; i++ )
for ( j = 0; j < num_per_entry; j++ )
diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
index ad8e24c..c0c75fd 100644
--- a/tools/libxc/xc_offline_page.c
+++ b/tools/libxc/xc_offline_page.c
@@ -715,6 +715,11 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
new_p = xc_map_foreign_range(xch, domid, PAGE_SIZE,
PROT_READ|PROT_WRITE, new_mfn);
+ if ( new_p == NULL )
+ {
+ ERROR("failed to map foreign range for new_p");
+ goto failed;
+ }
memcpy(new_p, backup, PAGE_SIZE);
munmap(new_p, PAGE_SIZE);
mops.arg1.mfn = new_mfn;
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 09c8f23..1bf25d2 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -742,6 +742,8 @@ const char *xc_strerror(xc_interface *xch, int errcode)
errbuf = pthread_getspecific(errbuf_pkey);
if (errbuf == NULL) {
errbuf = malloc(XS_BUFSIZE);
+ if ( errbuf == NULL )
+ return "(failed to allocate errbuf)";
pthread_setspecific(errbuf_pkey, errbuf);
}
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 570c6d4..9bc9172 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1608,7 +1608,7 @@ int xc_cpuid_set(xc_interface *xch,
int xc_cpuid_apply_policy(xc_interface *xch,
domid_t domid);
void xc_cpuid_to_str(const unsigned int *regs,
- char **strs);
+ char **strs); /* some strs[] may be NULL if ENOMEM */
int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
#endif
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 19/21] DRAFT libxc: check return values from malloc
2013-06-12 16:00 ` [PATCH 19/21] DRAFT libxc: check return values from malloc Ian Jackson
@ 2013-06-13 15:53 ` Ian Jackson
0 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-13 15:53 UTC (permalink / raw)
To: xen-devel, mattjd, security, andrew.cooper3
Ian Jackson writes ("[PATCH 19/21] DRAFT libxc: check return values from malloc"):
> *** DRAFT ONLY ***
> This is a direct backport of the 4.2 patch. I have not rerun the
> search yet, and need to do so.
I have finished this grep and found one that additional obvious change
was needed. I will include this in v8 of my 4.1 backports.
Ian.
commit 0ed45ae28bf70cee5400ce8251577d264880489d
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Thu Jun 13 16:52:48 2013 +0100
xc_flask_access
diff --git a/.topmsg b/.topmsg
index 988c545..6c3ca79 100644
--- a/.topmsg
+++ b/.topmsg
@@ -1,11 +1,5 @@
From: Ian Jackson <ian.jackson@eu.citrix.com>
-Subject: DRAFT libxc: check return values from malloc
-
-
-*** DRAFT ONLY ***
-This is a direct backport of the 4.2 patch. I have not rerun the
-search yet, and need to do so.
-
+Subject: libxc: check return values from malloc
A sufficiently malformed input to libxc (such as a malformed input ELF
or other guest-controlled data) might cause one of libxc's malloc() to
@@ -28,6 +22,9 @@ Changes in the Xen 4.2 version of this series:
* No need to fix code relating to superpage support.
* Additionally fix `dom->p2m_host = xc_dom_malloc...' in xc_dom_ia64.c.
+Changes in the Xen 4.1 version of this series:
+* An additional check is needed in xc_flask.c:xc_flask_access.
+
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/tools/libxc/xc_flask.c b/tools/libxc/xc_flask.c
index 27794a8..78c243c 100644
--- a/tools/libxc/xc_flask.c
+++ b/tools/libxc/xc_flask.c
@@ -284,6 +284,8 @@ int xc_flask_access(xc_interface *xc_handle, const char *scon, const char *tcon,
MAX_SHORT_DEC_LEN + 1 +
sizeof(req)*2 + 1;
buf = malloc(bufLen);
+ if ( buf == NULL )
+ return -ENOMEM;
snprintf(buf, bufLen, "%s %s %hu %x", scon, tcon, tclass, req);
op.cmd = FLASK_ACCESS;
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 20/21] libxc: range checks in xc_dom_p2m_host and _guest
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (18 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 19/21] DRAFT libxc: check return values from malloc Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
2013-06-12 16:00 ` [PATCH 21/21] libxc: check blob size before proceeding in xc_dom_check_gzip Ian Jackson
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, Ian Jackson, security
These functions take guest pfns and look them up in the p2m. They did
no range checking.
However, some callers, notably xc_dom_boot.c:setup_hypercall_page want
to pass untrusted guest-supplied value(s). It is most convenient to
detect this here and return INVALID_MFN.
This is part of the fix to a security issue, XSA-55.
Changes from Xen 4.2 version of this patch:
* 4.2 lacks dom->rambase_pfn, so don't add/subtract/check it.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxc/xc_dom.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index 9dffbe6..316e2ae 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -315,6 +315,8 @@ static inline xen_pfn_t xc_dom_p2m_host(struct xc_dom_image *dom, xen_pfn_t pfn)
{
if (dom->shadow_enabled)
return pfn;
+ if (pfn >= dom->total_pages)
+ return INVALID_MFN;
return dom->p2m_host[pfn];
}
@@ -323,6 +325,8 @@ static inline xen_pfn_t xc_dom_p2m_guest(struct xc_dom_image *dom,
{
if (xc_dom_feature_translated(dom))
return pfn;
+ if (pfn >= dom->total_pages)
+ return INVALID_MFN;
return dom->p2m_host[pfn];
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 21/21] libxc: check blob size before proceeding in xc_dom_check_gzip
2013-06-12 16:00 [PATCH RFC v7 00/22] XSA55 libelf fixes for Xen 4.1 Ian Jackson
` (19 preceding siblings ...)
2013-06-12 16:00 ` [PATCH 20/21] libxc: range checks in xc_dom_p2m_host and _guest Ian Jackson
@ 2013-06-12 16:00 ` Ian Jackson
20 siblings, 0 replies; 32+ messages in thread
From: Ian Jackson @ 2013-06-12 16:00 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, mattjd, security
From: Matthew Daley <mattjd@gmail.com>
This is part of the fix to a security issue, XSA-55.
Signed-off-by: Matthew Daley <mattjd@gmail.com>
---
tools/libxc/xc_dom_core.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 3cbf9f7..37bb566 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -284,6 +284,10 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen)
unsigned char *gzlen;
size_t unziplen;
+ if ( ziplen < 6 )
+ /* too small */
+ return 0;
+
if ( strncmp(blob, "\037\213", 2) )
/* not gzipped */
return 0;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 32+ messages in thread