| Message ID | 1458849640-22588-10-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > --- /dev/null > +++ b/xen/common/xsplice_elf.c > @@ -0,0 +1,294 @@ > +/* > + * 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> > + > +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf, > + const char *name) > +{ > + unsigned int i; > + > + for ( i = 0; i < elf->hdr->e_shnum; i++ ) Unless you do something unusual (which would be undesirable), useful ELF section number start from 1. > +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data) > +{ > + struct xsplice_elf_sec *sec; > + unsigned int i; > + > + /* xsplice_elf_load sanity checked e_shnum checked. */ redundant "checked" > + sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum); > + if ( !sec ) > + { > + printk(XENLOG_ERR "%s%s: Could not allocate memory for section table!\n", > + XSPLICE, elf->name); > + return -ENOMEM; > + } > + > + elf->sec = sec; > + > + /* N.B. We also will ingest SHN_UNDEF sections. */ Because of? The meaning of the fields in the 0-th section header is different from that of ordinary ones. > + for ( i = 0; i < elf->hdr->e_shnum; i++ ) > + { > + ssize_t delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; Why ssize_t? (This anyway should be a suitable ELF type.) > + > + if ( delta + sizeof(Elf_Shdr) > elf->len ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: Section header [%d] is past end of payload!\n", > + XSPLICE, elf->name, i); XSPLICE is a string literal, so should be prepended to the format string instead of forced through %s. And %u please for unsigned arguments. Also this check doesn't need doing inside the loop - you can simply check once (using e_shnum) that the entire section table is valid. > + return -EINVAL; > + } > + > + sec[i].sec = (Elf_Shdr *)(data + delta); Pointless cast bogusly casting away constness. > + delta = sec[i].sec->sh_offset; > + > + if ( delta > elf->len ) This is relevant only for sections having non-zero size. And then you of course need to take size into account when dong the bounds check. > + { > + dprintk(XENLOG_DEBUG, "%s%s: Section [%d] data is past end of payload!\n", > + XSPLICE, elf->name, i); > + return -EINVAL; > + } > + > + sec[i].data = data + delta; > + /* Name is populated in xsplice_elf_sections_name. */ > + sec[i].name = NULL; > + > + if ( sec[i].sec->sh_type == SHT_SYMTAB ) > + { > + if ( elf->symtab ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: Multiple symbol tables!\n", > + XSPLICE, elf->name); > + return -EINVAL; There's nothing invalid about this, it's simply unsupported by the implementation (read: a better error code please). > + } > + > + 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_DEBUG, "%s%s: Symbol table idx (%d) to strtab past end (%d)\n", > + XSPLICE, elf->name, elf->symtab->sec->sh_link, > + elf->hdr->e_shnum); > + return -EINVAL; > + } > + } > + } > + > + if ( !elf->symtab ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: No symbol table found!\n", > + XSPLICE, elf->name); > + return -EINVAL; > + } > + > + /* There can be multiple SHT_STRTAB so pick the right one. */ > + elf->strtab = &sec[elf->symtab->sec->sh_link]; How about checking this really is a SHT_STRTAB section? > + if ( !elf->symtab->sec->sh_size || !elf->symtab->sec->sh_entsize || > + elf->symtab->sec->sh_entsize != sizeof(Elf_Sym) ) The first sh_entsize check is redundant with the second, and the second is too strict (< suffices). Also shouldn't the string table section also have at least non-zero size? And its first and last bytes be NUL? > +static int elf_resolve_section_names(struct xsplice_elf *elf, const void *data) > +{ > + const char *shstrtab; > + unsigned int i; > + unsigned int offset, delta; > + > + /* > + * The elf->sec[0 -> e_shnum] structures have been verified by > + * elf_resolve_sections. Find file offset for section string table. > + */ > + offset = elf->sec[elf->hdr->e_shstrndx].sec->sh_offset; Truncating the value on 64-bit ELF. > + if ( offset > elf->len ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: shstrtab section offset (%u) past end of payload!\n", > + XSPLICE, elf->name, elf->hdr->e_shstrndx); > + return -EINVAL; > + } > + > + shstrtab = (data + offset); Pointless parentheses. > + /* We could ignore the first as it is reserved.. */ Double full stop. > + for ( i = 0; i < elf->hdr->e_shnum; i++ ) > + { > + delta = elf->sec[i].sec->sh_name; > + > + if ( offset + delta > elf->len ) This is too weak: After having bounds checked the string table section to be inside the image, you now need to bounds check the string offset to be inside the string table. Also it seems (just like above) you no-where check that the referenced section actually is a string table. > +static int elf_get_sym(struct xsplice_elf *elf, const void *data) > +{ > + 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; > + > + ASSERT(offset == strtab_sec->sec->sh_offset); > + > + /* 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 ) > + { > + printk(XENLOG_ERR "%s%s: Could not allocate memory for symbols\n", > + XSPLICE, elf->name); > + return -ENOMEM; > + } > + > + /* So we don't leak memory. */ > + elf->sym = sym; > + for ( i = 0; i < nsym; i++ ) As with sections, the 0th symbol table entry is special too. > + { > + Elf_Sym *s; > + > + if ( i * sizeof(Elf_Sym) > elf->len ) Considering that we know the symbol table section is within bounds, I don't think this check does any good. Plus it ought to be adding 1 to i and take the section file offset into account. > + { > + dprintk(XENLOG_DEBUG, "%s%s: Symbol header [%d] is past end of payload!\n", > + XSPLICE, elf->name, i); > + return -EINVAL; > + } > + > + s = &((Elf_Sym *)symtab_sec->data)[i]; > + > + /* If st->name is STN_UNDEF is zero, the check will always be true. */ Odd double use of "is". > + delta = s->st_name; > + > + /* Offset has been computed earlier. */ > + if ( offset + delta > elf->len ) This should again check against the string table size and again use >= . > + { > + dprintk(XENLOG_DEBUG, "%s%s: Symbol [%u] data is past end of payload!\n", > + XSPLICE, elf->name, i); > + return -EINVAL; > + } > + > + sym[i].sym = s; > + if ( s->st_name == STN_UNDEF ) > + sym[i].name = NULL; I don't think this is a good idea - since the first byte of a string table needs to be NUL, you can as well just point there (without any need for a conditional). > + else > + sym[i].name = data + ( delta + offset ); Stray blanks. > +static int xsplice_header_check(const struct xsplice_elf *elf) > +{ > + if ( sizeof(*elf->hdr) >= elf->len ) Strictly speaking just > . > + { > + dprintk(XENLOG_DEBUG, "%s%s: Section header is bigger than payload!\n", > + XSPLICE, elf->name); > + return -EINVAL; > + } > + > + if ( elf->hdr->e_shstrndx == SHN_UNDEF ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: Section name idx is undefined!?\n", > + XSPLICE, elf->name); > + return -EINVAL; > + } > + > + /* Check that section name index is within the sections. */ > + if ( elf->hdr->e_shstrndx > elf->hdr->e_shnum ) >= > + { > + dprintk(XENLOG_DEBUG, "%s%s: Section name idx (%d) is past end of sections (%d)!\n", > + XSPLICE, elf->name, elf->hdr->e_shstrndx, elf->hdr->e_shnum); > + return -EINVAL; > + } > + > + if ( elf->hdr->e_shnum > 64 ) > + { > + dprintk(XENLOG_DEBUG, "%s%s: Too many (%d) sections!\n", > + XSPLICE, elf->name, elf->hdr->e_shnum); > + return -EINVAL; > + } > + > + return 0; > +} Assuming there are no further checks hidden in other patches, I'm afraid there's quite a bit of stuff missing - there are plenty of other header fields most if not all of which need sanity checking. > +int xsplice_elf_load(struct xsplice_elf *elf, 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; > +} Instead of zeroing these fields, wouldn't it make sense to simply xfree(elf) as the last action here? > --- /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 { > + Elf_Shdr *sec; /* Hooked up in elf_resolve_sections. */ const? > + 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 { > + Elf_Sym *sym; const? > + const char *name; > +}; > + > +struct xsplice_elf { > + const char *name; /* Pointer to payload->name. */ > + ssize_t len; /* Length of the ELF file. */ Why ssize_t? > + 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; > + struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka to sec[x]. */ > + struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka to sec[y]. */ Many times - const? > +}; > + > +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf, > + const char *name); const (return value)? > +int xsplice_elf_load(struct xsplice_elf *elf, void *data); const (second parameter)? Jan
> > +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data) > > +{ .. snip.. > > + /* N.B. We also will ingest SHN_UNDEF sections. */ > > Because of? The meaning of the fields in the 0-th section header is > different from that of ordinary ones. > > > + for ( i = 0; i < elf->hdr->e_shnum; i++ ) The reason for this is not obvious .. In the payload loading patch I iterate over each elf->sec (starting at zero) and immediately dereference the sh_type: if ( (elf->sec[i].sec->sh_flags .. ) As you can imagine if I don't set elf->sec[0].sec this blows up. Hence the odd start at zero. However one can as well just fix the loop in 'move_payload' to start at 1 instead of 0 - which is what I did. > > + { > > + ssize_t delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; > > Why ssize_t? (This anyway should be a suitable ELF type.) > > > + > > + if ( delta + sizeof(Elf_Shdr) > elf->len ) > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: Section header [%d] is past end of payload!\n", > > + XSPLICE, elf->name, i); > > XSPLICE is a string literal, so should be prepended to the format > string instead of forced through %s. And %u please for unsigned > arguments. > > Also this check doesn't need doing inside the loop - you can simply > check once (using e_shnum) that the entire section table is valid. > > > + return -EINVAL; > > + } > > + > > + sec[i].sec = (Elf_Shdr *)(data + delta); > > Pointless cast bogusly casting away constness. > > > + delta = sec[i].sec->sh_offset; > > + > > + if ( delta > elf->len ) > > This is relevant only for sections having non-zero size. And then you of > course need to take size into account when dong the bounds check. > > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: Section [%d] data is past end of payload!\n", > > + XSPLICE, elf->name, i); > > + return -EINVAL; > > + } > > + > > + sec[i].data = data + delta; > > + /* Name is populated in xsplice_elf_sections_name. */ > > + sec[i].name = NULL; > > + > > + if ( sec[i].sec->sh_type == SHT_SYMTAB ) > > + { > > + if ( elf->symtab ) > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: Multiple symbol tables!\n", > > + XSPLICE, elf->name); > > + return -EINVAL; > > There's nothing invalid about this, it's simply unsupported by the > implementation (read: a better error code please). > > > + } > > + > > + 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_DEBUG, "%s%s: Symbol table idx (%d) to strtab past end (%d)\n", > > + XSPLICE, elf->name, elf->symtab->sec->sh_link, > > + elf->hdr->e_shnum); > > + return -EINVAL; > > + } > > + } > > + } > > + > > + if ( !elf->symtab ) > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: No symbol table found!\n", > > + XSPLICE, elf->name); > > + return -EINVAL; > > + } > > + > > + /* There can be multiple SHT_STRTAB so pick the right one. */ > > + elf->strtab = &sec[elf->symtab->sec->sh_link]; > > How about checking this really is a SHT_STRTAB section? > > > + if ( !elf->symtab->sec->sh_size || !elf->symtab->sec->sh_entsize || > > + elf->symtab->sec->sh_entsize != sizeof(Elf_Sym) ) > > The first sh_entsize check is redundant with the second, and the > second is too strict (< suffices). > > Also shouldn't the string table section also have at least non-zero > size? And its first and last bytes be NUL? > > > +static int elf_resolve_section_names(struct xsplice_elf *elf, const void *data) > > +{ > > + const char *shstrtab; > > + unsigned int i; > > + unsigned int offset, delta; > > + > > + /* > > + * The elf->sec[0 -> e_shnum] structures have been verified by > > + * elf_resolve_sections. Find file offset for section string table. > > + */ > > + offset = elf->sec[elf->hdr->e_shstrndx].sec->sh_offset; > > Truncating the value on 64-bit ELF. > > > + if ( offset > elf->len ) > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: shstrtab section offset (%u) past end of payload!\n", > > + XSPLICE, elf->name, elf->hdr->e_shstrndx); > > + return -EINVAL; > > + } > > + > > + shstrtab = (data + offset); > > Pointless parentheses. > > > + /* We could ignore the first as it is reserved.. */ > > Double full stop. > > > + for ( i = 0; i < elf->hdr->e_shnum; i++ ) > > + { > > + delta = elf->sec[i].sec->sh_name; > > + > > + if ( offset + delta > elf->len ) > > This is too weak: After having bounds checked the string table section > to be inside the image, you now need to bounds check the string offset > to be inside the string table. Also it seems (just like above) you > no-where check that the referenced section actually is a string table. > > > +static int elf_get_sym(struct xsplice_elf *elf, const void *data) > > +{ > > + 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; > > + > > + ASSERT(offset == strtab_sec->sec->sh_offset); > > + > > + /* 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 ) > > + { > > + printk(XENLOG_ERR "%s%s: Could not allocate memory for symbols\n", > > + XSPLICE, elf->name); > > + return -ENOMEM; > > + } > > + > > + /* So we don't leak memory. */ > > + elf->sym = sym; > > + for ( i = 0; i < nsym; i++ ) > > As with sections, the 0th symbol table entry is special too. > > > + { > > + Elf_Sym *s; > > + > > + if ( i * sizeof(Elf_Sym) > elf->len ) > > Considering that we know the symbol table section is within bounds, > I don't think this check does any good. Plus it ought to be adding 1 > to i and take the section file offset into account. > > > + { > > + dprintk(XENLOG_DEBUG, "%s%s: Symbol header [%d] is past end of payload!\n", > > + XSPLICE, elf->name, i); > > + return -EINVAL; > > + } > > + > > + s = &((Elf_Sym *)symtab_sec->data)[i]; > > + > > + /* If st->name is STN_UNDEF is zero, the check will always be true. */ > > Odd double use of "is". > > > + delta = s->st_name; > > + > > + /* Offset has been computed earlier. */ > > + if ( offset + delta > elf->len ) > > This should again check against the string table size and again use >= . I reworked this a bit (borrowed your idea of checking the full size of the section before the loop) - which removes the need to check the offset. What I ended up is something much simpler (as I know the offset is OK - I just need to check that the delta is within the section): if ( delta && (delta > strtab_sec->sec->sec_sh_size) ) .. The offset gets (in the new patchset) checked in elf_resolve_section. Albeit I am not sure about the >= instead of >, .. I need to think of that. .. snip.. > > +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; > > +} > > Instead of zeroing these fields, wouldn't it make sense to simply > xfree(elf) as the last action here? The struct xsplice_elf is allocated on the stack (in the next patch). > > --- /dev/null > > +++ b/xen/include/xen/xsplice_elf.h .. snip.. > > +struct xsplice_elf_sym { > > + Elf_Sym *sym; > > const? .. this is much harder. I end up computing the values for these symbols and have to write to this this structure a couple of times (at worst). > > > + const char *name; > > +}; > > + > > +struct xsplice_elf { > > + const char *name; /* Pointer to payload->name. */ > > + ssize_t len; /* Length of the ELF file. */ > > Why ssize_t? Made it 'size_t' > > > + 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; > > + struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka to sec[x]. */ > > + struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka to sec[y]. */ > > Many times - const? I have made the symtab and strtab const, but the 'sec' and 'sym' I can't easily. There are many instances where I poke in the section (like for ELF relocations) and have to modify this. I can do some casting but it gets a bit .. messy.
>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 04/06/16 3:40 AM >>> >> > +struct xsplice_elf_sym { >> > + Elf_Sym *sym; >> >> const? > >.. this is much harder. I end up computing the values for >these symbols and have to write to this this structure a couple of times >(at worst). So I've intentionally added question marks to many of these comments, as there certainly may be reasons to better not make some of the items const. It just generally seemed in many cases that what gets pointed to has no reason to get altered after initial setup. >> > + 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; >> > + struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka to sec[x]. */ >> > + struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka to sec[y]. */ >> >> Many times - const? > >I have made the symtab and strtab const, but the 'sec' and 'sym' >I can't easily. There are many instances where I poke in the >section (like for ELF relocations) and have to modify this. When processing relocations shouldn't need to modify symbols, and you also shouldn't need to modify section metadata - only section contents itself should be altered. >I can do some casting but it gets a bit .. messy. One thing should be very clear: No casts should be added which cast away constness. Jan
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..f22cede --- /dev/null +++ b/xen/common/xsplice_elf.c @@ -0,0 +1,294 @@ +/* + * 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> + +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf, + const char *name) +{ + unsigned int i; + + for ( i = 0; i < elf->hdr->e_shnum; i++ ) + { + if ( !strcmp(name, elf->sec[i].name) ) + return &elf->sec[i]; + } + + return NULL; +} + +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data) +{ + struct xsplice_elf_sec *sec; + unsigned int i; + + /* xsplice_elf_load sanity checked e_shnum checked. */ + sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum); + if ( !sec ) + { + printk(XENLOG_ERR "%s%s: Could not allocate memory for section table!\n", + XSPLICE, elf->name); + return -ENOMEM; + } + + elf->sec = sec; + + /* N.B. We also will ingest SHN_UNDEF sections. */ + for ( i = 0; i < elf->hdr->e_shnum; i++ ) + { + ssize_t delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize; + + if ( delta + sizeof(Elf_Shdr) > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: Section header [%d] is past end of payload!\n", + XSPLICE, elf->name, i); + return -EINVAL; + } + + sec[i].sec = (Elf_Shdr *)(data + delta); + delta = sec[i].sec->sh_offset; + + if ( delta > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: Section [%d] data is past end of payload!\n", + XSPLICE, elf->name, i); + return -EINVAL; + } + + sec[i].data = data + delta; + /* Name is populated in xsplice_elf_sections_name. */ + sec[i].name = NULL; + + if ( sec[i].sec->sh_type == SHT_SYMTAB ) + { + if ( elf->symtab ) + { + dprintk(XENLOG_DEBUG, "%s%s: Multiple symbol tables!\n", + XSPLICE, elf->name); + return -EINVAL; + } + + 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_DEBUG, "%s%s: Symbol table idx (%d) to strtab past end (%d)\n", + XSPLICE, elf->name, elf->symtab->sec->sh_link, + elf->hdr->e_shnum); + return -EINVAL; + } + } + } + + if ( !elf->symtab ) + { + dprintk(XENLOG_DEBUG, "%s%s: No symbol table found!\n", + XSPLICE, elf->name); + return -EINVAL; + } + + /* There can be multiple SHT_STRTAB so pick the right one. */ + elf->strtab = &sec[elf->symtab->sec->sh_link]; + + if ( !elf->symtab->sec->sh_size || !elf->symtab->sec->sh_entsize || + elf->symtab->sec->sh_entsize != sizeof(Elf_Sym) ) + { + dprintk(XENLOG_DEBUG, "%s%s: Symbol table header is corrupted!\n", + XSPLICE, elf->name); + return -EINVAL; + } + + return 0; +} + +static int elf_resolve_section_names(struct xsplice_elf *elf, const void *data) +{ + const char *shstrtab; + unsigned int i; + unsigned int offset, delta; + + /* + * The elf->sec[0 -> e_shnum] structures have been verified by + * elf_resolve_sections. Find file offset for section string table. + */ + offset = elf->sec[elf->hdr->e_shstrndx].sec->sh_offset; + + if ( offset > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: shstrtab section offset (%u) past end of payload!\n", + XSPLICE, elf->name, elf->hdr->e_shstrndx); + return -EINVAL; + } + + shstrtab = (data + offset); + + /* We could ignore the first as it is reserved.. */ + for ( i = 0; i < elf->hdr->e_shnum; i++ ) + { + delta = elf->sec[i].sec->sh_name; + + if ( offset + delta > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: shstrtab [%d] data is past end of payload!\n", + XSPLICE, 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) +{ + 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; + + ASSERT(offset == strtab_sec->sec->sh_offset); + + /* 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 ) + { + printk(XENLOG_ERR "%s%s: Could not allocate memory for symbols\n", + XSPLICE, elf->name); + return -ENOMEM; + } + + /* So we don't leak memory. */ + elf->sym = sym; + for ( i = 0; i < nsym; i++ ) + { + Elf_Sym *s; + + if ( i * sizeof(Elf_Sym) > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: Symbol header [%d] is past end of payload!\n", + XSPLICE, elf->name, i); + return -EINVAL; + } + + s = &((Elf_Sym *)symtab_sec->data)[i]; + + /* If st->name is STN_UNDEF is zero, the check will always be true. */ + delta = s->st_name; + + /* Offset has been computed earlier. */ + if ( offset + delta > elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: Symbol [%u] data is past end of payload!\n", + XSPLICE, elf->name, i); + return -EINVAL; + } + + sym[i].sym = s; + if ( s->st_name == STN_UNDEF ) + sym[i].name = NULL; + else + sym[i].name = data + ( delta + offset ); + } + elf->nsym = nsym; + + return 0; +} + +static int xsplice_header_check(const struct xsplice_elf *elf) +{ + if ( sizeof(*elf->hdr) >= elf->len ) + { + dprintk(XENLOG_DEBUG, "%s%s: Section header is bigger than payload!\n", + XSPLICE, elf->name); + return -EINVAL; + } + + if ( elf->hdr->e_shstrndx == SHN_UNDEF ) + { + dprintk(XENLOG_DEBUG, "%s%s: Section name idx is undefined!?\n", + XSPLICE, elf->name); + return -EINVAL; + } + + /* Check that section name index is within the sections. */ + if ( elf->hdr->e_shstrndx > elf->hdr->e_shnum ) + { + dprintk(XENLOG_DEBUG, "%s%s: Section name idx (%d) is past end of sections (%d)!\n", + XSPLICE, elf->name, elf->hdr->e_shstrndx, elf->hdr->e_shnum); + return -EINVAL; + } + + if ( elf->hdr->e_shnum > 64 ) + { + dprintk(XENLOG_DEBUG, "%s%s: Too many (%d) sections!\n", + XSPLICE, elf->name, elf->hdr->e_shnum); + return -EINVAL; + } + + return 0; +} + +int xsplice_elf_load(struct xsplice_elf *elf, 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 5c84851..00482d0 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..e2dea18 --- /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 { + 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 { + Elf_Sym *sym; + const char *name; +}; + +struct xsplice_elf { + const char *name; /* Pointer to payload->name. */ + ssize_t len; /* Length of the ELF file. */ + 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; + struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka to sec[x]. */ + struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka to sec[y]. */ +}; + +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, 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: + */