This is version 4 of my (prematurely-released) series to try to fix
libelf. This version deals better with some possibly-out-of-control
loops, fixes the three so-far-known regressions, and should fix the
32-bit ARM build.
Here is a summary of the series:
a 01/16 libelf: abolish libelf-relocate.c
A 02/16 libxc: introduce xc_dom_seg_to_ptr_pages
A 03/16 libelf: add `struct elf_binary*'' parameter to elf_load_image
a 04/16 libelf: abolish elf_sval and elf_access_signed
A 05/16 libelf: move include of <asm/guest_access.h> to top of file
a 06/16 libelf/xc_dom_load_elf_symtab: Do not use "syms"
uninitialised
* 07/16 libxl: introduce macros for memory access and pointer handling
a 08/16 tools/xcutils/readnotes: adjust print_l1_mfn_valid_note
A 09/16 libelf: check nul-terminated strings properly
* 10/16 libxl: check all pointer accesses
A 11/16 libxl: Check pointer references in elf_is_elfbinary
* 12/16 libelf: Make all callers call elf_check_broken
+ 13/16 libelf: use C99 bool for booleans
/ 14/16 libelf: use only unsigned integers
* 15/16 libelf: check loops for running away
a 16/16 libelf: abolish obsolete macros
Key to symbols:
* Updated in this version of the series.
+ New patch in this version.
/ Updated but only to remove changes into a separate patch.
- Updated with style changes only.
a Acked/reviwed by one reviewer.
A Acked/reviwed by more than one reviewer.
Also in every patch:
Updated commit msgs to correct email address for me.
libelf, and some of its callers, did not do nearly enough checking on
their input. Invalid inputs could cause signed integer arithmetic
overflows and wild pointer dereferences.
In this series we try to systematically eliminate this problem in a
way which has a reasonable chance of (i) still accepting all
previously-accepted ELF images (ii) not having remaining security
bugs, in a form which can be reviewied to verify (i) and (ii).
The approach is:
(i) Remove all uses of signed integers (of any kind). That
elmininates all integer overflows as sources of undefined
behaviour. Of course it still means that we can get incorrect
values throughout the code.
(ii) Replace all uses of pointers, both pointers into the supplied
ELF image, and pointers into the output (where we are loading)
by uintptr_t. That eliminates all pointer arithmetic overflows as
sources of undefined behaviour. Of course it still means that we
can get incorrect and unreasonable "pointer" values.
(iii) But these pointer values will be in uintptr_t, which cannot be
simply dereferenced by mistake. We will replace all dereferences
by macros which do range checking; out of range reads will read 0
and out of range writes will be ignored. Happily most (but not
all) of the reads in the code already go through macros which
abstract endianness[1] and/or 32/64bitness.
[1] Although not all the accesses use endian-aware techniques so
in fact the code can''t cope with foreign-endian ELFs. This is a
problem for another day.
(iv) Look for all loops and check that they are guaranteed to
terminate.
To enable verification of correctness of these changes I provide them
as a series roughly as follows:
1-6:
Pre-patches which make a few semantically neutral or semantically
correct changes. For human review.
7: Introduces a set of macros to abstract away pointer arithmetic and
input and output image memory accesses in libelf. Use these macros
everwhere they are applicable. However, define the macros in a way
that corresponds to the existing code. That this patch has no
functional change can be verified by comparing the before-and-after
assembler output from the compiler.
9. Introduce some macros for dealing with nul-terminated strings,
defined so as not to have any functional change at this stage.
10. Change the macro definitions, and introduce the new pseudopointer
types, pointer range checking, etc. For close human review. Each
macro change is justified in the commit message. This patch
eliminates most of the potential wild pointer accesses.
8,11-12,14-15:
Smaller patches for human review, fixing some leftover bugs,
including ensuring that all loops terminate.
13: Eliminate signed integers. Replace every "int",
"int*_t",
"long" and most "char"s by corresponding unsigned types.
This
eliminates all integer arithmetic overflows.
After this patch, libelf should be safe against hostile input:
* All arithmetic operations on values from the input file use
unsigned arithmetic which is guaranteed to be defined (although
it may of course result in wrong answers);
* All pointer accesses based on pointers to locations which depend on
the input file go via our range-checking accessors; accesses which
are not to the input or output regions are ignored (reads returning
0).
* The loops have been checked to ensure that they terminate and are at
worst O(image_size).
* Whenever an array variable was declared, the code has been manually
reviewed looking for possible out-of-bounds accesses.
This is XSA-55.
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).
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 b200123..4a31282 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -52,7 +52,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 2aafc44..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-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
--
1.7.2.5
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.
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 | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index ac36600..6c31440 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -294,16 +294,24 @@ 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;
+ if (pages_out) *pages_out = pages;
return xc_dom_pfn_to_ptr(dom, seg->pfn, pages);
}
+static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
+ struct xc_dom_seg *seg)
+{
+ return xc_dom_seg_to_ptr_pages(dom, seg, NULL);
+}
+
static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom,
xen_vaddr_t vaddr)
{
--
1.7.2.5
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 03/16] libelf: add `struct elf_binary*'' parameter to elf_load_image
The meat of this function is going to need a copy of the elf pointer,
in forthcoming patches.
No functional change in this patch.
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>
---
xen/common/libelf/libelf-loader.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/xen/common/libelf/libelf-loader.c
b/xen/common/libelf/libelf-loader.c
index 3cf9c59..bce667f 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -108,7 +108,8 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback
*log_callback,
elf->verbose = verbose;
}
-static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
+static int elf_load_image(struct elf_binary *elf,
+ void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
{
memcpy(dst, src, filesz);
memset(dst + filesz, 0, memsz - filesz);
@@ -122,7 +123,8 @@ void elf_set_verbose(struct elf_binary *elf)
elf->verbose = 1;
}
-static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
+static int elf_load_image(struct elf_binary *elf,
+ void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
{
int rc;
if ( filesz > ULONG_MAX || memsz > ULONG_MAX )
@@ -279,7 +281,7 @@ int elf_load_binary(struct elf_binary *elf)
dest = elf_get_ptr(elf, paddr);
elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%p ->
0x%p\n",
__func__, i, dest, dest + filesz);
- if ( elf_load_image(dest, elf->image + offset, filesz, memsz) != 0 )
+ if ( elf_load_image(elf, dest, elf->image + offset, filesz, memsz)
!= 0 )
return -1;
}
--
1.7.2.5
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 04/16] libelf: abolish elf_sval and elf_access_signed
These are not used anywhere.
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 8312065..1f08407 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 218bb18..ae03982 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
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 05/16] libelf: move include of <asm/guest_access.h> to top of file
libelf-loader.c #includes <asm/guest_access.h>, when being compiled
for Xen. Currently it does this in the middle of the file.
Move this #include to the top of the file, before libelf-private.h.
This is necessary because in forthcoming patches we will introduce
private #defines of memcpy etc. which would interfere with definitions
in headers #included from guest_access.h.
No semantic or functional change in this patch.
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>
---
xen/common/libelf/libelf-loader.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/xen/common/libelf/libelf-loader.c
b/xen/common/libelf/libelf-loader.c
index bce667f..94257f6 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -16,6 +16,10 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA
*/
+#ifdef __XEN__
+#include <asm/guest_access.h>
+#endif
+
#include "libelf-private.h"
/* ------------------------------------------------------------------------ */
@@ -116,7 +120,6 @@ static int elf_load_image(struct elf_binary *elf,
return 0;
}
#else
-#include <asm/guest_access.h>
void elf_set_verbose(struct elf_binary *elf)
{
--
1.7.2.5
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 06/16] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised
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.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
v2: Split this change into its own patch for proper review.
---
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 6583859..d2fc977 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -141,7 +141,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
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 07/16] libelf: introduce macros for memory access and pointer handling
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.
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.
* 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.
-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__)\.(\d+)\:/) {
$sedderybody .= " s/\\b$1\\.$2\\b/__XSA55MANGLED__$1.$./g;\n";
}
}
$sedderybody .= "}\n1;\n";
eval $sedderybody or die $@;
foreach (@lines) {
seddery();
print or die $!;
}
-8<-
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>
v4: Fix elf_load_binary''s phdr message to be correct on 32-bit.
Fix ELF_OBSOLETE_VOIDP_CAST to work on 32-bit.
v3.1:
Change elf_store_field to verify correctly on 32-bit.
comparison-generate copes with Xen 4.1''s lack of ./configure.
v2: Use Xen style for multi-line comments.
Postpone changes to readnotes.c:print_l1_mfn_valid_note.
Much improved verification instructions with new script.
Fixed commit message subject.
---
tools/libxc/xc_dom_elfloader.c | 30 +++---
tools/libxc/xc_hvm_build_x86.c | 2 +-
tools/xcutils/readnotes.c | 26 +++---
xen/common/libelf/libelf-dominfo.c | 51 +++++-----
xen/common/libelf/libelf-loader.c | 84 +++++++++--------
xen/common/libelf/libelf-tools.c | 94 +++++++++---------
xen/include/xen/libelf.h | 188 +++++++++++++++++++++++++++++++-----
7 files changed, 312 insertions(+), 163 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index d2fc977..9714b1e 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -113,9 +113,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;
@@ -144,11 +144,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) )
{
@@ -186,7 +186,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 )
{
@@ -202,9 +202,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;
}
}
@@ -213,9 +213,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++;
@@ -227,7 +227,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);
}
@@ -235,9 +235,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
/* Name is NULL. */
if ( elf_64bit(&syms) )
- *(Elf64_Word*)(&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 )
@@ -272,7 +272,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/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index ab33a7f..39f93a3 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -143,7 +143,7 @@ static int loadelfimage(xc_interface *xch, struct elf_binary
*elf,
if ( elf->dest == NULL )
goto err;
- elf->dest += elf->pstart & (PAGE_SIZE - 1);
+ ELF_ADVANCE_DEST(elf, elf->pstart & (PAGE_SIZE - 1));
/* Load the initial elf image. */
rc = elf_load_binary(elf);
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index c926186..2af047d 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -61,13 +61,13 @@ struct setup_header {
} __attribute__((packed));
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);
@@ -98,12 +98,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;
@@ -170,7 +170,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;
struct setup_header *hdr;
@@ -257,7 +257,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;
@@ -269,8 +269,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 )
@@ -278,13 +278,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");
@@ -292,7 +292,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 3242f54..566f6f9 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -44,7 +44,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 )
@@ -96,7 +96,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 {
@@ -215,15 +215,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") )
@@ -241,45 +242,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);
@@ -328,7 +330,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) )
@@ -457,12 +460,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;
@@ -532,11 +535,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 94257f6..f7fe283 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -26,7 +26,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) )
@@ -35,7 +35,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;
@@ -65,7 +65,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. */
@@ -77,9 +77,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);
@@ -113,10 +113,11 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback
*log_callback,
}
static int elf_load_image(struct elf_binary *elf,
- void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
+ ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src,
+ uint64_t filesz, uint64_t memsz)
{
- memcpy(dst, src, filesz);
- memset(dst + filesz, 0, memsz - filesz);
+ elf_memcpy_safe(elf, dst, src, filesz);
+ elf_memset_safe(elf, dst + filesz, 0, memsz - filesz);
return 0;
}
#else
@@ -126,16 +127,17 @@ void elf_set_verbose(struct elf_binary *elf)
elf->verbose = 1;
}
-static int elf_load_image(struct elf_binary *elf,
- void *dst, const void *src, uint64_t filesz, uint64_t
memsz)
+static int elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst,
ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz)
{
int rc;
if ( filesz > ULONG_MAX || memsz > ULONG_MAX )
return -1;
- rc = raw_copy_to_guest(dst, src, filesz);
+ /* We trust the dom0 kernel image completely, so we don''t care
+ * about overruns etc. here. */
+ rc = raw_copy_to_guest(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src), filesz);
if ( rc != 0 )
return -1;
- rc = raw_clear_guest(dst + filesz, memsz - filesz);
+ rc = raw_clear_guest(ELF_UNSAFE_PTR(dst + filesz), memsz - filesz);
if ( rc != 0 )
return -1;
return 0;
@@ -146,10 +148,10 @@ static int elf_load_image(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);
@@ -166,7 +168,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));
}
@@ -177,10 +179,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 )
@@ -189,18 +193,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);
@@ -209,37 +213,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;
@@ -267,9 +273,9 @@ void elf_parse_binary(struct elf_binary *elf)
int 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++ )
@@ -282,9 +288,9 @@ int 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);
- if ( elf_load_image(elf, dest, elf->image + offset, filesz, memsz)
!= 0 )
+ elf_msg(elf, "%s: phdr %" PRIu64 " at
0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n",
+ __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz));
+ if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz,
memsz) != 0 )
return -1;
}
@@ -292,18 +298,18 @@ int elf_load_binary(struct elf_binary *elf)
return 0;
}
-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 1f08407..bf68bcd 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)
@@ -200,10 +204,10 @@ uint64_t elf_note_numeric(struct elf_binary *elf, const
elf_note * note)
}
}
-uint64_t elf_note_numeric_array(struct elf_binary *elf, const elf_note *note,
+uint64_t elf_note_numeric_array(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note,
unsigned int unitsz, unsigned int idx)
{
- 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);
if ( descsz % unitsz || idx >= descsz / unitsz )
@@ -220,12 +224,12 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf,
const elf_note *note,
}
}
-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);
}
/* ------------------------------------------------------------------------ */
@@ -234,10 +238,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 ae03982..dc9b5ae 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -48,6 +48,97 @@ 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.
+ */
+
+#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*)(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*)(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 +174,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 +187,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,45 +232,72 @@ 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);
-uint64_t elf_note_numeric_array(struct elf_binary *, const elf_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);
+uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),
unsigned int unitsz, unsigned int idx);
-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 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 */
@@ -189,7 +313,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*,
void elf_parse_binary(struct elf_binary *elf);
int 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 */
@@ -221,9 +345,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 */
@@ -262,10 +386,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
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 08/16] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note
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.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
v2: Split out into its own patch.
---
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 2af047d..7ff2530 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -77,22 +77,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
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 09/16] libelf: check nul-terminated strings properly
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.
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>
v2: Fix coding style, in one "if" statement.
---
tools/xcutils/readnotes.c | 10 +++++++---
xen/common/libelf/libelf-dominfo.c | 13 ++++++++++---
xen/common/libelf/libelf-tools.c | 10 +++++++---
xen/include/xen/libelf.h | 7 +++++--
4 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index 7ff2530..ca86ba5 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -63,7 +63,7 @@ struct setup_header {
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,
@@ -103,10 +103,13 @@ 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 ||
+ 0 != strcmp(this_note_name, "Xen"))
continue;
notes_found++;
@@ -294,7 +297,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 566f6f9..ba0dc83 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -137,7 +137,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;
@@ -220,6 +223,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;
@@ -227,7 +231,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;
@@ -541,7 +548,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 bf68bcd..fa7dedd 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 dc9b5ae..0c2c11e 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -252,6 +252,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))
/*
@@ -279,7 +282,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);
@@ -289,7 +292,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);
uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),
--
1.7.2.5
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.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
v4: Fix ELF_UNSAFE_PTR to work on 32-bit even when provided 64-bit
values.
Fix xc_dom_load_elf_symtab not to call XC_DOM_PAGE_SIZE
unnecessarily if load is false. This was a regression.
v3.1:
Introduce a change to elf_store_field to undo the effects of
the v3.1 change to the previous patch (the definition there
is not compatible with the new types).
v3: Fix a whitespace error.
v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com>
v2: BUGFIX: elf_strval: Fix loop termination condition to actually work.
BUGFIX: elf_strval: Fix return value to not always be totally wild.
BUGFIX: xc_dom_load_elf_symtab: do proper check for small header size.
xc_dom_load_elf_symtab: narrow scope of `hdr_ptr''.
xc_dom_load_elf_symtab: split out uninit''d symtab.class ref fix.
More comments on the lifetime/validity of elf-> dest ptrs etc.
libelf.h: write "obsolete" out in full
libelf.h: rename "dontuse" to "typeonly" and add doc
comment
elf_ptrval_in_range: Document trustedness of arguments.
Style and commit message fixes.
---
tools/libxc/xc_dom_elfloader.c | 49 ++++++++-
tools/libxc/xc_hvm_build_x86.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 | 103 ++++++++++++++++++-
xen/include/xen/libelf.h | 194 +++++++++++++++++++++++++-----------
8 files changed, 305 insertions(+), 85 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 9714b1e..a28cfa3 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -128,19 +128,31 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image
*dom,
if ( load )
{
+ char *hdr_ptr;
+ unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
+
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);
- *(int *)hdr = size - sizeof(int);
+ hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start);
+ elf->caller_xdest_base = hdr_ptr;
+ elf->caller_xdest_size = page_size -
+ (dom->bsd_symtab_start & (page_size-1));
+ 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);
}
@@ -168,9 +180,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);
@@ -309,8 +344,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image *dom)
{
struct elf_binary *elf = dom->private_loader;
int rc;
+ 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);
rc = elf_load_binary(elf);
if ( rc < 0 )
{
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index 39f93a3..eff55a4 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -137,11 +137,12 @@ static int loadelfimage(xc_interface *xch, struct
elf_binary *elf,
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;
ELF_ADVANCE_DEST(elf, elf->pstart & (PAGE_SIZE - 1));
@@ -150,8 +151,9 @@ static int loadelfimage(xc_interface *xch, struct elf_binary
*elf,
if ( rc < 0 )
PERROR("Failed to load elf binary\n");
- 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 9980ea2..db31a91 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -765,7 +765,8 @@ int __init construct_dom0(
mapcache_override_current(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;
rc = elf_load_binary(&elf);
if ( rc < 0 )
{
diff --git a/xen/common/libelf/libelf-dominfo.c
b/xen/common/libelf/libelf-dominfo.c
index ba0dc83..b9a4e25 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -254,7 +254,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 f7fe283..878552e 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -24,23 +24,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) +
@@ -300,7 +302,7 @@ int 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 0d4dcf6..0bd9e66 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 fa7dedd..4e9ac21 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -20,28 +20,97 @@
/* ------------------------------------------------------------------------ */
-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.
+ */
+{
+ elf_ptrval regionp = (elf_ptrval)region;
+
+ if ( (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 +191,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 0c2c11e..22aa4fc 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,54 +68,66 @@ 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.
*/
-#define ELF_REALPTR2PTRVAL(realpointer) (realpointer)
+typedef 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*/
+
+#ifdef __XEN__
+# define ELF_PRPTRVAL "lu"
+ /*
+ * PRIuPTR is misdefined in xen/include/xen/inttypes.h, on 32-bit,
+ * to "u", when in fact uintptr_t is an unsigned long.
+ */
+#else
+# define ELF_PRPTRVAL PRIuPTR
+#endif
+ /* printf format a la PRId... for a PTRVAL */
-#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*)(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*)(ptrval))
+#define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_ptrval)(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
@@ -122,23 +135,25 @@ typedef void elf_log_callback(struct elf_binary*, void
*caller_data,
*/
/* 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)
/* ------------------------------------------------------------------------ */
-
typedef union {
Elf32_Ehdr e32;
Elf64_Ehdr e64;
@@ -182,7 +197,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;
@@ -190,10 +205,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;
@@ -201,12 +222,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;
};
/* ------------------------------------------------------------------------ */
@@ -224,22 +255,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.
@@ -251,23 +287,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. */
@@ -306,6 +356,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
@@ -321,6 +375,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 */
@@ -395,16 +452,33 @@ 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_size >= amount )
+ {
+ elf->dest_base += amount;
+ elf->dest_size -= amount;
+ }
+ else
+ {
+ elf->dest_size = 0;
+ }
+}
#endif /* __XEN_LIBELF_H__ */
--
1.7.2.5
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 11/16] libelf: Check pointer references in elf_is_elfbinary
elf_is_elfbinary didn''t take a length parameter and could potentially
access out of range when provided with a very short image.
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>
v2: Style fix.
Fix commit message subject.
---
tools/libxc/xc_dom_elfloader.c | 2 +-
xen/arch/x86/bzimage.c | 4 ++--
xen/common/libelf/libelf-loader.c | 2 +-
xen/common/libelf/libelf-tools.c | 9 ++++++---
xen/include/xen/libelf.h | 2 +-
5 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index a28cfa3..177219f 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -93,7 +93,7 @@ static int check_elf_kernel(struct xc_dom_image *dom, int
verbose)
return -EINVAL;
}
- if ( !elf_is_elfbinary(dom->kernel_blob) )
+ if ( !elf_is_elfbinary(dom->kernel_blob, dom->kernel_size) )
{
if ( verbose )
xc_dom_panic(dom->xch,
diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index c5519d8..58fda16 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -220,7 +220,7 @@ unsigned long __init bzimage_headroom(char *image_start,
image_length = hdr->payload_length;
}
- if ( elf_is_elfbinary(image_start) )
+ if ( elf_is_elfbinary(image_start, image_length) )
return 0;
orig_image_len = image_length;
@@ -251,7 +251,7 @@ int __init bzimage_parse(char *image_base, char
**image_start, unsigned long *im
*image_len = hdr->payload_length;
}
- if ( elf_is_elfbinary(*image_start) )
+ if ( elf_is_elfbinary(*image_start, *image_len) )
return 0;
BUG_ON(!(image_base < *image_start));
diff --git a/xen/common/libelf/libelf-loader.c
b/xen/common/libelf/libelf-loader.c
index 878552e..6c43c34 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -29,7 +29,7 @@ int elf_init(struct elf_binary *elf, const char *image_input,
size_t size)
ELF_HANDLE_DECL(elf_shdr) shdr;
uint64_t i, count, section, offset;
- if ( !elf_is_elfbinary(image_input) )
+ if ( !elf_is_elfbinary(image_input, size) )
{
elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__);
return -1;
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index 4e9ac21..1d5963a 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -329,11 +329,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 22aa4fc..5b71c7f 100644
--- a/xen/include/xen/libelf.h
+++ b/xen/include/xen/libelf.h
@@ -349,7 +349,7 @@ uint64_t elf_note_numeric_array(struct elf_binary *,
ELF_HANDLE_DECL(elf_note),
unsigned int unitsz, unsigned int idx);
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note);
-int elf_is_elfbinary(const void *image);
+int elf_is_elfbinary(const void *image_start, size_t image_size);
int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr)
phdr);
/* ------------------------------------------------------------------------ */
--
1.7.2.5
Ian Jackson
2013-Jun-04 17:59 UTC
[PATCH 12/16] libelf: Make all callers call elf_check_broken
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".
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
v3.1:
Add error check to xc_dom_parse_elf_kernel.
Move check in xc_hvm_build_x86.c:setup_guest to right place.
v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com>
v2 was Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
v2: Style fixes.
---
tools/libxc/xc_dom_elfloader.c | 25 +++++++++++++++++++++----
tools/libxc/xc_hvm_build_x86.c | 3 +++
tools/xcutils/readnotes.c | 3 +++
xen/arch/arm/kernel.c | 10 ++++++++++
xen/arch/x86/domain_build.c | 28 +++++++++++++++++++++-------
5 files changed, 58 insertions(+), 11 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 177219f..f1abe15 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -275,6 +275,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__);
@@ -311,19 +318,23 @@ 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;
+ }
if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) )
{
xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does
not"
" support unprivileged (DomU) operation",
__FUNCTION__);
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
/* find kernel segment */
@@ -337,7 +348,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_x86.c b/tools/libxc/xc_hvm_build_x86.c
index eff55a4..8bb0178 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -524,6 +524,9 @@ static int setup_guest(xc_interface *xch,
error_out:
rc = -1;
out:
+ if ( elf_check_broken(&elf) )
+ ERROR("HVM ELF broken: %s", elf_check_broken(&elf));
+
/* ensure no unclaimed pages are left unused */
xc_domain_claim_pages(xch, dom, 0 /* cancels the claim */);
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index ca86ba5..b868fba 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -300,6 +300,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/arm/kernel.c b/xen/arch/arm/kernel.c
index 8f4a60d..43cf2ab 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -171,6 +171,8 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
{
int rc;
+ memset(&info->elf.elf, 0, sizeof(info->elf.elf));
+
info->kernel_order = get_order_from_bytes(size);
info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0);
if ( info->kernel_img == NULL )
@@ -194,8 +196,16 @@ static int kernel_try_elf_prepare(struct kernel_info *info,
info->entry = info->elf.parms.virt_entry;
info->load = kernel_elf_load;
+ if ( elf_check_broken(&info->elf.elf) )
+ printk("Xen: warning: ELF kernel broken: %s\n",
+ elf_check_broken(&info->elf.elf));
+
return 0;
err:
+ if ( elf_check_broken(&info->elf.elf) )
+ printk("Xen: ELF kernel broken: %s\n",
+ elf_check_broken(&info->elf.elf));
+
free_xenheap_pages(info->kernel_img, info->kernel_order);
return rc;
}
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index db31a91..03fe845 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -380,7 +380,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;
@@ -408,14 +408,16 @@ int __init construct_dom0(
if ( !compatible )
{
printk("Mismatch between Xen and DOM0 kernel\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE
&&
!test_bit(XENFEAT_dom0, parms.f_supported) )
{
printk("Kernel does not support Dom0 operation\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
if ( compat32 )
@@ -596,7 +598,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) )
@@ -771,7 +774,7 @@ int __init construct_dom0(
if ( rc < 0 )
{
printk("Failed to load the kernel binary\n");
- return rc;
+ goto out;
}
bootstrap_map(NULL);
@@ -783,7 +786,8 @@ int __init construct_dom0(
mapcache_override_current(NULL);
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);
@@ -1133,9 +1137,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
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.)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
v3: Use <stdbool.h>''s bool (or _Bool) instead of defining
elf_bool.
Split this into a separate patch.
---
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 f1abe15..605b1e0 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 */);
}
/* ------------------------------------------------------------------------ */
@@ -82,7 +82,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 )
{
@@ -110,7 +110,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 b9a4e25..c4ced67 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -101,7 +101,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 6c43c34..798f88b 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -92,7 +92,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;
@@ -107,7 +107,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 0bd9e66..ea7e197 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 1d5963a..f96ac7a 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>
@@ -50,7 +50,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) )
@@ -89,7 +89,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;
@@ -329,7 +329,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;
@@ -339,7 +339,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 5b71c7f..2a88ea9 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
@@ -236,7 +238,7 @@ struct elf_binary {
elf_log_callback *log_callback;
void *log_caller_data;
#endif
- int verbose;
+ bool verbose;
const char *broken;
};
@@ -300,8 +302,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) \
({ \
@@ -349,8 +351,8 @@ uint64_t elf_note_numeric_array(struct elf_binary *,
ELF_HANDLE_DECL(elf_note),
unsigned int unitsz, unsigned int idx);
ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note);
-int elf_is_elfbinary(const void *image_start, size_t image_size);
-int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr)
phdr);
+bool elf_is_elfbinary(const void *image_start, size_t image_size);
+bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr)
phdr);
/* ------------------------------------------------------------------------ */
/* xc_libelf_loader.c */
@@ -364,7 +366,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);
@@ -416,7 +418,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
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").
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>
v4: Fix regression in elf_round_up; use uint64_t here.
v3: Changes to booleans split off into separate patch.
v2: BUGFIX: Eliminate conversion to int of return from elf_xen_parse_notes.
BUGFIX: Fix the one printf format thing which needs changing.
Remove irrelevant change to constify note_desc.name in libelf-dominfo.c.
In xc_dom_load_elf_symtab change one sizeof(int) to sizeof(unsigned).
Do not change type of 2nd argument to memset.
Provide seddery for easier review.
Style fix.
---
tools/libxc/Makefile | 9 +++++-
tools/libxc/xc_dom.h | 7 +++--
tools/libxc/xc_dom_elfloader.c | 42 ++++++++++++++++-------------
tools/xcutils/readnotes.c | 15 ++++++-----
xen/common/libelf/Makefile | 2 +
xen/common/libelf/libelf-dominfo.c | 50 ++++++++++++++++++------------------
xen/common/libelf/libelf-loader.c | 20 +++++++-------
xen/common/libelf/libelf-tools.c | 24 ++++++++--------
xen/include/xen/libelf.h | 21 ++++++++------
9 files changed, 103 insertions(+), 87 deletions(-)
diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 4a31282..512a994 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -51,8 +51,13 @@ endif
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 6c31440..d4b4f0d 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -155,9 +155,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 605b1e0..60000da 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -82,7 +82,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 )
{
@@ -104,12 +104,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;
@@ -117,7 +117,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) )
{
@@ -139,13 +139,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image
*dom,
elf->caller_xdest_size = page_size -
(dom->bsd_symtab_start & (page_size-1));
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 )
@@ -156,15 +156,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;
@@ -173,22 +173,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;
/*
@@ -208,7 +208,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));
@@ -254,7 +254,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);
@@ -293,10 +293,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 )
@@ -357,10 +361,10 @@ 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;
- int rc;
+ elf_errorstatus rc;
xen_pfn_t pages;
elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg,
&pages);
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index b868fba..9be1594 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -70,7 +70,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);
@@ -79,7 +79,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. */
@@ -99,10 +99,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) )
@@ -160,7 +160,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;
}
}
@@ -170,12 +170,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;
struct setup_header *hdr;
uint64_t payload_offset, payload_length;
diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile
index 18dc8e2..5bf8f76 100644
--- a/xen/common/libelf/Makefile
+++ b/xen/common/libelf/Makefile
@@ -2,6 +2,8 @@ obj-bin-y := libelf.o
SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
+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 c4ced67..a9a5f41 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -29,15 +29,15 @@ static const char *const elf_xen_feature_names[] = {
[XENFEAT_pae_pgdir_above_4gb] = "pae_pgdir_above_4gb",
[XENFEAT_dom0] = "dom0"
};
-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;
@@ -94,7 +94,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)
{
@@ -125,7 +125,7 @@ int elf_xen_parse_note(struct elf_binary *elf,
const char *str = NULL;
uint64_t val = 0;
unsigned int i;
- 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) )
@@ -216,12 +216,12 @@ int elf_xen_parse_note(struct elf_binary *elf,
return 0;
}
-static int elf_xen_parse_notes(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)
{
- int xen_elfnotes = 0;
+ unsigned xen_elfnotes = 0;
ELF_HANDLE_DECL(elf_note) note;
const char *note_name;
@@ -237,7 +237,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 ~0U;
xen_elfnotes++;
}
return xen_elfnotes;
@@ -246,12 +246,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))
@@ -334,13 +334,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: "
@@ -378,7 +378,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) &&
@@ -464,13 +464,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;
@@ -495,13 +495,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 == ~0U )
return -1;
- xen_elfnotes += rc;
+ xen_elfnotes += more_notes;
}
/*
@@ -518,17 +518,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 == ~0U )
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 798f88b..937c99b 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -24,7 +24,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;
@@ -114,7 +114,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback
*log_callback,
elf->verbose = verbose;
}
-static int elf_load_image(struct elf_binary *elf,
+static elf_errorstatus elf_load_image(struct elf_binary *elf,
ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src,
uint64_t filesz, uint64_t memsz)
{
@@ -129,9 +129,9 @@ void elf_set_verbose(struct elf_binary *elf)
elf->verbose = 1;
}
-static int elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst,
ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz)
+static elf_errorstatus elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID
dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz)
{
- int rc;
+ elf_errorstatus rc;
if ( filesz > ULONG_MAX || memsz > ULONG_MAX )
return -1;
/* We trust the dom0 kernel image completely, so we don''t care
@@ -151,7 +151,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;
@@ -187,7 +187,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;
@@ -220,7 +220,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++ )
{
@@ -233,10 +233,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. */
@@ -273,7 +273,7 @@ void elf_parse_binary(struct elf_binary *elf)
__FUNCTION__, elf->pstart, elf->pend);
}
-int elf_load_binary(struct elf_binary *elf)
+elf_errorstatus elf_load_binary(struct elf_binary *elf)
{
ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t i, count, paddr, offset, filesz, memsz;
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index f96ac7a..d7d5d00 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -119,19 +119,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);
}
@@ -141,7 +141,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++ )
{
@@ -153,7 +153,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;
@@ -167,7 +167,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;
@@ -261,7 +261,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;
@@ -277,7 +277,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;
}
@@ -285,7 +285,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,7 +303,7 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note
unsigned int unitsz, unsigned int idx)
{
ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
- int descsz = elf_uval(elf, note, descsz);
+ unsigned descsz = elf_uval(elf, note, descsz);
if ( descsz % unitsz || idx >= descsz / unitsz )
return 0;
@@ -321,8 +321,8 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_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 2a88ea9..40a01b4 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__
@@ -327,12 +330,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);
@@ -342,7 +345,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);
@@ -357,7 +360,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.
@@ -370,7 +373,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*,
#endif
void elf_parse_binary(struct elf_binary *elf);
-int elf_load_binary(struct elf_binary *elf);
+elf_errorstatus elf_load_binary(struct elf_binary *elf);
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);
@@ -383,7 +386,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 */
@@ -417,7 +420,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
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. Also check that the returned psuedopointer is sane.
* 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.
* Some fixed limits to avoid potentially O(image_size^2) loops:
- maximum length of strings: 4K (longer ones ignored totally)
- maximum total number of ELF notes: 65536 (any more are ignored)
* Check that the total program header contents we copy doesn''t
exceed the image size.
* Remove an entirely useless loop from elf_xen_parse (!)
* Replace a nested search loop in in xc_dom_load_elf_symtab in
xc_dom_elfloader.c by a precomputation of a bitmap of referenced
symtabs.
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.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
v4: Fix regression due to misplacement of test in elf_shdr_by_name
(uninitialised variable).
Introduce fixed limits.
Avoid O(size^2) loops.
Check returned psuedopointer from elf_note_next is correct.
A few style fixes.
v3: Fix a whitespace error.
v2: BUGFIX: elf_shdr_by_name, elf_note_next: Reject new <= old, not just
<.
elf_shdr_by_name: Change order of checks to be a bit clearer.
elf_load_bsdsyms: shdr loop check, improve chance of brokenness detection.
Style fixes.
---
tools/libxc/xc_dom_elfloader.c | 31 ++++++++++++++++------
xen/common/libelf/libelf-dominfo.c | 48 ++++++++++++++++++++++++------------
xen/common/libelf/libelf-loader.c | 42 ++++++++++++++++++++++++++++++-
xen/common/libelf/libelf-tools.c | 28 +++++++++++++++++++-
xen/include/xen/libelf.h | 3 ++
5 files changed, 123 insertions(+), 29 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 60000da..2e93ec7 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"
@@ -118,6 +119,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) )
{
@@ -219,22 +221,33 @@ 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));
+ 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 a9a5f41..83708f4 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -219,7 +219,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;
@@ -231,6 +232,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;
@@ -471,6 +478,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;
@@ -485,6 +493,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;
@@ -497,7 +512,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 == ~0U )
return -1;
@@ -515,12 +531,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 == ~0U )
return -1;
@@ -538,20 +559,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 937c99b..8d05dc6 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -75,6 +75,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;
@@ -170,6 +173,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));
@@ -224,6 +230,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) )
{
@@ -235,8 +244,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. */
@@ -256,6 +273,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);
@@ -278,11 +298,15 @@ elf_errorstatus 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 remain_allow_copy = elf->size;
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);
@@ -290,6 +314,20 @@ elf_errorstatus 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));
if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz,
memsz) != 0 )
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index d7d5d00..19a82f1 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -128,7 +128,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)
@@ -146,6 +155,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;
@@ -201,6 +213,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;
+ }
}
}
@@ -324,7 +341,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 40a01b4..755ddae 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
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.)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
v2: New patch.
---
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 | 24 +++++++++---------
xen/common/libelf/libelf-tools.c | 24 +++++++++---------
xen/include/xen/libelf.h | 48 +++++++++---------------------------
6 files changed, 44 insertions(+), 68 deletions(-)
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 2e93ec7..01eb4de 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -114,9 +114,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;
@@ -239,7 +239,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;
@@ -275,7 +275,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 9be1594..4ff7085 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -80,7 +80,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 83708f4..1e4db6e 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -218,8 +218,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;
@@ -256,7 +256,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 8d05dc6..fbc8de7 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -118,7 +118,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback
*log_callback,
}
static elf_errorstatus elf_load_image(struct elf_binary *elf,
- ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src,
+ elf_ptrval dst, elf_ptrval src,
uint64_t filesz, uint64_t memsz)
{
elf_memcpy_safe(elf, dst, src, filesz);
@@ -132,7 +132,7 @@ void elf_set_verbose(struct elf_binary *elf)
elf->verbose = 1;
}
-static elf_errorstatus elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID
dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz)
+static elf_errorstatus elf_load_image(struct elf_binary *elf, elf_ptrval dst,
elf_ptrval src, uint64_t filesz, uint64_t memsz)
{
elf_errorstatus rc;
if ( filesz > ULONG_MAX || memsz > ULONG_MAX )
@@ -187,12 +187,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 )
@@ -226,7 +226,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++ )
{
@@ -242,7 +242,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);
@@ -297,7 +297,7 @@ elf_errorstatus 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;
elf_ptrval remain_allow_copy = elf->size;
count = elf_uval(elf, elf->ehdr, e_phnum);
@@ -329,7 +329,7 @@ elf_errorstatus 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));
if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz,
memsz) != 0 )
return -1;
}
@@ -338,7 +338,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
return 0;
}
-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 19a82f1..b91701b 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -168,7 +168,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);
@@ -182,7 +182,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);
@@ -230,24 +230,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);
@@ -255,8 +255,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;
@@ -280,7 +280,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));
@@ -292,7 +292,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;
@@ -301,7 +301,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)
@@ -319,7 +319,7 @@ uint64_t elf_note_numeric(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note
uint64_t elf_note_numeric_array(struct elf_binary *elf,
ELF_HANDLE_DECL(elf_note) note,
unsigned int unitsz, unsigned int idx)
{
- 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);
if ( descsz % unitsz || idx >= descsz / unitsz )
diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
index 755ddae..94ff182 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.
*/
typedef uintptr_t elf_ptrval;
@@ -85,15 +78,9 @@ typedef 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*/
-
#ifdef __XEN__
# define ELF_PRPTRVAL "lu"
/*
@@ -124,17 +111,6 @@ typedef 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_ptrval)(ptrval))
/*
* Turns a PTRVAL into an actual C pointer. Before this is done
@@ -211,7 +187,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;
@@ -289,7 +265,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 */
@@ -341,17 +317,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);
uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),
unsigned int unitsz, unsigned int idx);
@@ -378,7 +354,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*,
void elf_parse_binary(struct elf_binary *elf);
elf_errorstatus 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 */
@@ -413,9 +389,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
Ian Jackson writes ("[PATCH 4 00/16] XSA55 libelf fixes for
unstable"):> This is version 4 of my (prematurely-released) series to try to fix
> libelf. This version deals better with some possibly-out-of-control
> loops, fixes the three so-far-known regressions, and should fix the
> 32-bit ARM build.
This is available at
http://xenbits.xen.org/gitweb/?p=people/iwj/xen-unstable.git;a=summary
git://xenbits.xen.org/people/iwj/xen-unstable.git
in the commits
xsa55-unstable-base-rebasing..xsa55-unstable-rebasing
Ian.
On 04/06/13 19:08, Ian Jackson wrote:> Ian Jackson writes ("[PATCH 4 00/16] XSA55 libelf fixes for unstable"): >> This is version 4 of my (prematurely-released) series to try to fix >> libelf. This version deals better with some possibly-out-of-control >> loops, fixes the three so-far-known regressions, and should fix the >> 32-bit ARM build. > This is available at > http://xenbits.xen.org/gitweb/?p=people/iwj/xen-unstable.git;a=summary > git://xenbits.xen.org/people/iwj/xen-unstable.git > in the commits > xsa55-unstable-base-rebasing..xsa55-unstable-rebasing > > Ian.Applying the diff between v3 and v4 to Xen-4.2 and 4.1 caused dom0 to GPF on boot. It turns out that an error is shown: Xen warning: dom0 kernel broken ELF: program segments total to more than the input image size Which might suggest which change in the set is broken. However, there is a bug in Xen in so far as it should not try to start executing dom0 if it failed to load. ~Andrew
George Dunlap
2013-Jun-05 10:32 UTC
Re: [PATCH 03/16] libelf: add `struct elf_binary*'' parameter to elf_load_image
On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> The meat of this function is going to need a copy of the elf pointer, > in forthcoming patches. > > No functional change in this patch. > > 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>Is there any reason not to commit the patches that have been acked and reviewed, so we can start getting stuff tested sooner (and reduce the cognitive load on you of having to deal with 16 patches)? -George
Andrew Cooper
2013-Jun-05 11:01 UTC
Re: [PATCH 03/16] libelf: add `struct elf_binary*'' parameter to elf_load_image
On 05/06/13 11:32, George Dunlap wrote:> On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >> The meat of this function is going to need a copy of the elf pointer, >> in forthcoming patches. >> >> No functional change in this patch. >> >> 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> > Is there any reason not to commit the patches that have been acked and > reviewed, so we can start getting stuff tested sooner (and reduce the > cognitive load on you of having to deal with 16 patches)? > > -GeorgeI would certainly appreciate a reduction of cognative load. From the series, I believe that patches 1-6 are sufficiently simple and obviously correct. ~Andrew
Andrew Cooper writes ("Re: [PATCH 4 00/16] XSA55 libelf fixes for
unstable"):> Xen warning: dom0 kernel broken ELF: program segments total to more
> than the input image size
I had a total brain fart when I wrote this, and forgot about
uninitialised data. I think this patch (added to the end of the
series) should fix it.
If you report success I intend to provide a v5 of my series with this
integrated in its proper place.
Thanks,
Ian.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/xen/common/libelf/libelf-loader.c
b/xen/common/libelf/libelf-loader.c
index fbc8de7..1e3c869 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -298,7 +298,12 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf)
ELF_HANDLE_DECL(elf_phdr) phdr;
uint64_t i, count, paddr, offset, filesz, memsz;
elf_ptrval dest;
- elf_ptrval remain_allow_copy = elf->size;
+ uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
+ /*
+ * 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).
+ */
count = elf_uval(elf, elf->ehdr, e_phnum);
for ( i = 0; i < count; i++ )
Ian Jackson
2013-Jun-05 11:54 UTC
Re: [PATCH 03/16] libelf: add `struct elf_binary*'' parameter to elf_load_image
George Dunlap writes ("Re: [Xen-devel] [PATCH 03/16] libelf: add `struct
elf_binary*'' parameter to
elf_load_image"):> Is there any reason not to commit the patches that have been acked and
> reviewed, so we can start getting stuff tested sooner
Not really except ...
> (and reduce the cognitive load on you of having to deal with 16
> patches)?
... that it will make advisories more confusing to write and backports
more confusing to do and actually make my life harder.
Ian.
Andrew Cooper
2013-Jun-05 14:51 UTC
Re: [PATCH 12/16] libelf: Make all callers call elf_check_broken
On 04/06/13 18:59, Ian Jackson wrote:> 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". > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > > v3.1: > Add error check to xc_dom_parse_elf_kernel. > Move check in xc_hvm_build_x86.c:setup_guest to right place. > > v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com> > v2 was Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > v2: Style fixes. > --- > tools/libxc/xc_dom_elfloader.c | 25 +++++++++++++++++++++---- > tools/libxc/xc_hvm_build_x86.c | 3 +++ > tools/xcutils/readnotes.c | 3 +++ > xen/arch/arm/kernel.c | 10 ++++++++++ > xen/arch/x86/domain_build.c | 28 +++++++++++++++++++++------- > 5 files changed, 58 insertions(+), 11 deletions(-) > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > index 177219f..f1abe15 100644 > --- a/tools/libxc/xc_dom_elfloader.c > +++ b/tools/libxc/xc_dom_elfloader.c > @@ -275,6 +275,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__); > @@ -311,19 +318,23 @@ 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; > + } > > if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) ) > { > xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does not" > " support unprivileged (DomU) operation", __FUNCTION__); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > /* find kernel segment */ > @@ -337,7 +348,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)); > +I know this is just nitpicking, but trailing whitespace. As far as I can tell, this is the only instance in the whole series ~Andrew> + return rc; > } > > static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) > diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c > index eff55a4..8bb0178 100644 > --- a/tools/libxc/xc_hvm_build_x86.c > +++ b/tools/libxc/xc_hvm_build_x86.c > @@ -524,6 +524,9 @@ static int setup_guest(xc_interface *xch, > error_out: > rc = -1; > out: > + if ( elf_check_broken(&elf) ) > + ERROR("HVM ELF broken: %s", elf_check_broken(&elf)); > + > /* ensure no unclaimed pages are left unused */ > xc_domain_claim_pages(xch, dom, 0 /* cancels the claim */); > > diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c > index ca86ba5..b868fba 100644 > --- a/tools/xcutils/readnotes.c > +++ b/tools/xcutils/readnotes.c > @@ -300,6 +300,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/arm/kernel.c b/xen/arch/arm/kernel.c > index 8f4a60d..43cf2ab 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -171,6 +171,8 @@ static int kernel_try_elf_prepare(struct kernel_info *info, > { > int rc; > > + memset(&info->elf.elf, 0, sizeof(info->elf.elf)); > + > info->kernel_order = get_order_from_bytes(size); > info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0); > if ( info->kernel_img == NULL ) > @@ -194,8 +196,16 @@ static int kernel_try_elf_prepare(struct kernel_info *info, > info->entry = info->elf.parms.virt_entry; > info->load = kernel_elf_load; > > + if ( elf_check_broken(&info->elf.elf) ) > + printk("Xen: warning: ELF kernel broken: %s\n", > + elf_check_broken(&info->elf.elf)); > + > return 0; > err: > + if ( elf_check_broken(&info->elf.elf) ) > + printk("Xen: ELF kernel broken: %s\n", > + elf_check_broken(&info->elf.elf)); > + > free_xenheap_pages(info->kernel_img, info->kernel_order); > return rc; > } > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c > index db31a91..03fe845 100644 > --- a/xen/arch/x86/domain_build.c > +++ b/xen/arch/x86/domain_build.c > @@ -380,7 +380,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; > @@ -408,14 +408,16 @@ int __init construct_dom0( > if ( !compatible ) > { > printk("Mismatch between Xen and DOM0 kernel\n"); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE && > !test_bit(XENFEAT_dom0, parms.f_supported) ) > { > printk("Kernel does not support Dom0 operation\n"); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > if ( compat32 ) > @@ -596,7 +598,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) ) > @@ -771,7 +774,7 @@ int __init construct_dom0( > if ( rc < 0 ) > { > printk("Failed to load the kernel binary\n"); > - return rc; > + goto out; > } > bootstrap_map(NULL); > > @@ -783,7 +786,8 @@ int __init construct_dom0( > mapcache_override_current(NULL); > 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); > @@ -1133,9 +1137,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; > } > > /*
Andrew Cooper
2013-Jun-05 15:31 UTC
Re: [PATCH 12/16] libelf: Make all callers call elf_check_broken
On 04/06/13 18:59, Ian Jackson wrote:> 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". > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > > v3.1: > Add error check to xc_dom_parse_elf_kernel. > Move check in xc_hvm_build_x86.c:setup_guest to right place. > > v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com> > v2 was Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > v2: Style fixes. > --- > tools/libxc/xc_dom_elfloader.c | 25 +++++++++++++++++++++---- > tools/libxc/xc_hvm_build_x86.c | 3 +++ > tools/xcutils/readnotes.c | 3 +++ > xen/arch/arm/kernel.c | 10 ++++++++++ > xen/arch/x86/domain_build.c | 28 +++++++++++++++++++++------- > 5 files changed, 58 insertions(+), 11 deletions(-) > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > index 177219f..f1abe15 100644 > --- a/tools/libxc/xc_dom_elfloader.c > +++ b/tools/libxc/xc_dom_elfloader.c > @@ -275,6 +275,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__); > @@ -311,19 +318,23 @@ 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; > + } > > if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) ) > { > xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does not" > " support unprivileged (DomU) operation", __FUNCTION__); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > /* find kernel segment */ > @@ -337,7 +348,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_x86.c b/tools/libxc/xc_hvm_build_x86.c > index eff55a4..8bb0178 100644 > --- a/tools/libxc/xc_hvm_build_x86.c > +++ b/tools/libxc/xc_hvm_build_x86.c > @@ -524,6 +524,9 @@ static int setup_guest(xc_interface *xch, > error_out: > rc = -1; > out: > + if ( elf_check_broken(&elf) ) > + ERROR("HVM ELF broken: %s", elf_check_broken(&elf)); > + > /* ensure no unclaimed pages are left unused */ > xc_domain_claim_pages(xch, dom, 0 /* cancels the claim */); > > diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c > index ca86ba5..b868fba 100644 > --- a/tools/xcutils/readnotes.c > +++ b/tools/xcutils/readnotes.c > @@ -300,6 +300,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/arm/kernel.c b/xen/arch/arm/kernel.c > index 8f4a60d..43cf2ab 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -171,6 +171,8 @@ static int kernel_try_elf_prepare(struct kernel_info *info, > { > int rc; > > + memset(&info->elf.elf, 0, sizeof(info->elf.elf)); > + > info->kernel_order = get_order_from_bytes(size); > info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0); > if ( info->kernel_img == NULL ) > @@ -194,8 +196,16 @@ static int kernel_try_elf_prepare(struct kernel_info *info, > info->entry = info->elf.parms.virt_entry; > info->load = kernel_elf_load; > > + if ( elf_check_broken(&info->elf.elf) ) > + printk("Xen: warning: ELF kernel broken: %s\n", > + elf_check_broken(&info->elf.elf)); > + > return 0; > err: > + if ( elf_check_broken(&info->elf.elf) ) > + printk("Xen: ELF kernel broken: %s\n", > + elf_check_broken(&info->elf.elf)); > + > free_xenheap_pages(info->kernel_img, info->kernel_order); > return rc; > } > diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c > index db31a91..03fe845 100644 > --- a/xen/arch/x86/domain_build.c > +++ b/xen/arch/x86/domain_build.c > @@ -380,7 +380,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; > @@ -408,14 +408,16 @@ int __init construct_dom0( > if ( !compatible ) > { > printk("Mismatch between Xen and DOM0 kernel\n"); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE && > !test_bit(XENFEAT_dom0, parms.f_supported) ) > { > printk("Kernel does not support Dom0 operation\n"); > - return -EINVAL; > + rc = -EINVAL; > + goto out; > } > > if ( compat32 ) > @@ -596,7 +598,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) ) > @@ -771,7 +774,7 @@ int __init construct_dom0( > if ( rc < 0 ) > { > printk("Failed to load the kernel binary\n"); > - return rc; > + goto out; > } > bootstrap_map(NULL); > > @@ -783,7 +786,8 @@ int __init construct_dom0( > mapcache_override_current(NULL); > 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); > @@ -1133,9 +1137,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));this should be if ( elf_check_broken(&elf) ) { printk(foo); return -EINVAL; } Otherwise we exit construct_dom0() with an rc of 0. During my testing, this involved dying with a general protection fault very shortly after the dom0 entry point. ~Andrew> > + 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; > } > > /*
On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> * 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.[snip]> -#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_size >= amount ) > + { > + elf->dest_base += amount; > + elf->dest_size -= amount; > + } > + else > + { > + elf->dest_size = 0; > + } > +}This is probably a minor thing, but is there a reason not to set elf->dest_base to NULL here? I realize that technically you''ve said "dest_size=0 -> dest_base is also invalid", but it never hurts to have a little extra safety. Other than that, I''ve tried to do a careful reading, and it seems like it does what it says on the tin. I have mostly looked at the code that *has* changed, not the code that has *not* changed, but the renaming of key structures and functions should have helped the compiler do that for us. It''s good to go in with or without the above suggestion as far as I can tell. Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com> -George
On Wed, Jun 5, 2013 at 5:59 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: [...]> @@ -128,19 +128,31 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, > > if ( load ) > { > + char *hdr_ptr; > + unsigned int page_size = XC_DOM_PAGE_SIZE(dom); > + > 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); > - *(int *)hdr = size - sizeof(int); > + hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start); > + elf->caller_xdest_base = hdr_ptr;This result needs to be checked against NULL, otherwise a silly dom->bsd_symtab_start will mistakenly allow access to the base of address space. [...]> @@ -309,8 +344,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) > { > struct elf_binary *elf = dom->private_loader; > int rc; > + 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);Similarly, but with a silly dom->kernel_seg. (crashsig 843a77d)
On Wed, Jun 5, 2013 at 5:59 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> This is version 4 of my (prematurely-released) series to try to fix > libelf. This version deals better with some possibly-out-of-control > loops, fixes the three so-far-known regressions, and should fix the > 32-bit ARM build.Looks like there''s another issue that needs fixing up in this XSA (surprise!): setup_hypercall_page (in xc_dom_boot.c) calls xc_dom_p2m_guest with an unchecked, user-controlled pfn: Starting program: /usr/local/sbin/xl create /dev/null kernel=\''check/_usr_lib_debug_vmlinux-3_2_0-4-amd64-37bdfe88206dbe6f75d9c6e021fd9b83c27814d2-5873-0_001 _1e-06-file.2.0-4-amd64\''\;\ name=\''blah\'' [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Parsing config from /dev/null Program received signal SIGSEGV, Segmentation fault. 0x00007ffff6d8cb01 in xc_dom_p2m_guest (pfn=<optimized out>, dom=0x626fd0) at xc_dom.h:338 338 return dom->p2m_host[pfn - dom->rambase_pfn]; (gdb) p pfn $1 = <optimized out> (gdb) p dom->rambase_pfn $2 = 0 (gdb) bt 5 #0 0x00007ffff6d8cb01 in xc_dom_p2m_guest (pfn=<optimized out>, dom=0x626fd0) at xc_dom.h:338 #1 setup_hypercall_page (dom=0x626fd0) at xc_dom_boot.c:56 #2 xc_dom_boot_image (dom=dom@entry=0x626fd0) at xc_dom_boot.c:260 #3 0x00007ffff799b85a in libxl__build_pv (gc=gc@entry=0x629390, domid=domid@entry=9368, info=info@entry=0x7fffffffdd00, state=state@entry=0x625a80) at libxl_dom.c:399 #4 0x00007ffff7991887 in libxl__domain_build (gc=0x629390, d_config=d_config@entry=0x7fffffffdcc0, domid=9368, state=state@entry=0x625a80) at libxl_create.c:349 (More stack frames follow...) (gdb) fr 1 #1 setup_hypercall_page (dom=0x626fd0) at xc_dom_boot.c:56 56 domctl.u.hypercall_init.gmfn = xc_dom_p2m_guest(dom, pfn); (gdb) p pfn $3 = <optimized out> (gdb) l 51 52 DOMPRINTF("%s: vaddr=0x%" PRIx64 " pfn=0x%" PRIpfn "", __FUNCTION__, 53 dom->parms.virt_hypercall, pfn); 54 domctl.cmd = XEN_DOMCTL_hypercall_init; 55 domctl.domain = dom->guest_domid; 56 domctl.u.hypercall_init.gmfn = xc_dom_p2m_guest(dom, pfn); 57 rc = do_domctl(dom->xch, &domctl); 58 if ( rc != 0 ) 59 xc_dom_panic(dom->xch, 60 XC_INTERNAL_ERROR, "%s: HYPERCALL_INIT failed (rc=%d)", (gdb) l - 41 static int setup_hypercall_page(struct xc_dom_image *dom) 42 { 43 DECLARE_DOMCTL; 44 xen_pfn_t pfn; 45 int rc; 46 47 if ( dom->parms.virt_hypercall == -1 ) 48 return 0; 49 pfn = (dom->parms.virt_hypercall - dom->parms.virt_base) 50 >> XC_DOM_PAGE_SHIFT(dom); (gdb) p dom->parms.virt_hypercall $4 = 0 (gdb) p dom->parms.virt_base $5 = 18446744071562067968 (gdb) p/x dom->parms.virt_base $6 = 0xffffffff80000000 Here, the silly dom->parms.virt_base is leading to an out-of-bounds array access to the guest p2m table. (Also, perhaps dom->parms.virt_hypercall should be being compared to UNSET_ADDR, not -1, on line 47.) - Matthew
George Dunlap
2013-Jun-06 14:08 UTC
Re: [PATCH 12/16] libelf: Make all callers call elf_check_broken
On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c > index ca86ba5..b868fba 100644 > --- a/tools/xcutils/readnotes.c > +++ b/tools/xcutils/readnotes.c > @@ -300,6 +300,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)); > +This seems to have some ghastly combination of spaces and tabs that make it render strangely if tabsize != 8. (Understandable as the whole file is full of tabs...) Other than that, the changes look good and sensible. I haven''t done a survey of the code to see what may have been missed, however. Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com> -G
On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> 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.So the main point here is to avoid false positives, as opposed to actually fixing any potential bugs? I.e., we can say, "There are absolutely no int''s anywhere in the code" and can stop worrying about int-related bugs? Or do you think there may be actual overflow conditions that might happen when using an int as a boolean? All the changes look fine to me: Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
George Dunlap writes ("Re: [Xen-devel] [PATCH 13/16] libelf: use C99 bool
for booleans"):> On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson
<ian.jackson@eu.citrix.com> wrote:
> > 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.
>
> So the main point here is to avoid false positives, as opposed to
> actually fixing any potential bugs? I.e., we can say, "There are
> absolutely no int''s anywhere in the code" and can stop
worrying about
> int-related bugs?
Exactly.
> Or do you think there may be actual overflow conditions that might
> happen when using an int as a boolean?
No.
> All the changes look fine to me:
>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Thanks,
Ian.
George Dunlap writes ("Re: [Xen-devel] [PATCH 10/16] libelf: check all
pointer accesses"):> On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson
<ian.jackson@eu.citrix.com> wrote:
> > +{
> > + if ( elf->dest_size >= amount )
> > + {
> > + elf->dest_base += amount;
> > + elf->dest_size -= amount;
> > + }
> > + else
> > + {
> > + elf->dest_size = 0;
> > + }
> > +}
>
> This is probably a minor thing, but is there a reason not to set
> elf->dest_base to NULL here?
Well, there''s nothing really wrong with elf->dest_base apart from
that
it points to a zero-length area.
But your comment makes me realise that we should call elf_mark_broken
in this case.
> I realize that technically you''ve said "dest_size=0 ->
dest_base is
> also invalid", but it never hurts to have a little extra safety.
If there is any code which accesses elf->dest_base[] without checking
elf->dest_size then that code is already a problem.
Making the change you propose would raise questions about whether (eg)
some other code somewhere might think dest_base==0 means something
special. (I don''t think it does, but it''s an argument against
changing things.)
> Other than that, I''ve tried to do a careful reading, and it seems
like
> it does what it says on the tin. I have mostly looked at the code
> that *has* changed, not the code that has *not* changed, but the
> renaming of key structures and functions should have helped the
> compiler do that for us. It''s good to go in with or without the
above
> suggestion as far as I can tell.
Thanks.
Ian.
Matthew Daley writes ("Re: [PATCH 10/16] libelf: check all pointer
accesses"):> This result needs to be checked against NULL, otherwise a silly
> dom->bsd_symtab_start will mistakenly allow access to the base of
> address space.
Urgh. I bet there''s a lot of this about...
Ian.
On Fri, 2013-06-07 at 00:25 +1200, Matthew Daley wrote:> On Wed, Jun 5, 2013 at 5:59 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: > [...] > > @@ -128,19 +128,31 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, > > > > if ( load ) > > { > > + char *hdr_ptr; > > + unsigned int page_size = XC_DOM_PAGE_SIZE(dom); > > + > > 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); > > - *(int *)hdr = size - sizeof(int); > > + hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start); > > + elf->caller_xdest_base = hdr_ptr; > > This result needs to be checked against NULL, otherwise a silly > dom->bsd_symtab_start will mistakenly allow access to the base of > address space.I think this may be looking up the address previously allocated in the ! load else case of this if using bsd_symtab_start as a key. The allocation in that else case is properly checked. Given that bsd_symtab_start cannot be changed under our feet if I''m right then the only way you could get NULL here would be if our internal bookkeeping had gotten messed up. IOW I think abort() would be an appropriate response.> [...] > > @@ -309,8 +344,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) > > { > > struct elf_binary *elf = dom->private_loader; > > int rc; > > + 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); > > Similarly, but with a silly dom->kernel_seg.I don''t know whether or not this one is "prevalidated" like the vaddr case.> (crashsig 843a77d)Does this indicate that you''ve observed the xc_dom_vaddr or the xc_dom_seg one or both? Obviously if it is the former or both then I am talking out my behind... Ian.
On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > index 605b1e0..60000da 100644 > --- a/tools/libxc/xc_dom_elfloader.c > +++ b/tools/libxc/xc_dom_elfloader.c > @@ -82,7 +82,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 ) > { > @@ -104,12 +104,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; > @@ -117,7 +117,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) ) > { > @@ -139,13 +139,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, > elf->caller_xdest_size = page_size - > (dom->bsd_symtab_start & (page_size-1)); > 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);Something about this s/sizeof(int)/sizeof(unsigned)/; bit seems a bit weird to me. This is reading a standard data format, not just communicating between two bits of code we control, right? Can we be absolutely certain that forevermore, sizeof(int) and sizeof(unsigned) are the same (and will be the same size as the elements of the elf binary)? Alternately, was the old code wrong and the new code right? (This goes for all such changes in this file)> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c > index c4ced67..a9a5f41 100644 > --- a/xen/common/libelf/libelf-dominfo.c > +++ b/xen/common/libelf/libelf-dominfo.c[snip]> @@ -246,12 +246,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;This function has comparisons like the following: if ( len >= sizeof(value)-1 ) break; Are we certain that, for instance, sizeof() cannot be 0? According to this stackoverfow article, gcc allows empty structs which will have a sizeof() zero: http://stackoverflow.com/questions/2632021/can-sizeof-return-0-zero In this case of course we''ve hard-coded the values in there, so in this particular case it''s OK. Something to keep an eye out for...> @@ -495,13 +495,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 == ~0U ) > return -1;Rather than have this somewhat magical "~0U" value copied all over, would it make more sense to have a #define? Maybe ELF_PARSE_NOTES_INVAL or something? Other than that, I''ve tried to take a look at all of the changed variables and I think making them unsigned should be fine. -George
On Thu, Jun 6, 2013 at 3:51 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:>> I realize that technically you''ve said "dest_size=0 -> dest_base is >> also invalid", but it never hurts to have a little extra safety. > > If there is any code which accesses elf->dest_base[] without checking > elf->dest_size then that code is already a problem. > > Making the change you propose would raise questions about whether (eg) > some other code somewhere might think dest_base==0 means something > special. (I don''t think it does, but it''s an argument against > changing things.)Yes, of course there is a problem; what I was suggesting was, supposing there were such a problem, which would be most likely to show it up in a helpful way -- setting dest_base to 0, or leaving it to what is (at the moment) a valid address? We could also, I suppose, do "dest_base+=dest_size" before setting dest_size to 0. Then dest_base in theory shouldn''t be a valid address either. -George
George Dunlap writes ("Re: [Xen-devel] [PATCH 10/16] libelf: check all
pointer accesses"):> On Thu, Jun 6, 2013 at 3:51 PM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> > Making the change you propose would raise questions about whether (eg)
> > some other code somewhere might think dest_base==0 means something
> > special. (I don''t think it does, but it''s an
argument against
> > changing things.)
>
> Yes, of course there is a problem; what I was suggesting was,
> supposing there were such a problem, which would be most likely to
> show it up in a helpful way -- setting dest_base to 0, or leaving it
> to what is (at the moment) a valid address?
Setting it to 0 would be semantically wrong for that other code.
Leaving it as a non-0 value would be right. All the places where it''s
used have been changed to check the dest_size too.
> We could also, I suppose, do "dest_base+=dest_size" before
setting
> dest_size to 0. Then dest_base in theory shouldn''t be a valid
address
> either.
That would be Undefined Behaviour. If we did that the compiler might
be "entitled" to "prove" false things about our code, and
compile it.
Ian.
George Dunlap writes ("Re: [Xen-devel] [PATCH 14/16] libelf: use only
unsigned integers"):> On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson
<ian.jackson@eu.citrix.com> wrote:
> > diff --git a/tools/libxc/xc_dom_elfloader.c
b/tools/libxc/xc_dom_elfloader.
> > - size = sizeof(int) + elf_size(elf, elf->ehdr) +
> > + size = sizeof(unsigned) + elf_size(elf, elf->ehdr) +
> > elf_shdr_count(elf) * elf_size(elf, shdr);
>
> Something about this s/sizeof(int)/sizeof(unsigned)/; bit seems a bit
> weird to me. This is reading a standard data format, not just
> communicating between two bits of code we control, right?
>
> Can we be absolutely certain that forevermore, sizeof(int) and
> sizeof(unsigned) are the same (and will be the same size as the
> elements of the elf binary)? Alternately, was the old code wrong and
> the new code right?
Yes, we can be sure that sizeof(int)==sizeof(unsigned), forever and
always. If they weren''t the right size as the things in the elf then
the code was broken before :-).
I''m changing this so you can grep for \bint\b afterwards and see a
very small number of hits left.
> > -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;
>
> This function has comparisons like the following:
>
> if ( len >= sizeof(value)-1 )
> break;
>
> Are we certain that, for instance, sizeof() cannot be 0?
Yes. sizeof(value) cannot be 0 unless someone changes value[128] to
value[0].
> According to this stackoverfow article, gcc allows empty structs which
> will have a sizeof() zero:
>
> http://stackoverflow.com/questions/2632021/can-sizeof-return-0-zero
But value is a char array of nonzero size.
> In this case of course we''ve hard-coded the values in there, so in
> this particular case it''s OK. Something to keep an eye out for...
Indeed.
> > - 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 == ~0U )
> > return -1;
>
> Rather than have this somewhat magical "~0U" value copied all
over,
> would it make more sense to have a #define? Maybe
> ELF_PARSE_NOTES_INVAL or something?
OK, I can do that.
> Other than that, I''ve tried to take a look at all of the changed
> variables and I think making them unsigned should be fine.
Good, thanks.
Ian.
Matthew Daley writes ("Re: [PATCH 4 00/16] XSA55 libelf fixes for
unstable"):> Looks like there''s another issue that needs fixing up in this XSA
> (surprise!):
Urgh.
> setup_hypercall_page (in xc_dom_boot.c) calls xc_dom_p2m_guest with an
> unchecked, user-controlled pfn:
...> Here, the silly dom->parms.virt_base is leading to an out-of-bounds
> array access to the guest p2m table.
Thanks. I have a proposed fix for this, below. I haven''t tested it.
Can you do so easily ? It seems a bit remote from the problem but I
think it should suffice.
I searched the code for (a) other callers of xc_dom_p2m_{guest,host}
and (b) other uses of the parms field from the ELF and I think the
rest are probably OK, although I didn''t spend a great deal of time on
it.
> (Also, perhaps
> dom->parms.virt_hypercall should be being compared to UNSET_ADDR, not
> -1, on line 47.)
Ian.
From: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH] libxc: range checks in xc_dom_p2m_host and _guest
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.
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 567913f..239ef33 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -341,6 +341,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->rambase_pfn + dom->total_pages)
+ return INVALID_MFN;
return dom->p2m_host[pfn - dom->rambase_pfn];
}
@@ -349,6 +351,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->rambase_pfn + dom->total_pages)
+ return INVALID_MFN;
return dom->p2m_host[pfn - dom->rambase_pfn];
}
--
tg: (28e5cf0..) xsa55/xc_dom_p2m-check (depends on: xsa55/xc-malloc-check)
Ian Jackson
2013-Jun-06 18:41 UTC
Re: [PATCH 12/16] libelf: Make all callers call elf_check_broken
George Dunlap writes ("Re: [Xen-devel] [PATCH 12/16] libelf: Make all
callers call elf_check_broken"):> On Tue, Jun 4, 2013 at 6:59 PM, Ian Jackson
<ian.jackson@eu.citrix.com> wrote:
> > diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
> > index ca86ba5..b868fba 100644
> > --- a/tools/xcutils/readnotes.c
> > +++ b/tools/xcutils/readnotes.c
> > @@ -300,6 +300,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));
> > +
>
> This seems to have some ghastly combination of spaces and tabs that
> make it render strangely if tabsize != 8. (Understandable as the
> whole file is full of tabs...)
I have made my new line have tabs like the rest of the file.
> Other than that, the changes look good and sensible. I haven''t
done a
> survey of the code to see what may have been missed, however.
>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Thanks,
Ian.
On Fri, Jun 7, 2013 at 6:39 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Matthew Daley writes ("Re: [PATCH 4 00/16] XSA55 libelf fixes for unstable"): >> Looks like there''s another issue that needs fixing up in this XSA >> (surprise!): > > Urgh. > >> setup_hypercall_page (in xc_dom_boot.c) calls xc_dom_p2m_guest with an >> unchecked, user-controlled pfn: > ... >> Here, the silly dom->parms.virt_base is leading to an out-of-bounds >> array access to the guest p2m table. > > Thanks. I have a proposed fix for this, below. I haven''t tested it. > Can you do so easily ? It seems a bit remote from the problem but I > think it should suffice.Seems to work now in v5: Starting program: /usr/local/sbin/xl create /dev/null kernel=\''check/_usr_lib_debug_vmlinux-3_2_0-4-amd64-37bdfe88206dbe6f75d9c6e021fd9b83c27814d2-5873-0_001_1e-06-file.2.0-4-amd64\''\;\ name=\''poop\'' [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Parsing config from /dev/null xc: error: panic: xc_dom_boot.c:61: setup_hypercall_page: HYPERCALL_INIT failed (rc=-1): Internal error libxl: error: libxl_dom.c:400:libxl__build_pv: xc_dom_boot_image failed: Permission denied libxl: error: libxl_create.c:900:domcreate_rebuild_done: cannot (re-)build domain: -3 [Inferior 1 (process 5459) exited with code 03]
On Fri, Jun 7, 2013 at 2:59 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Matthew Daley writes ("Re: [PATCH 10/16] libelf: check all pointer accesses"): >> This result needs to be checked against NULL, otherwise a silly >> dom->bsd_symtab_start will mistakenly allow access to the base of >> address space. > > Urgh. I bet there''s a lot of this about...Fixed (at least this specific instance ;) ) in v5.
On Fri, Jun 7, 2013 at 3:30 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Fri, 2013-06-07 at 00:25 +1200, Matthew Daley wrote: >> On Wed, Jun 5, 2013 at 5:59 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >> [...] >> > @@ -128,19 +128,31 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, >> > >> > if ( load ) >> > { >> > + char *hdr_ptr; >> > + unsigned int page_size = XC_DOM_PAGE_SIZE(dom); >> > + >> > 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); >> > - *(int *)hdr = size - sizeof(int); >> > + hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start); >> > + elf->caller_xdest_base = hdr_ptr; >> >> This result needs to be checked against NULL, otherwise a silly >> dom->bsd_symtab_start will mistakenly allow access to the base of >> address space. > > I think this may be looking up the address previously allocated in the ! > load else case of this if using bsd_symtab_start as a key.That xc_dom_malloc function doesn''t allocate pages for xc_dom_pfn_to_ptr to use, it''s just a GC''d wrapper around malloc.> The > allocation in that else case is properly checked. Given that > bsd_symtab_start cannot be changed under our feet if I''m right then the > only way you could get NULL here would be if our internal bookkeeping > had gotten messed up.In the case I observed, bsd_symtab_start was just calculated with a silly value to begin with.> > IOW I think abort() would be an appropriate response. > >> [...] >> > @@ -309,8 +344,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) >> > { >> > struct elf_binary *elf = dom->private_loader; >> > int rc; >> > + 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); >> >> Similarly, but with a silly dom->kernel_seg. > > I don''t know whether or not this one is "prevalidated" like the vaddr > case.I could have sworn I saw a crash that had dest_base set to NULL (presumably from this assignment), but I can''t find one any more...> >> (crashsig 843a77d) > > Does this indicate that you''ve observed the xc_dom_vaddr or the > xc_dom_seg one or both? > > Obviously if it is the former or both then I am talking out my behind... >Just the former (apparently).
>>> On 06.06.13 at 20:14, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > George Dunlap writes ("Re: [Xen-devel] [PATCH 14/16] libelf: use only unsigned > integers"): >> On Tue, Jun 4, 2013 at 7:00 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader. >> > - size = sizeof(int) + elf_size(elf, elf->ehdr) + >> > + size = sizeof(unsigned) + elf_size(elf, elf->ehdr) + >> > elf_shdr_count(elf) * elf_size(elf, shdr); >> >> Something about this s/sizeof(int)/sizeof(unsigned)/; bit seems a bit >> weird to me. This is reading a standard data format, not just >> communicating between two bits of code we control, right? >> >> Can we be absolutely certain that forevermore, sizeof(int) and >> sizeof(unsigned) are the same (and will be the same size as the >> elements of the elf binary)? Alternately, was the old code wrong and >> the new code right? > > Yes, we can be sure that sizeof(int)==sizeof(unsigned), forever and > always. If they weren''t the right size as the things in the elf then > the code was broken before :-). > > I''m changing this so you can grep for \bint\b afterwards and see a > very small number of hits left.But there''s a point in what George says nevertheless: What is this sizeof(int) (now sizeof(unsigned)) referring to anyway? Going through the whole function at the end of the patch series, I had a pretty hard time finding that this is being used in the if() portion of the else body that gets modified here. Noticing right away that there''s possibly truncation here - the field ought to be size_t. And that would make it desirable to switch to sizeof(size), making clear which item it is that the size is needed of. Yet of course with this kind of coding done originally, finding _all_ consumers of this is going to be non-trivial - this should have been some sort of structure to begin with. And there doesn''t seem to be a load equivalent of elf_store_val() (and also elf_store_field()) - am I overlooking something? What''s the point of storing things that never get read? There appears to be a similar problem in elf_load_bsdsyms(), using uint32_t instead of elf_ptrval. Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH 14/16] libelf: use only
unsigned integers"):> On 06.06.13 at 20:14, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Yes, we can be sure that sizeof(int)==sizeof(unsigned), forever and
> > always. If they weren''t the right size as the things in the
elf then
> > the code was broken before :-).
> >
> > I''m changing this so you can grep for \bint\b afterwards and
see a
> > very small number of hits left.
>
> But there''s a point in what George says nevertheless: What is this
> sizeof(int) (now sizeof(unsigned)) referring to anyway?
Yes, I agree that there is a point. However what we are trying to do
here is prepare a security patch.
Fixes to braindamage of this kind should not appear in security
patches because security patches shouldn''t change anything they
don''t
need to.
Anything which is broken before the security patch can safely remain
broken afterwards.
Ian.
>>> On 07.06.13 at 16:35, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [Xen-devel] [PATCH 14/16] libelf: use only unsigned > integers"): >> On 06.06.13 at 20:14, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: >> > Yes, we can be sure that sizeof(int)==sizeof(unsigned), forever and >> > always. If they weren''t the right size as the things in the elf then >> > the code was broken before :-). >> > >> > I''m changing this so you can grep for \bint\b afterwards and see a >> > very small number of hits left. >> >> But there''s a point in what George says nevertheless: What is this >> sizeof(int) (now sizeof(unsigned)) referring to anyway? > > Yes, I agree that there is a point. However what we are trying to do > here is prepare a security patch. > > Fixes to braindamage of this kind should not appear in security > patches because security patches shouldn''t change anything they don''t > need to.As long as you''re convinced that this doesn''t have an impact on security, I agree.> Anything which is broken before the security patch can safely remain > broken afterwards.Sure. Jan