|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 10/27] xsplice: Add helper elf routines
> > +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data)
> > +{
..snip..
> > + /* e_shoff and e_shnum overflow checks are done in
> > xsplice_header_check. */
> > + delta = elf->hdr->e_shoff + elf->hdr->e_shnum * elf->hdr->e_shentsize;
>
> The added comment just helps make obvious that the overflow I
> believe Andrew was worried about is still not being taken care of:
> All xsplice_header_check() does is range check the two values
> mentioned in the comment. But I agree that a proper range check
> (at once eliminating overflow concerns for the arithmetic here)
> would better live there (and also see there).
..snip..
> > +static int xsplice_header_check(const struct xsplice_elf *elf)
> > +{
..snip..
> > + if ( elf->hdr->e_shnum > 64 )
> > + {
> > + dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
> > + elf->name, elf->hdr->e_shnum);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if ( elf->hdr->e_shoff > ULONG_MAX )
>
> Why not ">= elf->len" (and I see it was almost that way in v8.1)?
I misunderstood your comment. You mentioned to me that we have
an boundary check here (when it was against elf->len) and that you
wanted an overflow - so I replaced it - while you meant - in addition to.
But adding in both:
elf->hdr->e_shoff >= ULONG_MAX || elf->hdr->e_shoff >= elf->len
feels unneccessary. And the boundary check is more imporant.
I added both in the code.
> And then followed (further down) by another check taking
> elf->hdr->e_shnum * elf->hdr->e_shentsize into account (of
> course as things stand now, elf->hdr->e_shentsize can also be
> arbitrarily large, so this would need to be suitably structured
> - e.g. "(elf->len - elf->hdr->e_shoff) / elf->hdr->e_shentsize <
> elf->hdr->e_shnum").
Ah, so that is how you want to check for e_shnum!
Here is the updated patch:
From eb90312d4ff66c17fad2d4cd5379974fc4c9f2f6 Mon Sep 17 00:00:00 2001
From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Date: Fri, 19 Feb 2016 14:37:17 -0500
Subject: [PATCH] xsplice: Add helper elf routines
Add Elf routines and data structures in preparation for loading an
xSplice payload.
We make an assumption that the max number of sections an ELF payload
can have is 64. We can in future make this be dependent on the
names of the sections and verifying against a list, but for right now
this suffices.
Also we a whole lot of checks to make sure that the ELF payload
file is not corrupted nor that the offsets point past the file.
For most of the checks we print an message if the hypervisor is built
with debug enabled.
Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Keir Fraser <keir@xxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
v2: - With the #define ELFSIZE in the ARM file we can use the common
#defines instead of using #ifdef CONFIG_ARM_32. Moved to another
patch.
- Add checks for ELF file.
- Add name to be printed.
- Add len for easier ELF checks.
- Expand on the checks. Add macro.
v3: Remove the return_ macro
- Add return_ macro back but make it depend on debug=y
- Per Andrew review: add local variable. Fix memory leak in
elf_resolve_sections, Remove macro and use dprintk. Fix alignment.
Use void* instead of uint8_t to handle raw payload.
v4 - Fix memory leak in elf_get_sym
- Add XSPLICE to printk/dprintk
v5: Sprinkle newlines.
v6: Squash the ELF header checks from 'xsplice: Implement payload loading' here,
Do better job at checking string sections and the users of them (sh_size),
Use XSPLICE as a string literal,
Move some checks outside the loop,
Make sure that SHT_STRTAB are really what they say
Sprinkle consts.
v7:
Check sh_entsize and sh_offset.
Added Andrew's Reviewed-by and Ian's Acked-by
Redo check on sh_entsize to not be !=
v8: Make all the dprintk(XENLOG_DEBUG be XENLOG_ERR
v9: Changed elf_verify_strtab to use const char and return EINVAL.
Remove 'if ( !delta )' check in elf_resolve_sections
Remove stale comments.
Fixed one off check against sh_link.
Document boundary checks against shstrtab and symtab.
Fixed return codes in xsplice_header_check.
Add check for sections to not be within ELF header.
Added overflow check for e_shoff in xsplice_header_check.
Moved XSPLICE macro by four tabs.
Make ->sym be const.
v10:
- Change the check against 64 to be against SHN_LORESERVE
- Remove Reviewed-by
- In elf_resolve_sections skip delta check if SHT_NOBITS is set in
second conditional.
- In elf_get_sym use symtab_sec->sec->sh_entsize to access
Elf_Sym symbols and also make it a const. Also
fix boundary check against .strtab and make assigment of
sym[i].name more natural.
- In xsplice_header_check add comment about EI_CLASS and e_flags
being platform specific. Check against e_version and EI_VERSION.
Also reinstate elf->hdr->e_shoff >= elf->len check. Add Jan's check against
elf->hdr->e_shnum * elf->hdr->e_shentsize
---
xen/common/Makefile | 1 +
xen/common/xsplice_elf.c | 375 ++++++++++++++++++++++++++++++++++++++++++
xen/include/xen/xsplice.h | 3 +
xen/include/xen/xsplice_elf.h | 51 ++++++
4 files changed, 430 insertions(+)
create mode 100644 xen/common/xsplice_elf.c
create mode 100644 xen/include/xen/xsplice_elf.h
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 1e4bc70..afd84b6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -59,6 +59,7 @@ obj-y += wait.o
obj-$(CONFIG_XENOPROF) += xenoprof.o
obj-y += xmalloc_tlsf.o
obj-$(CONFIG_XSPLICE) += xsplice.o
+obj-$(CONFIG_XSPLICE) += xsplice_elf.o
obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo
unlz4 earlycpio,$(n).init.o)
diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
new file mode 100644
index 0000000..702d43e
--- /dev/null
+++ b/xen/common/xsplice_elf.c
@@ -0,0 +1,375 @@
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/xsplice_elf.h>
+#include <xen/xsplice.h>
+
+const struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf
*elf,
+ const char *name)
+{
+ unsigned int i;
+
+ for ( i = 1; i < elf->hdr->e_shnum; i++ )
+ {
+ if ( !strcmp(name, elf->sec[i].name) )
+ return &elf->sec[i];
+ }
+
+ return NULL;
+}
+
+static int elf_verify_strtab(const struct xsplice_elf_sec *sec)
+{
+ const Elf_Shdr *s;
+ const char *contents;
+
+ s = sec->sec;
+
+ if ( s->sh_type != SHT_STRTAB )
+ return -EINVAL;
+
+ if ( !s->sh_size )
+ return -EINVAL;
+
+ contents = sec->data;
+
+ if ( contents[0] || contents[s->sh_size - 1] )
+ return -EINVAL;
+
+ return 0;
+}
+
+static int elf_resolve_sections(struct xsplice_elf *elf, const void *data)
+{
+ struct xsplice_elf_sec *sec;
+ unsigned int i;
+ Elf_Off delta;
+ int rc;
+
+ /* xsplice_elf_load sanity checked e_shnum. */
+ sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum);
+ if ( !sec )
+ {
+ dprintk(XENLOG_ERR, XSPLICE"%s: Could not allocate memory for section
table!\n",
+ elf->name);
+ return -ENOMEM;
+ }
+
+ elf->sec = sec;
+
+ /* e_shoff and e_shnum overflow checks are done in xsplice_header_check. */
+ delta = elf->hdr->e_shoff + elf->hdr->e_shnum * elf->hdr->e_shentsize;
+ if ( delta > elf->len )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of
payload!\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ for ( i = 1; i < elf->hdr->e_shnum; i++ )
+ {
+ delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
+
+ sec[i].sec = data + delta;
+
+ delta = sec[i].sec->sh_offset;
+ /*
+ * N.B. elf_resolve_section_names, elf_get_sym skip this check as
+ * we do it here.
+ */
+ if ( delta < sizeof(Elf_Ehdr) ||
+ (sec[i].sec->sh_type != SHT_NOBITS && /* Skip SHT_NOBITS */
+ (delta > elf->len || (delta + sec[i].sec->sh_size > elf->len))) )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section [%u] data %s of
payload!\n",
+ elf->name, i,
+ delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past
end");
+ return -EINVAL;
+ }
+
+ sec[i].data = data + delta;
+ /* Name is populated in elf_resolve_section_names. */
+ sec[i].name = NULL;
+
+ if ( sec[i].sec->sh_type == SHT_SYMTAB )
+ {
+ if ( elf->symtab )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported multiple symbol
tables!\n",
+ elf->name);
+ return -EOPNOTSUPP;
+ }
+
+ elf->symtab = &sec[i];
+
+ /*
+ * elf->symtab->sec->sh_link would point to the right section
+ * but we hadn't finished parsing all the sections.
+ */
+ if ( elf->symtab->sec->sh_link >= elf->hdr->e_shnum )
+ {
+ dprintk(XENLOG_ERR, XSPLICE
+ "%s: Symbol table idx (%u) to strtab past end (%u)\n",
+ elf->name, elf->symtab->sec->sh_link,
+ elf->hdr->e_shnum);
+ return -EINVAL;
+ }
+ }
+ }
+
+ if ( !elf->symtab )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: No symbol table found!\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ if ( !elf->symtab->sec->sh_size ||
+ elf->symtab->sec->sh_entsize < sizeof(Elf_Sym) ||
+ elf->symtab->sec->sh_size % elf->symtab->sec->sh_entsize )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Symbol table header is corrupted!\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ /*
+ * There can be multiple SHT_STRTAB (.shstrtab, .strtab) so pick the one
+ * associated with the symbol table.
+ */
+ elf->strtab = &sec[elf->symtab->sec->sh_link];
+
+ rc = elf_verify_strtab(elf->strtab);
+ if ( rc )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: String table section is corrupted\n",
+ elf->name);
+ }
+
+ return rc;
+}
+
+static int elf_resolve_section_names(struct xsplice_elf *elf, const void *data)
+{
+ const char *shstrtab;
+ unsigned int i;
+ Elf_Off offset, delta;
+ struct xsplice_elf_sec *sec;
+ int rc;
+
+ /*
+ * The elf->sec[0 -> e_shnum] structures have been verified by
+ * elf_resolve_sections. Find file offset for section string table
+ * (normally called .shstrtab)
+ */
+ sec = &elf->sec[elf->hdr->e_shstrndx];
+
+ rc = elf_verify_strtab(sec);
+ if ( rc )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is corrupted\n",
+ elf->name);
+ return rc;
+ }
+
+ /* Verified in elf_resolve_sections but just in case. */
+ offset = sec->sec->sh_offset;
+ ASSERT(offset < elf->len && (offset + sec->sec->sh_size <= elf->len));
+
+ shstrtab = data + offset;
+
+ for ( i = 1; i < elf->hdr->e_shnum; i++ )
+ {
+ delta = elf->sec[i].sec->sh_name;
+
+ /* Boundary check on offset of name within the .shstrtab. */
+ if ( delta >= sec->sec->sh_size )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end of
payload!\n",
+ elf->name, i);
+ return -EINVAL;
+ }
+
+ elf->sec[i].name = shstrtab + delta;
+ }
+
+ return 0;
+}
+
+static int elf_get_sym(struct xsplice_elf *elf, const void *data)
+{
+ const struct xsplice_elf_sec *symtab_sec, *strtab_sec;
+ struct xsplice_elf_sym *sym;
+ unsigned int i, delta, offset, nsym;
+
+ symtab_sec = elf->symtab;
+ strtab_sec = elf->strtab;
+
+ /* Pointers arithmetic to get file offset. */
+ offset = strtab_sec->data - data;
+
+ /* Checked already in elf_resolve_sections, but just in case. */
+ ASSERT(offset == strtab_sec->sec->sh_offset);
+ ASSERT(offset < elf->len && (offset + strtab_sec->sec->sh_size <=
elf->len));
+
+ /* symtab_sec->data was computed in elf_resolve_sections. */
+ ASSERT((symtab_sec->sec->sh_offset + data) == symtab_sec->data);
+
+ /* No need to check values as elf_resolve_sections did it. */
+ nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
+
+ sym = xmalloc_array(struct xsplice_elf_sym, nsym);
+ if ( !sym )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Could not allocate memory for
symbols\n",
+ elf->name);
+ return -ENOMEM;
+ }
+
+ /* So we don't leak memory. */
+ elf->sym = sym;
+
+ for ( i = 1; i < nsym; i++ )
+ {
+ const Elf_Sym *s = symtab_sec->data + symtab_sec->sec->sh_entsize * i;
+
+ delta = s->st_name;
+ /* Boundary check within the .strtab. */
+ if ( delta >= strtab_sec->sec->sh_size )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] name is not within
.strtab!\n",
+ elf->name, i);
+ return -EINVAL;
+ }
+
+ sym[i].sym = s;
+ sym[i].name = strtab_sec->data + delta;
+ }
+ elf->nsym = nsym;
+
+ return 0;
+}
+
+static int xsplice_header_check(const struct xsplice_elf *elf)
+{
+ const Elf_Ehdr *hdr = elf->hdr;
+
+ if ( sizeof(*elf->hdr) > elf->len )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than
payload!\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ if ( !IS_ELF(*hdr) )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
+ return -EINVAL;
+ }
+
+ /* EI_CLASS, and e_flags are platform specific. */
+ if ( hdr->e_version != EV_CURRENT ||
+ hdr->e_ident[EI_VERSION] != EV_CURRENT ||
+ hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
+ hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
+ hdr->e_type != ET_REL ||
+ hdr->e_phnum != 0 )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
+ return -EOPNOTSUPP;
+ }
+
+ if ( elf->hdr->e_shstrndx == SHN_UNDEF )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ /* Check that section name index is within the sections. */
+ if ( elf->hdr->e_shstrndx >= elf->hdr->e_shnum )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx (%u) is past end of
sections (%u)!\n",
+ elf->name, elf->hdr->e_shstrndx, elf->hdr->e_shnum);
+ return -EINVAL;
+ }
+
+ if ( elf->hdr->e_shnum >= SHN_LORESERVE )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
+ elf->name, elf->hdr->e_shnum);
+ return -EOPNOTSUPP;
+ }
+
+ if ( elf->hdr->e_shoff >= elf->len || elf->hdr->e_shoff >= ULONG_MAX )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
+ return -EINVAL;
+ }
+
+ if ( elf->hdr->e_shentsize < sizeof(Elf_Shdr) )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is %u! Expected
%zu!?\n",
+ elf->name, elf->hdr->e_shentsize, sizeof(Elf_Shdr));
+ return -EINVAL;
+ }
+
+ if ( ((elf->len - elf->hdr->e_shoff) / elf->hdr->e_shentsize) <
+ elf->hdr->e_shnum )
+ {
+ dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is corrupted!\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int xsplice_elf_load(struct xsplice_elf *elf, const void *data)
+{
+ int rc;
+
+ elf->hdr = data;
+
+ rc = xsplice_header_check(elf);
+ if ( rc )
+ return rc;
+
+ rc = elf_resolve_sections(elf, data);
+ if ( rc )
+ return rc;
+
+ rc = elf_resolve_section_names(elf, data);
+ if ( rc )
+ return rc;
+
+ rc = elf_get_sym(elf, data);
+ if ( rc )
+ return rc;
+
+ return 0;
+}
+
+void xsplice_elf_free(struct xsplice_elf *elf)
+{
+ xfree(elf->sec);
+ elf->sec = NULL;
+ xfree(elf->sym);
+ elf->sym = NULL;
+ elf->nsym = 0;
+ elf->name = NULL;
+ elf->len = 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index b9f08cd..7559877 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -10,6 +10,9 @@ struct xen_sysctl_xsplice_op;
#ifdef CONFIG_XSPLICE
+/* Convenience define for printk. */
+#define XSPLICE "xsplice: "
+
int xsplice_op(struct xen_sysctl_xsplice_op *);
#else
diff --git a/xen/include/xen/xsplice_elf.h b/xen/include/xen/xsplice_elf.h
new file mode 100644
index 0000000..686aaf0
--- /dev/null
+++ b/xen/include/xen/xsplice_elf.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#ifndef __XEN_XSPLICE_ELF_H__
+#define __XEN_XSPLICE_ELF_H__
+
+#include <xen/types.h>
+#include <xen/elfstructs.h>
+
+/* The following describes an Elf file as consumed by xSplice. */
+struct xsplice_elf_sec {
+ const Elf_Shdr *sec; /* Hooked up in
elf_resolve_sections.*/
+ const char *name; /* Human readable name hooked in
+ elf_resolve_section_names. */
+ const void *data; /* Pointer to the section (done by
+ elf_resolve_sections). */
+};
+
+struct xsplice_elf_sym {
+ const Elf_Sym *sym;
+ const char *name;
+};
+
+struct xsplice_elf {
+ const char *name; /* Pointer to payload->name. */
+ size_t len; /* Length of the ELF file. */
+ const Elf_Ehdr *hdr; /* ELF file. */
+ struct xsplice_elf_sec *sec; /* Array of sections, allocated by
us. */
+ struct xsplice_elf_sym *sym; /* Array of symbols , allocated by
us. */
+ unsigned int nsym;
+ const struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka
to sec[x]. */
+ const struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka
to sec[y]. */
+};
+
+const struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf
*elf,
+ const char *name);
+int xsplice_elf_load(struct xsplice_elf *elf, const void *data);
+void xsplice_elf_free(struct xsplice_elf *elf);
+
+#endif /* __XEN_XSPLICE_ELF_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.4.3
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |