Re: [PATCH v9 10/27] xsplice: Add helper elf routines

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > +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@citrix.com>
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@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>

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@lists.xen.org
http://lists.xen.org/xen-devel