| Message ID | 1455644269-40358-4-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.02.16 at 18:37, <roger.pau@citrix.com> wrote: > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) > sz = sizeof(uint32_t); > > /* Space for the elf and elf section headers */ > - sz += (elf_uval(elf, elf->ehdr, e_ehsize) + > - elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize)); > + sz += elf_uval(elf, elf->ehdr, e_ehsize) + > + 3 * elf_uval(elf, elf->ehdr, e_shentsize); I think a literal 3 like this either needs a #define (at once serving as documentation) or a comment. Perhaps rather the former, considering that the (same?) 3 appears again further down. Plus - what guarantees there are 3 section headers in the image? > sz = elf_round_up(elf, sz); > > - /* Space for the symbol and string tables. */ > + /* Space for the symbol and string table. */ > 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)); > + > + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) > + continue; > + > + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); > + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); > + > + 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_STRTAB ) > + /* Invalid symtab -> strtab link */ > + break; This is not sufficient - what if sh_link is out of bounds, but the bogusly accessed field happens to hold SHT_STRTAB? (Oddly enough you have at least an SHN_UNDEF check in the second loop below.) Also even if (I assume) elf_load_image() would refuse to read outside the bounds of the image, wouldn't you better check here that both symtab and strtab are within image bounds? The more that you ignore errors from elf_load_image() (and elf_mem{cpy,set}_safe() don't even return any)? > @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, > uint64_t pstart) > > static void elf_load_bsdsyms(struct elf_binary *elf) > { > - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; > - unsigned long sz; > - elf_ptrval maxva; > - elf_ptrval symbase; > - elf_ptrval symtab_addr; > - ELF_HANDLE_DECL(elf_shdr) shdr; > - unsigned i, type; > + /* > + * Header that is placed at the end of the kernel and allows > + * the OS to find where the symtab and strtab have been loaded. > + * It mimics a valid ELF file header, although it only contains > + * a symtab and a strtab section. > + * > + * NB: according to the ELF spec there's only ONE symtab per ELF > + * file, and accordingly we will only load the corresponding > + * strtab, so we only need three section headers in our fake ELF > + * header (first section header is always a dummy). > + */ > + struct { > + uint32_t size; > + struct { > + elf_ehdr header; > + elf_shdr section[3]; > + } __attribute__((packed)) elf_header; > + } __attribute__((packed)) header; If this is placed at the end, how can the OS reasonably use it without knowing that there are exactly 3 section header entries? I.e. how would it find the base of this structure? > + ELF_HANDLE_DECL(elf_ehdr) header_handle; > + unsigned long shdr_size; > + ELF_HANDLE_DECL(elf_shdr) section_handle; > + ELF_HANDLE_DECL(elf_shdr) image_handle; > + unsigned int i, link; > + elf_ptrval header_base; > + elf_ptrval elf_header_base; > + elf_ptrval symtab_base; > + elf_ptrval strtab_base; > > if ( !elf->bsd_symtab_pstart ) > return; > > -#define elf_hdr_elm(_elf, _hdr, _elm, _val) \ > -do { \ > - if ( elf_64bit(_elf) ) \ > - elf_store_field(_elf, _hdr, e64._elm, _val); \ > - else \ > - elf_store_field(_elf, _hdr, e32._elm, _val); \ > +#define elf_store_field_bitness(_elf, _hdr, _elm, _val) \ > +do { \ > + if ( elf_64bit(_elf) ) \ > + elf_store_field(_elf, _hdr, e64._elm, _val); \ > + else \ > + 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_MAKE_HANDLE(elf_ehdr, symtab_addr); > - sz = elf_uval(elf, elf->ehdr, e_ehsize); > - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); > - maxva += sz; /* no round up */ > +#define SYMTAB_INDEX 1 > +#define STRTAB_INDEX 2 > > - elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); > - elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize)); > - elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0); > - elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); > + /* Allow elf_memcpy_safe to write to symbol_header. */ > + elf->caller_xdest_base = &header; > + elf->caller_xdest_size = sizeof(header); > > - /* Copy Elf section headers. */ > - shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); > - sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); > - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), > - ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), > - sz); > - maxva = elf_round_up(elf, (unsigned long)maxva + sz); > + /* > + * Calculate the position of the various elements in GUEST MEMORY SPACE. > + * This addresses MUST only be used with elf_load_image. > + * > + * NB: strtab_base cannot be calculated at this point because we don't > + * know the size of the symtab yet, and the strtab will be placed after it. > + */ > + header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart); > + elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + > + sizeof(uint32_t); > + symtab_base = elf_round_up(elf, header_base + sizeof(header)); > + > + /* Fill the ELF header, copied from the original ELF header. */ > + header_handle = ELF_MAKE_HANDLE(elf_ehdr, > + ELF_REALPTR2PTRVAL(&header.elf_header.header)); > + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle), > + ELF_HANDLE_PTRVAL(elf->ehdr), > + elf_uval(elf, elf->ehdr, e_ehsize)); > + > + /* Set the offset to the shdr array. */ > + elf_store_field_bitness(elf, header_handle, e_shoff, > + offsetof(typeof(header.elf_header), section)); > + > + /* Set the right number of section headers. */ > + elf_store_field_bitness(elf, header_handle, e_shnum, 3); > + > + /* Clear a couple of fields we don't use. */ > + elf_store_field_bitness(elf, header_handle, e_phoff, 0); > + elf_store_field_bitness(elf, header_handle, e_phentsize, 0); > + elf_store_field_bitness(elf, header_handle, e_phnum, 0); Perhaps better just give the structure above an initializer? > + /* Zero the dummy section. */ Dummy? And anyway I think you mean "section header" here. > + section_handle = ELF_MAKE_HANDLE(elf_shdr, > + ELF_REALPTR2PTRVAL(&header.elf_header.section[SHN_UNDEF])); > + shdr_size = elf_uval(elf, elf->ehdr, e_shentsize); > + elf_memset_safe(elf, ELF_HANDLE_PTRVAL(section_handle), 0, shdr_size); > > + /* > + * Find the actual symtab and strtab in the ELF. > + * > + * The symtab section header is going to reside in section[SYMTAB_INDEX], > + * while the corresponding strtab is going to be placed in > + * section[STRTAB_INDEX]. sh_offset is mangled so it points to the offset > + * where the sections are actually loaded (relative to the ELF header > + * location). > + */ > + section_handle = ELF_MAKE_HANDLE(elf_shdr, > + ELF_REALPTR2PTRVAL(&header.elf_header.section[SYMTAB_INDEX])); > 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) ) > + image_handle = elf_shdr_by_index(elf, i); > + if ( elf_uval(elf, image_handle, sh_type) != SHT_SYMTAB ) > + continue; > + > + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), > + ELF_HANDLE_PTRVAL(image_handle), > + shdr_size); > + > + link = elf_uval(elf, section_handle, sh_link); > + if ( link == SHN_UNDEF ) > { > - 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); > - 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_round_up(elf, (unsigned long)maxva + sz); > + elf_mark_broken(elf, "bad link in symtab"); > + break; > } > - 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 */ > + > + /* Load symtab into guest memory. */ > + elf_load_image(elf, symtab_base, elf_section_start(elf, section_handle), > + elf_uval(elf, section_handle, sh_size), > + elf_uval(elf, section_handle, sh_size)); > + elf_store_field_bitness(elf, section_handle, sh_offset, > + symtab_base - elf_header_base); > + elf_store_field_bitness(elf, section_handle, sh_link, > + STRTAB_INDEX); > + > + /* Calculate the guest address where strtab is loaded. */ > + strtab_base = elf_round_up(elf, symtab_base + > + elf_uval(elf, section_handle, sh_size)); > + > + /* Load strtab section header. */ > + section_handle = ELF_MAKE_HANDLE(elf_shdr, > + ELF_REALPTR2PTRVAL(&header.elf_header.section[STRTAB_INDEX])); > + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), > + ELF_HANDLE_PTRVAL(elf_shdr_by_index(elf, link)), > + shdr_size); > + > + if ( elf_uval(elf, section_handle, sh_type) != SHT_STRTAB ) > { > - elf_mark_broken(elf, "bad section header length"); > + elf_mark_broken(elf, "strtab not found"); > break; > } > - if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */ > - break; > - shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p); > + > + /* Load strtab into guest memory. */ > + elf_load_image(elf, strtab_base, elf_section_start(elf, section_handle), > + elf_uval(elf, section_handle, sh_size), > + elf_uval(elf, section_handle, sh_size)); > + elf_store_field_bitness(elf, section_handle, sh_offset, > + strtab_base - elf_header_base); > + > + /* Store the whole size (including headers and loaded sections). */ > + header.size = strtab_base + elf_uval(elf, section_handle, sh_size) > - > + elf_header_base; > + break; > } So you're properly breaking out of the loop here - why don't you also do this in the parsing one? Jan
El 26/2/16 a les 14:15, Jan Beulich ha escrit: >>>> On 16.02.16 at 18:37, <roger.pau@citrix.com> wrote: >> --- a/xen/common/libelf/libelf-loader.c >> +++ b/xen/common/libelf/libelf-loader.c >> @@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) >> sz = sizeof(uint32_t); >> >> /* Space for the elf and elf section headers */ >> - sz += (elf_uval(elf, elf->ehdr, e_ehsize) + >> - elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize)); >> + sz += elf_uval(elf, elf->ehdr, e_ehsize) + >> + 3 * elf_uval(elf, elf->ehdr, e_shentsize); > > I think a literal 3 like this either needs a #define (at once serving > as documentation) or a comment. Perhaps rather the former, > considering that the (same?) 3 appears again further down. Plus - > what guarantees there are 3 section headers in the image? This is not related to the image itself, but to the metadata that libelf places at the end of the loaded kernel in order to stash the symtab and strtab for the guest to use. The layout is as follows (I should add this to the patch itself as a comment, since I guess this is still quite confusing): +------------------------+ | | | KERNEL | | | +------------------------+ pend | ALIGNMENT | +------------------------+ bsd_symtab_pstart | | | size | bsd_symtab_pend - bsd_symtab_pstart | | +------------------------+ | | | ELF header | | | +------------------------+ | | | ELF section header 0 | Dummy section header | | +------------------------+ | | | ELF section header 1 | SYMTAB section header | | +------------------------+ | | | ELF section header 2 | STRTAB section header | | +------------------------+ | | | SYMTAB | | | +------------------------+ | | | STRTAB | | | +------------------------+ bsd_symtab_pend There are always only tree sections because that's all we need, section 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is used to describe the STRTAB. According to the ELF spec there can only be one SYMTAB, so that's all we need. > >> sz = elf_round_up(elf, sz); >> >> - /* Space for the symbol and string tables. */ >> + /* Space for the symbol and string table. */ >> 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)); >> + >> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >> + continue; >> + >> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >> + >> + 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_STRTAB ) >> + /* Invalid symtab -> strtab link */ >> + break; > > This is not sufficient - what if sh_link is out of bounds, but the > bogusly accessed field happens to hold SHT_STRTAB? (Oddly > enough you have at least an SHN_UNDEF check in the second > loop below.) The out-of-bounds access would be detected by elf_access_ok (if it's out of the scope of the ELF file, which is all we care about). In fact the elf_access_ok above could be removed because elf_uval already performs out-of-bounds checks. The result is definitely no worse that what we are doing ATM. > > Also even if (I assume) elf_load_image() would refuse to read > outside the bounds of the image, wouldn't you better check here > that both symtab and strtab are within image bounds? The more > that you ignore errors from elf_load_image() (and > elf_mem{cpy,set}_safe() don't even return any)? Yes, I will add error checks to elf_load_image calls below. >> @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, >> uint64_t pstart) >> >> static void elf_load_bsdsyms(struct elf_binary *elf) >> { >> - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; >> - unsigned long sz; >> - elf_ptrval maxva; >> - elf_ptrval symbase; >> - elf_ptrval symtab_addr; >> - ELF_HANDLE_DECL(elf_shdr) shdr; >> - unsigned i, type; >> + /* >> + * Header that is placed at the end of the kernel and allows >> + * the OS to find where the symtab and strtab have been loaded. >> + * It mimics a valid ELF file header, although it only contains >> + * a symtab and a strtab section. >> + * >> + * NB: according to the ELF spec there's only ONE symtab per ELF >> + * file, and accordingly we will only load the corresponding >> + * strtab, so we only need three section headers in our fake ELF >> + * header (first section header is always a dummy). >> + */ >> + struct { >> + uint32_t size; >> + struct { >> + elf_ehdr header; >> + elf_shdr section[3]; >> + } __attribute__((packed)) elf_header; >> + } __attribute__((packed)) header; > > If this is placed at the end, how can the OS reasonably use it > without knowing that there are exactly 3 section header > entries? I.e. how would it find the base of this structure? See the commend below, the base is found at the end of the kernel, and then the ELF header contains the right pointers to the sections headers (by appropriately setting e_shoff). IMHO, this is an overly complex way to pass a SYMTAB and STRTAB, but it's baked in the ABI, so I don't see us changing it. >> + ELF_HANDLE_DECL(elf_ehdr) header_handle; >> + unsigned long shdr_size; >> + ELF_HANDLE_DECL(elf_shdr) section_handle; >> + ELF_HANDLE_DECL(elf_shdr) image_handle; >> + unsigned int i, link; >> + elf_ptrval header_base; >> + elf_ptrval elf_header_base; >> + elf_ptrval symtab_base; >> + elf_ptrval strtab_base; >> >> if ( !elf->bsd_symtab_pstart ) >> return; >> >> -#define elf_hdr_elm(_elf, _hdr, _elm, _val) \ >> -do { \ >> - if ( elf_64bit(_elf) ) \ >> - elf_store_field(_elf, _hdr, e64._elm, _val); \ >> - else \ >> - elf_store_field(_elf, _hdr, e32._elm, _val); \ >> +#define elf_store_field_bitness(_elf, _hdr, _elm, _val) \ >> +do { \ >> + if ( elf_64bit(_elf) ) \ >> + elf_store_field(_elf, _hdr, e64._elm, _val); \ >> + else \ >> + 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_MAKE_HANDLE(elf_ehdr, symtab_addr); >> - sz = elf_uval(elf, elf->ehdr, e_ehsize); >> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); >> - maxva += sz; /* no round up */ >> +#define SYMTAB_INDEX 1 >> +#define STRTAB_INDEX 2 >> >> - elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); >> - elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize)); >> - elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0); >> - elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); >> + /* Allow elf_memcpy_safe to write to symbol_header. */ >> + elf->caller_xdest_base = &header; >> + elf->caller_xdest_size = sizeof(header); >> >> - /* Copy Elf section headers. */ >> - shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); >> - sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); >> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), >> - ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), >> - sz); >> - maxva = elf_round_up(elf, (unsigned long)maxva + sz); >> + /* >> + * Calculate the position of the various elements in GUEST MEMORY SPACE. >> + * This addresses MUST only be used with elf_load_image. >> + * >> + * NB: strtab_base cannot be calculated at this point because we don't >> + * know the size of the symtab yet, and the strtab will be placed after it. >> + */ >> + header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart); >> + elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + >> + sizeof(uint32_t); >> + symtab_base = elf_round_up(elf, header_base + sizeof(header)); >> + >> + /* Fill the ELF header, copied from the original ELF header. */ >> + header_handle = ELF_MAKE_HANDLE(elf_ehdr, >> + ELF_REALPTR2PTRVAL(&header.elf_header.header)); >> + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle), >> + ELF_HANDLE_PTRVAL(elf->ehdr), >> + elf_uval(elf, elf->ehdr, e_ehsize)); >> + >> + /* Set the offset to the shdr array. */ >> + elf_store_field_bitness(elf, header_handle, e_shoff, >> + offsetof(typeof(header.elf_header), section)); >> + >> + /* Set the right number of section headers. */ >> + elf_store_field_bitness(elf, header_handle, e_shnum, 3); >> + >> + /* Clear a couple of fields we don't use. */ >> + elf_store_field_bitness(elf, header_handle, e_phoff, 0); >> + elf_store_field_bitness(elf, header_handle, e_phentsize, 0); >> + elf_store_field_bitness(elf, header_handle, e_phnum, 0); > > Perhaps better just give the structure above an initializer? Not really, the problem is that we copy the original header from the ELF binary, and then we mangle it. This is just a fixup to make it clear that no program headers are present (we just use section headers in order to describe the SYMTAB and STRTAB positions). >> + /* Zero the dummy section. */ > > Dummy? And anyway I think you mean "section header" here. Yes, the right comment would be: zero the dummy section header. > >> + section_handle = ELF_MAKE_HANDLE(elf_shdr, >> + ELF_REALPTR2PTRVAL(&header.elf_header.section[SHN_UNDEF])); >> + shdr_size = elf_uval(elf, elf->ehdr, e_shentsize); >> + elf_memset_safe(elf, ELF_HANDLE_PTRVAL(section_handle), 0, shdr_size); >> >> + /* >> + * Find the actual symtab and strtab in the ELF. >> + * >> + * The symtab section header is going to reside in section[SYMTAB_INDEX], >> + * while the corresponding strtab is going to be placed in >> + * section[STRTAB_INDEX]. sh_offset is mangled so it points to the offset >> + * where the sections are actually loaded (relative to the ELF header >> + * location). >> + */ >> + section_handle = ELF_MAKE_HANDLE(elf_shdr, >> + ELF_REALPTR2PTRVAL(&header.elf_header.section[SYMTAB_INDEX])); >> 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) ) >> + image_handle = elf_shdr_by_index(elf, i); >> + if ( elf_uval(elf, image_handle, sh_type) != SHT_SYMTAB ) >> + continue; >> + >> + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), >> + ELF_HANDLE_PTRVAL(image_handle), >> + shdr_size); >> + >> + link = elf_uval(elf, section_handle, sh_link); >> + if ( link == SHN_UNDEF ) >> { >> - 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); >> - 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_round_up(elf, (unsigned long)maxva + sz); >> + elf_mark_broken(elf, "bad link in symtab"); >> + break; >> } >> - 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 */ >> + >> + /* Load symtab into guest memory. */ >> + elf_load_image(elf, symtab_base, elf_section_start(elf, section_handle), >> + elf_uval(elf, section_handle, sh_size), >> + elf_uval(elf, section_handle, sh_size)); >> + elf_store_field_bitness(elf, section_handle, sh_offset, >> + symtab_base - elf_header_base); >> + elf_store_field_bitness(elf, section_handle, sh_link, >> + STRTAB_INDEX); >> + >> + /* Calculate the guest address where strtab is loaded. */ >> + strtab_base = elf_round_up(elf, symtab_base + >> + elf_uval(elf, section_handle, sh_size)); >> + >> + /* Load strtab section header. */ >> + section_handle = ELF_MAKE_HANDLE(elf_shdr, >> + ELF_REALPTR2PTRVAL(&header.elf_header.section[STRTAB_INDEX])); >> + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), >> + ELF_HANDLE_PTRVAL(elf_shdr_by_index(elf, link)), >> + shdr_size); >> + >> + if ( elf_uval(elf, section_handle, sh_type) != SHT_STRTAB ) >> { >> - elf_mark_broken(elf, "bad section header length"); >> + elf_mark_broken(elf, "strtab not found"); >> break; >> } >> - if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */ >> - break; >> - shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p); >> + >> + /* Load strtab into guest memory. */ >> + elf_load_image(elf, strtab_base, elf_section_start(elf, section_handle), >> + elf_uval(elf, section_handle, sh_size), >> + elf_uval(elf, section_handle, sh_size)); >> + elf_store_field_bitness(elf, section_handle, sh_offset, >> + strtab_base - elf_header_base); >> + >> + /* Store the whole size (including headers and loaded sections). */ >> + header.size = strtab_base + elf_uval(elf, section_handle, sh_size) >> - >> + elf_header_base; >> + break; >> } > > So you're properly breaking out of the loop here - why don't you > also do this in the parsing one? Oh, my bad, this is a bug in the parsing code, will fix it, thanks. FWIW, it works because proper ELF binaries only have one SYMTAB, but we shouldn't rely on this. Roger.
>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote: > El 26/2/16 a les 14:15, Jan Beulich ha escrit: >>>>> On 16.02.16 at 18:37, <roger.pau@citrix.com> wrote: >>> --- a/xen/common/libelf/libelf-loader.c >>> +++ b/xen/common/libelf/libelf-loader.c >>> @@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) >>> sz = sizeof(uint32_t); >>> >>> /* Space for the elf and elf section headers */ >>> - sz += (elf_uval(elf, elf->ehdr, e_ehsize) + >>> - elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize)); >>> + sz += elf_uval(elf, elf->ehdr, e_ehsize) + >>> + 3 * elf_uval(elf, elf->ehdr, e_shentsize); >> >> I think a literal 3 like this either needs a #define (at once serving >> as documentation) or a comment. Perhaps rather the former, >> considering that the (same?) 3 appears again further down. Plus - >> what guarantees there are 3 section headers in the image? > > This is not related to the image itself, but to the metadata that libelf > places at the end of the loaded kernel in order to stash the symtab and > strtab for the guest to use. I understand this, yet imo the literal 3 still should be replaced. > The layout is as follows (I should add this to the patch itself as a > comment, since I guess this is still quite confusing): > > +------------------------+ > | | > | KERNEL | > | | > +------------------------+ pend > | ALIGNMENT | > +------------------------+ bsd_symtab_pstart > | | > | size | bsd_symtab_pend - bsd_symtab_pstart > | | > +------------------------+ > | | > | ELF header | > | | > +------------------------+ > | | > | ELF section header 0 | Dummy section header > | | > +------------------------+ > | | > | ELF section header 1 | SYMTAB section header > | | > +------------------------+ > | | > | ELF section header 2 | STRTAB section header > | | > +------------------------+ > | | > | SYMTAB | > | | > +------------------------+ > | | > | STRTAB | > | | > +------------------------+ bsd_symtab_pend > > There are always only tree sections because that's all we need, section > 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is > used to describe the STRTAB. All fine, but this still doesn't clarify how the kernel learns where bsd_symtab_pstart is. > According to the ELF spec there can only be > one SYMTAB, so that's all we need. Did you perhaps overlook the spec saying "... but this restriction may be relaxed in the future", plus the fact that we're talking about an executable file here (which commonly have two symbol tables - .dynsym and .symtab), not an object one? (This isn't to say that you need to make the code handle multiple ones, if you know that *BSD will only ever have one.) >>> sz = elf_round_up(elf, sz); >>> >>> - /* Space for the symbol and string tables. */ >>> + /* Space for the symbol and string table. */ >>> 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)); >>> + >>> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >>> + continue; >>> + >>> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >>> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >>> + >>> + 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_STRTAB ) >>> + /* Invalid symtab -> strtab link */ >>> + break; >> >> This is not sufficient - what if sh_link is out of bounds, but the >> bogusly accessed field happens to hold SHT_STRTAB? (Oddly >> enough you have at least an SHN_UNDEF check in the second >> loop below.) > > The out-of-bounds access would be detected by elf_access_ok (if it's out > of the scope of the ELF file, which is all we care about). In fact the > elf_access_ok above could be removed because elf_uval already performs > out-of-bounds checks. The result is definitely no worse that what we are > doing ATM. No, the out of bounds check should be more strict than just considering the whole image: The image is broken if the link points to a non-existing section. >>> @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, >>> uint64_t pstart) >>> >>> static void elf_load_bsdsyms(struct elf_binary *elf) >>> { >>> - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; >>> - unsigned long sz; >>> - elf_ptrval maxva; >>> - elf_ptrval symbase; >>> - elf_ptrval symtab_addr; >>> - ELF_HANDLE_DECL(elf_shdr) shdr; >>> - unsigned i, type; >>> + /* >>> + * Header that is placed at the end of the kernel and allows >>> + * the OS to find where the symtab and strtab have been loaded. >>> + * It mimics a valid ELF file header, although it only contains >>> + * a symtab and a strtab section. >>> + * >>> + * NB: according to the ELF spec there's only ONE symtab per ELF >>> + * file, and accordingly we will only load the corresponding >>> + * strtab, so we only need three section headers in our fake ELF >>> + * header (first section header is always a dummy). >>> + */ >>> + struct { >>> + uint32_t size; >>> + struct { >>> + elf_ehdr header; >>> + elf_shdr section[3]; >>> + } __attribute__((packed)) elf_header; >>> + } __attribute__((packed)) header; >> >> If this is placed at the end, how can the OS reasonably use it >> without knowing that there are exactly 3 section header >> entries? I.e. how would it find the base of this structure? > > See the commend below, the base is found at the end of the kernel, and > then the ELF header contains the right pointers to the sections headers > (by appropriately setting e_shoff). Thing is - I can't see which "comment below" you try to refer me to. >>> + ELF_HANDLE_DECL(elf_ehdr) header_handle; >>> + unsigned long shdr_size; >>> + ELF_HANDLE_DECL(elf_shdr) section_handle; >>> + ELF_HANDLE_DECL(elf_shdr) image_handle; >>> + unsigned int i, link; >>> + elf_ptrval header_base; >>> + elf_ptrval elf_header_base; >>> + elf_ptrval symtab_base; >>> + elf_ptrval strtab_base; >>> >>> if ( !elf->bsd_symtab_pstart ) >>> return; >>> >>> -#define elf_hdr_elm(_elf, _hdr, _elm, _val) \ >>> -do { \ >>> - if ( elf_64bit(_elf) ) \ >>> - elf_store_field(_elf, _hdr, e64._elm, _val); \ >>> - else \ >>> - elf_store_field(_elf, _hdr, e32._elm, _val); \ >>> +#define elf_store_field_bitness(_elf, _hdr, _elm, _val) \ >>> +do { \ >>> + if ( elf_64bit(_elf) ) \ >>> + elf_store_field(_elf, _hdr, e64._elm, _val); \ >>> + else \ >>> + 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_MAKE_HANDLE(elf_ehdr, symtab_addr); >>> - sz = elf_uval(elf, elf->ehdr, e_ehsize); >>> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); >>> - maxva += sz; /* no round up */ >>> +#define SYMTAB_INDEX 1 >>> +#define STRTAB_INDEX 2 >>> >>> - elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); >>> - elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize)); >>> - elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0); >>> - elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); >>> + /* Allow elf_memcpy_safe to write to symbol_header. */ >>> + elf->caller_xdest_base = &header; >>> + elf->caller_xdest_size = sizeof(header); >>> >>> - /* Copy Elf section headers. */ >>> - shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); >>> - sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); >>> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), >>> - ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), >>> - sz); >>> - maxva = elf_round_up(elf, (unsigned long)maxva + sz); >>> + /* >>> + * Calculate the position of the various elements in GUEST MEMORY SPACE. >>> + * This addresses MUST only be used with elf_load_image. >>> + * >>> + * NB: strtab_base cannot be calculated at this point because we don't >>> + * know the size of the symtab yet, and the strtab will be placed after it. >>> + */ >>> + header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart); >>> + elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + >>> + sizeof(uint32_t); >>> + symtab_base = elf_round_up(elf, header_base + sizeof(header)); >>> + >>> + /* Fill the ELF header, copied from the original ELF header. */ >>> + header_handle = ELF_MAKE_HANDLE(elf_ehdr, >>> + ELF_REALPTR2PTRVAL(&header.elf_header.header)); >>> + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle), >>> + ELF_HANDLE_PTRVAL(elf->ehdr), >>> + elf_uval(elf, elf->ehdr, e_ehsize)); >>> + >>> + /* Set the offset to the shdr array. */ >>> + elf_store_field_bitness(elf, header_handle, e_shoff, >>> + offsetof(typeof(header.elf_header), section)); >>> + >>> + /* Set the right number of section headers. */ >>> + elf_store_field_bitness(elf, header_handle, e_shnum, 3); >>> + >>> + /* Clear a couple of fields we don't use. */ >>> + elf_store_field_bitness(elf, header_handle, e_phoff, 0); >>> + elf_store_field_bitness(elf, header_handle, e_phentsize, 0); >>> + elf_store_field_bitness(elf, header_handle, e_phnum, 0); >> >> Perhaps better just give the structure above an initializer? > > Not really, the problem is that we copy the original header from the ELF > binary, and then we mangle it. This is just a fixup to make it clear > that no program headers are present (we just use section headers in > order to describe the SYMTAB and STRTAB positions). Ah, I see now. >>> + /* Zero the dummy section. */ >> >> Dummy? And anyway I think you mean "section header" here. > > Yes, the right comment would be: zero the dummy section header. But then still - why "dummy"? The 0-th section header has a purpose beyond being a filler, even if that purpose doesn't matter for the intentions you have here. Jan
El 29/2/16 a les 10:31, Jan Beulich ha escrit: >>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote: >> El 26/2/16 a les 14:15, Jan Beulich ha escrit: >>>>>> On 16.02.16 at 18:37, <roger.pau@citrix.com> wrote: >>>> --- a/xen/common/libelf/libelf-loader.c >>>> +++ b/xen/common/libelf/libelf-loader.c >>>> @@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) >>>> sz = sizeof(uint32_t); >>>> >>>> /* Space for the elf and elf section headers */ >>>> - sz += (elf_uval(elf, elf->ehdr, e_ehsize) + >>>> - elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize)); >>>> + sz += elf_uval(elf, elf->ehdr, e_ehsize) + >>>> + 3 * elf_uval(elf, elf->ehdr, e_shentsize); >>> >>> I think a literal 3 like this either needs a #define (at once serving >>> as documentation) or a comment. Perhaps rather the former, >>> considering that the (same?) 3 appears again further down. Plus - >>> what guarantees there are 3 section headers in the image? >> >> This is not related to the image itself, but to the metadata that libelf >> places at the end of the loaded kernel in order to stash the symtab and >> strtab for the guest to use. > > I understand this, yet imo the literal 3 still should be replaced. Yes, I agree. >> The layout is as follows (I should add this to the patch itself as a >> comment, since I guess this is still quite confusing): >> >> +------------------------+ >> | | >> | KERNEL | >> | | >> +------------------------+ pend >> | ALIGNMENT | >> +------------------------+ bsd_symtab_pstart >> | | >> | size | bsd_symtab_pend - bsd_symtab_pstart >> | | >> +------------------------+ >> | | >> | ELF header | >> | | >> +------------------------+ >> | | >> | ELF section header 0 | Dummy section header >> | | >> +------------------------+ >> | | >> | ELF section header 1 | SYMTAB section header >> | | >> +------------------------+ >> | | >> | ELF section header 2 | STRTAB section header >> | | >> +------------------------+ >> | | >> | SYMTAB | >> | | >> +------------------------+ >> | | >> | STRTAB | >> | | >> +------------------------+ bsd_symtab_pend >> >> There are always only tree sections because that's all we need, section >> 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is >> used to describe the STRTAB. > > All fine, but this still doesn't clarify how the kernel learns where > bsd_symtab_pstart is. The BSDs linker scripts places an "end" symbol after all the loaded sections: https://svnweb.freebsd.org/base/head/sys/conf/ldscript.amd64?revision=284870&view=co If you execute: $ nm -n /boot/kernel/kernel Against a FreeBSD kernel the output is as follows: [...] ffffffff81dc4760 B cpu_info ffffffff81dc4b60 B dpcpu ffffffff81dc4b68 B smp_tlb_pmap ffffffff81dc4b70 B drq_rman ffffffff81dc4bb8 B mem_rman ffffffff81dc4c00 B irq_rman ffffffff81dc4c48 B port_rman ffffffff81dc4c90 B tsc_is_invariant ffffffff81dc4c94 B tsc_perf_stat ffffffff81dc4c98 B tsc_freq ffffffff81dc4ca0 B smp_tsc ffffffff81dc4ca8 B HYPERVISOR_shared_info ffffffff81dc4cb0 B xen_vector_callback_enabled ffffffff81dc4cb8 B HYPERVISOR_start_info ffffffff81dc4cc0 A _end ffffffff81dc4cc0 A end >> According to the ELF spec there can only be >> one SYMTAB, so that's all we need. > > Did you perhaps overlook the spec saying "... but this restriction > may be relaxed in the future", plus the fact that we're talking > about an executable file here (which commonly have two symbol > tables - .dynsym and .symtab), not an object one? (This isn't to > say that you need to make the code handle multiple ones, if you > know that *BSD will only ever have one.) DYNSYM is just a subset of SYMTAB, BSDs prefer (it's not a strict requirement) the SYMTAB because of the in-kernel debugger. Also DYNSYM is already loaded by default because it's covered by the program headers. I can add support for loading multiple SYMTABs/STRTABs, but shouldn't we wait until the spec is updated? As I read the spec ATM, an ELF file with multiple SYMTABs is invalid. I assume that if the ELF format ever allows more than one SYMTAB, the version is going to be bumped at least (or some other field is going to be used to signal this change). >>>> sz = elf_round_up(elf, sz); >>>> >>>> - /* Space for the symbol and string tables. */ >>>> + /* Space for the symbol and string table. */ >>>> 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)); >>>> + >>>> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >>>> + continue; >>>> + >>>> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >>>> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >>>> + >>>> + 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_STRTAB ) >>>> + /* Invalid symtab -> strtab link */ >>>> + break; >>> >>> This is not sufficient - what if sh_link is out of bounds, but the >>> bogusly accessed field happens to hold SHT_STRTAB? (Oddly >>> enough you have at least an SHN_UNDEF check in the second >>> loop below.) >> >> The out-of-bounds access would be detected by elf_access_ok (if it's out >> of the scope of the ELF file, which is all we care about). In fact the >> elf_access_ok above could be removed because elf_uval already performs >> out-of-bounds checks. The result is definitely no worse that what we are >> doing ATM. > > No, the out of bounds check should be more strict than just > considering the whole image: The image is broken if the link > points to a non-existing section. Ah, do you mean I should mark the image as broken if either of the checks fail? In this case the image is broken if the sh_link field points to anything different than a STRTAB section, so I should add a elf_mark_broken before the break. elf_access_ok already marks the image as broken if an out-of-bounds access is attempted. >>>> @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, >>>> uint64_t pstart) >>>> >>>> static void elf_load_bsdsyms(struct elf_binary *elf) >>>> { >>>> - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; >>>> - unsigned long sz; >>>> - elf_ptrval maxva; >>>> - elf_ptrval symbase; >>>> - elf_ptrval symtab_addr; >>>> - ELF_HANDLE_DECL(elf_shdr) shdr; >>>> - unsigned i, type; >>>> + /* >>>> + * Header that is placed at the end of the kernel and allows >>>> + * the OS to find where the symtab and strtab have been loaded. >>>> + * It mimics a valid ELF file header, although it only contains >>>> + * a symtab and a strtab section. >>>> + * >>>> + * NB: according to the ELF spec there's only ONE symtab per ELF >>>> + * file, and accordingly we will only load the corresponding >>>> + * strtab, so we only need three section headers in our fake ELF >>>> + * header (first section header is always a dummy). >>>> + */ >>>> + struct { >>>> + uint32_t size; >>>> + struct { >>>> + elf_ehdr header; >>>> + elf_shdr section[3]; >>>> + } __attribute__((packed)) elf_header; >>>> + } __attribute__((packed)) header; >>> >>> If this is placed at the end, how can the OS reasonably use it >>> without knowing that there are exactly 3 section header >>> entries? I.e. how would it find the base of this structure? >> >> See the commend below, the base is found at the end of the kernel, and >> then the ELF header contains the right pointers to the sections headers >> (by appropriately setting e_shoff). > > Thing is - I can't see which "comment below" you try to refer me to. I was trying to point you to the big diagram/layout that I've posted at the start of the email. It doesn't need to know there are exactly 3 sections, this is fetched form the ELF header e_shnum field. And the ELF header is fetched using the "end" symbol as a reference to pend. >>>> + ELF_HANDLE_DECL(elf_ehdr) header_handle; >>>> + unsigned long shdr_size; >>>> + ELF_HANDLE_DECL(elf_shdr) section_handle; >>>> + ELF_HANDLE_DECL(elf_shdr) image_handle; >>>> + unsigned int i, link; >>>> + elf_ptrval header_base; >>>> + elf_ptrval elf_header_base; >>>> + elf_ptrval symtab_base; >>>> + elf_ptrval strtab_base; >>>> >>>> if ( !elf->bsd_symtab_pstart ) >>>> return; >>>> >>>> -#define elf_hdr_elm(_elf, _hdr, _elm, _val) \ >>>> -do { \ >>>> - if ( elf_64bit(_elf) ) \ >>>> - elf_store_field(_elf, _hdr, e64._elm, _val); \ >>>> - else \ >>>> - elf_store_field(_elf, _hdr, e32._elm, _val); \ >>>> +#define elf_store_field_bitness(_elf, _hdr, _elm, _val) \ >>>> +do { \ >>>> + if ( elf_64bit(_elf) ) \ >>>> + elf_store_field(_elf, _hdr, e64._elm, _val); \ >>>> + else \ >>>> + 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_MAKE_HANDLE(elf_ehdr, symtab_addr); >>>> - sz = elf_uval(elf, elf->ehdr, e_ehsize); >>>> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); >>>> - maxva += sz; /* no round up */ >>>> +#define SYMTAB_INDEX 1 >>>> +#define STRTAB_INDEX 2 >>>> >>>> - elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); >>>> - elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize)); >>>> - elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0); >>>> - elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); >>>> + /* Allow elf_memcpy_safe to write to symbol_header. */ >>>> + elf->caller_xdest_base = &header; >>>> + elf->caller_xdest_size = sizeof(header); >>>> >>>> - /* Copy Elf section headers. */ >>>> - shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); >>>> - sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); >>>> - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), >>>> - ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), >>>> - sz); >>>> - maxva = elf_round_up(elf, (unsigned long)maxva + sz); >>>> + /* >>>> + * Calculate the position of the various elements in GUEST MEMORY SPACE. >>>> + * This addresses MUST only be used with elf_load_image. >>>> + * >>>> + * NB: strtab_base cannot be calculated at this point because we don't >>>> + * know the size of the symtab yet, and the strtab will be placed after it. >>>> + */ >>>> + header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart); >>>> + elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + >>>> + sizeof(uint32_t); >>>> + symtab_base = elf_round_up(elf, header_base + sizeof(header)); >>>> + >>>> + /* Fill the ELF header, copied from the original ELF header. */ >>>> + header_handle = ELF_MAKE_HANDLE(elf_ehdr, >>>> + ELF_REALPTR2PTRVAL(&header.elf_header.header)); >>>> + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle), >>>> + ELF_HANDLE_PTRVAL(elf->ehdr), >>>> + elf_uval(elf, elf->ehdr, e_ehsize)); >>>> + >>>> + /* Set the offset to the shdr array. */ >>>> + elf_store_field_bitness(elf, header_handle, e_shoff, >>>> + offsetof(typeof(header.elf_header), section)); >>>> + >>>> + /* Set the right number of section headers. */ >>>> + elf_store_field_bitness(elf, header_handle, e_shnum, 3); >>>> + >>>> + /* Clear a couple of fields we don't use. */ >>>> + elf_store_field_bitness(elf, header_handle, e_phoff, 0); >>>> + elf_store_field_bitness(elf, header_handle, e_phentsize, 0); >>>> + elf_store_field_bitness(elf, header_handle, e_phnum, 0); >>> >>> Perhaps better just give the structure above an initializer? >> >> Not really, the problem is that we copy the original header from the ELF >> binary, and then we mangle it. This is just a fixup to make it clear >> that no program headers are present (we just use section headers in >> order to describe the SYMTAB and STRTAB positions). > > Ah, I see now. > >>>> + /* Zero the dummy section. */ >>> >>> Dummy? And anyway I think you mean "section header" here. >> >> Yes, the right comment would be: zero the dummy section header. > > But then still - why "dummy"? The 0-th section header has a purpose > beyond being a filler, even if that purpose doesn't matter for the > intentions you have here. OK, what about if I replace the comment with: /* Zero the undefined section header. */ I think that's more inline with the specification. I could also fill it with specific values if you think it's clearer: elf_store_field_bitness(..., sh_name, 0); elf_store_field_bitness(..., sh_type, SHT_NULL); elf_store_field_bitness(..., sh_flags, 0); [...] Which is equivalent to zeroing it. Thanks for the review, Roger.
>>> On 29.02.16 at 11:57, <roger.pau@citrix.com> wrote: > El 29/2/16 a les 10:31, Jan Beulich ha escrit: >>>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote: >>> The layout is as follows (I should add this to the patch itself as a >>> comment, since I guess this is still quite confusing): >>> >>> +------------------------+ >>> | | >>> | KERNEL | >>> | | >>> +------------------------+ pend >>> | ALIGNMENT | >>> +------------------------+ bsd_symtab_pstart >>> | | >>> | size | bsd_symtab_pend - bsd_symtab_pstart >>> | | >>> +------------------------+ >>> | | >>> | ELF header | >>> | | >>> +------------------------+ >>> | | >>> | ELF section header 0 | Dummy section header >>> | | >>> +------------------------+ >>> | | >>> | ELF section header 1 | SYMTAB section header >>> | | >>> +------------------------+ >>> | | >>> | ELF section header 2 | STRTAB section header >>> | | >>> +------------------------+ >>> | | >>> | SYMTAB | >>> | | >>> +------------------------+ >>> | | >>> | STRTAB | >>> | | >>> +------------------------+ bsd_symtab_pend >>> >>> There are always only tree sections because that's all we need, section >>> 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is >>> used to describe the STRTAB. >> >> All fine, but this still doesn't clarify how the kernel learns where >> bsd_symtab_pstart is. > > The BSDs linker scripts places an "end" symbol after all the loaded > sections: > > https://svnweb.freebsd.org/base/head/sys/conf/ldscript.amd64?revision=284870& > view=co That's fine. But how do they know it is legitimate to even touch what follows, not to speak of assign meaning to the value found there? And are there no alignment/padding considerations necessary? >>> According to the ELF spec there can only be >>> one SYMTAB, so that's all we need. >> >> Did you perhaps overlook the spec saying "... but this restriction >> may be relaxed in the future", plus the fact that we're talking >> about an executable file here (which commonly have two symbol >> tables - .dynsym and .symtab), not an object one? (This isn't to >> say that you need to make the code handle multiple ones, if you >> know that *BSD will only ever have one.) > > DYNSYM is just a subset of SYMTAB, BSDs prefer (it's not a strict > requirement) the SYMTAB because of the in-kernel debugger. Also DYNSYM > is already loaded by default because it's covered by the program headers. > > I can add support for loading multiple SYMTABs/STRTABs, but shouldn't we > wait until the spec is updated? As I read the spec ATM, an ELF file with > multiple SYMTABs is invalid. I assume that if the ELF format ever allows > more than one SYMTAB, the version is going to be bumped at least (or > some other field is going to be used to signal this change). As said I don't see the need for you to add multiple symbol table support. I only meant to point out that the potential exists. >>>>> sz = elf_round_up(elf, sz); >>>>> >>>>> - /* Space for the symbol and string tables. */ >>>>> + /* Space for the symbol and string table. */ >>>>> 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)); >>>>> + >>>>> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >>>>> + continue; >>>>> + >>>>> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >>>>> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >>>>> + >>>>> + 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_STRTAB ) >>>>> + /* Invalid symtab -> strtab link */ >>>>> + break; >>>> >>>> This is not sufficient - what if sh_link is out of bounds, but the >>>> bogusly accessed field happens to hold SHT_STRTAB? (Oddly >>>> enough you have at least an SHN_UNDEF check in the second >>>> loop below.) >>> >>> The out-of-bounds access would be detected by elf_access_ok (if it's out >>> of the scope of the ELF file, which is all we care about). In fact the >>> elf_access_ok above could be removed because elf_uval already performs >>> out-of-bounds checks. The result is definitely no worse that what we are >>> doing ATM. >> >> No, the out of bounds check should be more strict than just >> considering the whole image: The image is broken if the link >> points to a non-existing section. > > Ah, do you mean I should mark the image as broken if either of the > checks fail? Perhaps, but my main point continue to be that there is a check missing here. >>>>> @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, >>>>> uint64_t pstart) >>>>> >>>>> static void elf_load_bsdsyms(struct elf_binary *elf) >>>>> { >>>>> - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; >>>>> - unsigned long sz; >>>>> - elf_ptrval maxva; >>>>> - elf_ptrval symbase; >>>>> - elf_ptrval symtab_addr; >>>>> - ELF_HANDLE_DECL(elf_shdr) shdr; >>>>> - unsigned i, type; >>>>> + /* >>>>> + * Header that is placed at the end of the kernel and allows >>>>> + * the OS to find where the symtab and strtab have been loaded. >>>>> + * It mimics a valid ELF file header, although it only contains >>>>> + * a symtab and a strtab section. >>>>> + * >>>>> + * NB: according to the ELF spec there's only ONE symtab per ELF >>>>> + * file, and accordingly we will only load the corresponding >>>>> + * strtab, so we only need three section headers in our fake ELF >>>>> + * header (first section header is always a dummy). >>>>> + */ >>>>> + struct { >>>>> + uint32_t size; >>>>> + struct { >>>>> + elf_ehdr header; >>>>> + elf_shdr section[3]; >>>>> + } __attribute__((packed)) elf_header; >>>>> + } __attribute__((packed)) header; >>>> >>>> If this is placed at the end, how can the OS reasonably use it >>>> without knowing that there are exactly 3 section header >>>> entries? I.e. how would it find the base of this structure? >>> >>> See the commend below, the base is found at the end of the kernel, and >>> then the ELF header contains the right pointers to the sections headers >>> (by appropriately setting e_shoff). >> >> Thing is - I can't see which "comment below" you try to refer me to. > > I was trying to point you to the big diagram/layout that I've posted at > the start of the email. > > It doesn't need to know there are exactly 3 sections, this is fetched > form the ELF header e_shnum field. And the ELF header is fetched using > the "end" symbol as a reference to pend. Ah, okay, so the ABI is _not_ for the kernel to start looking from the end, but to start looking from its own image end. >>>>> + /* Zero the dummy section. */ >>>> >>>> Dummy? And anyway I think you mean "section header" here. >>> >>> Yes, the right comment would be: zero the dummy section header. >> >> But then still - why "dummy"? The 0-th section header has a purpose >> beyond being a filler, even if that purpose doesn't matter for the >> intentions you have here. > > OK, what about if I replace the comment with: > > /* Zero the undefined section header. */ > > I think that's more inline with the specification. I could also fill it > with specific values if you think it's clearer: > > elf_store_field_bitness(..., sh_name, 0); > elf_store_field_bitness(..., sh_type, SHT_NULL); > elf_store_field_bitness(..., sh_flags, 0); > [...] > > Which is equivalent to zeroing it. Just zeroing is fine afaic. Jan
El 29/2/16 a les 13:14, Jan Beulich ha escrit: >>>> On 29.02.16 at 11:57, <roger.pau@citrix.com> wrote: >> El 29/2/16 a les 10:31, Jan Beulich ha escrit: >>>>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote: >>>> The layout is as follows (I should add this to the patch itself as a >>>> comment, since I guess this is still quite confusing): >>>> >>>> +------------------------+ >>>> | | >>>> | KERNEL | >>>> | | >>>> +------------------------+ pend >>>> | ALIGNMENT | >>>> +------------------------+ bsd_symtab_pstart >>>> | | >>>> | size | bsd_symtab_pend - bsd_symtab_pstart >>>> | | >>>> +------------------------+ >>>> | | >>>> | ELF header | >>>> | | >>>> +------------------------+ >>>> | | >>>> | ELF section header 0 | Dummy section header >>>> | | >>>> +------------------------+ >>>> | | >>>> | ELF section header 1 | SYMTAB section header >>>> | | >>>> +------------------------+ >>>> | | >>>> | ELF section header 2 | STRTAB section header >>>> | | >>>> +------------------------+ >>>> | | >>>> | SYMTAB | >>>> | | >>>> +------------------------+ >>>> | | >>>> | STRTAB | >>>> | | >>>> +------------------------+ bsd_symtab_pend >>>> >>>> There are always only tree sections because that's all we need, section >>>> 0 is ignored, section 1 is used to describe the SYMTAB, and section 2 is >>>> used to describe the STRTAB. >>> >>> All fine, but this still doesn't clarify how the kernel learns where >>> bsd_symtab_pstart is. >> >> The BSDs linker scripts places an "end" symbol after all the loaded >> sections: >> >> https://svnweb.freebsd.org/base/head/sys/conf/ldscript.amd64?revision=284870& >> view=co > > That's fine. But how do they know it is legitimate to even touch what > follows, not to speak of assign meaning to the value found there? The kernel signals that it wants it's SYMTAB/STRTAB loaded using the XEN_ELFNOTE_BSD_SYMTAB ELFNOTE. Then AFAICT it's just a matter of 'faith', there's no signal from Xen to the guest kernel in order to confirm that the SYMTAB has indeed been loaded... There's the ELF magic in the header, that one can check in order to make sure, but apart from that I don't think there's anything else. > And are there no alignment/padding considerations necessary? bsd_symtab_pstart is aligned to 4 or 8 bytes (depending on the kernel bitness). IMHO, the best way to solve this would be to pass the SYMTAB and STRTAB as modules for PVH (using the new module list that was introduced with the new boot ABI), and use the module command line to signal which one is the strtab and which one is the symtab. I think this can be done in a backwards compatible way, but this is out of the scope of this patch. >>>>>> sz = elf_round_up(elf, sz); >>>>>> >>>>>> - /* Space for the symbol and string tables. */ >>>>>> + /* Space for the symbol and string table. */ >>>>>> 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)); >>>>>> + >>>>>> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >>>>>> + continue; >>>>>> + >>>>>> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >>>>>> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >>>>>> + >>>>>> + 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_STRTAB ) >>>>>> + /* Invalid symtab -> strtab link */ >>>>>> + break; >>>>> >>>>> This is not sufficient - what if sh_link is out of bounds, but the >>>>> bogusly accessed field happens to hold SHT_STRTAB? (Oddly >>>>> enough you have at least an SHN_UNDEF check in the second >>>>> loop below.) >>>> >>>> The out-of-bounds access would be detected by elf_access_ok (if it's out >>>> of the scope of the ELF file, which is all we care about). In fact the >>>> elf_access_ok above could be removed because elf_uval already performs >>>> out-of-bounds checks. The result is definitely no worse that what we are >>>> doing ATM. >>> >>> No, the out of bounds check should be more strict than just >>> considering the whole image: The image is broken if the link >>> points to a non-existing section. >> >> Ah, do you mean I should mark the image as broken if either of the >> checks fail? > > Perhaps, but my main point continue to be that there is a check > missing here. I'm quite sure I'm missing something, but what kind of extra checks do you envision? AFAICT, we already check that the section we are trying to load is not out-of-bounds (both elf_access_ok and elf_uval make sure of that), and that it has the right type (STRTAB). Apart from that, I don't know what else to check. There's no signature in the section headers in order to check it's integrity. Roger.
>>> On 29.02.16 at 17:20, <roger.pau@citrix.com> wrote: > El 29/2/16 a les 13:14, Jan Beulich ha escrit: >>>>> On 29.02.16 at 11:57, <roger.pau@citrix.com> wrote: >>> El 29/2/16 a les 10:31, Jan Beulich ha escrit: >>>>>>> On 26.02.16 at 18:02, <roger.pau@citrix.com> wrote: >>>>>>> - /* Space for the symbol and string tables. */ >>>>>>> + /* Space for the symbol and string table. */ >>>>>>> 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)); >>>>>>> + >>>>>>> + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) >>>>>>> + continue; >>>>>>> + >>>>>>> + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); >>>>>>> + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); >>>>>>> + >>>>>>> + 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_STRTAB ) >>>>>>> + /* Invalid symtab -> strtab link */ >>>>>>> + break; >>>>>> >>>>>> This is not sufficient - what if sh_link is out of bounds, but the >>>>>> bogusly accessed field happens to hold SHT_STRTAB? (Oddly >>>>>> enough you have at least an SHN_UNDEF check in the second >>>>>> loop below.) >>>>> >>>>> The out-of-bounds access would be detected by elf_access_ok (if it's out >>>>> of the scope of the ELF file, which is all we care about). In fact the >>>>> elf_access_ok above could be removed because elf_uval already performs >>>>> out-of-bounds checks. The result is definitely no worse that what we are >>>>> doing ATM. >>>> >>>> No, the out of bounds check should be more strict than just >>>> considering the whole image: The image is broken if the link >>>> points to a non-existing section. >>> >>> Ah, do you mean I should mark the image as broken if either of the >>> checks fail? >> >> Perhaps, but my main point continue to be that there is a check >> missing here. > > I'm quite sure I'm missing something, but what kind of extra checks do > you envision? 0 < sh_link < elf_shdr_count(elf) Jan
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 5039f3f..62d421a 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -133,204 +133,6 @@ static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom) return 0; } -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(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2; - xen_vaddr_t symtab, maxaddr; - elf_ptrval hdr; - size_t size; - unsigned h, count, type, i, tables = 0; - unsigned long *strtab_referenced = NULL; - - if ( elf_swap(elf) ) - { - DOMPRINTF("%s: non-native byte order, bsd symtab not supported", - __FUNCTION__); - return 0; - } - - size = elf->bsd_symtab_pend - elf->bsd_symtab_pstart; - - if ( load ) - { - char *hdr_ptr; - size_t allow_size; - - if ( !dom->bsd_symtab_start ) - return 0; - hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); - if ( hdr_ptr == NULL ) - { - DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom,dom->bsd_symtab_start" - " => NULL", __FUNCTION__); - return -1; - } - elf->caller_xdest_base = hdr_ptr; - elf->caller_xdest_size = allow_size; - hdr = ELF_REALPTR2PTRVAL(hdr_ptr); - elf_store_val(elf, unsigned, hdr, size - sizeof(unsigned)); - } - else - { - char *hdr_ptr; - - 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); - dom->kernel_seg.vend = elf_round_up(elf, dom->bsd_symtab_start + size); - return 0; - } - - elf_memcpy_safe(elf, hdr + sizeof(unsigned), - ELF_IMAGE_BASE(elf), - 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(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; - } - else - { - 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(unsigned) ) - { - DOMPRINTF("%s: header size %"PRIx64" too small", - __FUNCTION__, (uint64_t)elf->caller_xdest_size); - return -1; - } - if ( elf_init(&syms, elf->caller_xdest_base + sizeof(unsigned), - elf->caller_xdest_size - sizeof(unsigned)) ) - 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(unsigned); - maxaddr = elf_round_up(&syms, symtab + elf_size(&syms, syms.ehdr) + - elf_shdr_count(&syms) * elf_size(&syms, shdr)); - - DOMPRINTF("%s: bsd_symtab_start=%" PRIx64 ", kernel.end=0x%" PRIx64 - " -- symtab=0x%" PRIx64 ", maxaddr=0x%" PRIx64 "", - __FUNCTION__, dom->bsd_symtab_start, dom->kernel_seg.vend, - symtab, maxaddr); - - count = elf_shdr_count(&syms); - /* elf_shdr_count guarantees that count is reasonable */ - - strtab_referenced = xc_dom_malloc(dom, bitmap_size(count)); - if ( strtab_referenced == NULL ) - return -1; - bitmap_clear(strtab_referenced, count); - /* Note the symtabs @h linked to by any strtab @i. */ - for ( i = 0; i < count; i++ ) - { - shdr2 = elf_shdr_by_index(&syms, i); - if ( elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB ) - { - h = elf_uval(&syms, shdr2, sh_link); - if (h < count) - set_bit(h, strtab_referenced); - } - } - - for ( h = 0; h < count; h++ ) - { - shdr = elf_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 ) - { - /* Skip symtab @h if we found no corresponding strtab @i. */ - if ( !test_bit(h, strtab_referenced) ) - { - if ( elf_64bit(&syms) ) - elf_store_field(elf, shdr, e64.sh_offset, 0); - else - elf_store_field(elf, shdr, e32.sh_offset, 0); - continue; - } - } - - if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) ) - { - /* Mangled to be based on ELF header location. */ - if ( elf_64bit(&syms) ) - elf_store_field(elf, shdr, e64.sh_offset, maxaddr - symtab); - else - 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++; - DOMPRINTF("%s: h=%u %s, size=0x%zx, maxaddr=0x%" PRIx64 "", - __FUNCTION__, h, - type == SHT_SYMTAB ? "symtab" : "strtab", - size, maxaddr); - - shdr2 = elf_shdr_by_index(elf, h); - elf_memcpy_safe(elf, elf_section_start(&syms, shdr), - elf_section_start(elf, shdr2), - size); - } - - /* Name is NULL. */ - if ( elf_64bit(&syms) ) - elf_store_field(elf, shdr, e64.sh_name, 0); - else - 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__); - dom->bsd_symtab_start = 0; - return 0; - } - - return 0; -} - static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom) /* * This function sometimes returns -1 for error and sometimes @@ -385,9 +187,6 @@ static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom) dom->kernel_seg.vstart = dom->parms.virt_kstart; dom->kernel_seg.vend = dom->parms.virt_kend; - if ( dom->parms.bsd_symtab ) - xc_dom_load_elf_symtab(dom, elf, 0); - dom->guest_type = xc_dom_guest_type(dom, elf); DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "", __FUNCTION__, dom->guest_type, @@ -422,8 +221,6 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom) DOMPRINTF("%s: failed to load elf binary", __FUNCTION__); return rc; } - if ( dom->parms.bsd_symtab ) - xc_dom_load_elf_symtab(dom, elf, 1); return 0; } diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 6f42bea..5875795 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -153,7 +153,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) { uint64_t sz; ELF_HANDLE_DECL(elf_shdr) shdr; - unsigned i, type; + unsigned int i; if ( !ELF_HANDLE_VALID(elf->sym_tab) ) return; @@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) sz = sizeof(uint32_t); /* Space for the elf and elf section headers */ - sz += (elf_uval(elf, elf->ehdr, e_ehsize) + - elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize)); + sz += elf_uval(elf, elf->ehdr, e_ehsize) + + 3 * elf_uval(elf, elf->ehdr, e_shentsize); sz = elf_round_up(elf, sz); - /* Space for the symbol and string tables. */ + /* Space for the symbol and string table. */ 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)); + + if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) + continue; + + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); + shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); + + 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_STRTAB ) + /* Invalid symtab -> strtab link */ + break; + + sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); } elf->bsd_symtab_pstart = pstart; @@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) static void elf_load_bsdsyms(struct elf_binary *elf) { - ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; - unsigned long sz; - elf_ptrval maxva; - elf_ptrval symbase; - elf_ptrval symtab_addr; - ELF_HANDLE_DECL(elf_shdr) shdr; - unsigned i, type; + /* + * Header that is placed at the end of the kernel and allows + * the OS to find where the symtab and strtab have been loaded. + * It mimics a valid ELF file header, although it only contains + * a symtab and a strtab section. + * + * NB: according to the ELF spec there's only ONE symtab per ELF + * file, and accordingly we will only load the corresponding + * strtab, so we only need three section headers in our fake ELF + * header (first section header is always a dummy). + */ + struct { + uint32_t size; + struct { + elf_ehdr header; + elf_shdr section[3]; + } __attribute__((packed)) elf_header; + } __attribute__((packed)) header; + + ELF_HANDLE_DECL(elf_ehdr) header_handle; + unsigned long shdr_size; + ELF_HANDLE_DECL(elf_shdr) section_handle; + ELF_HANDLE_DECL(elf_shdr) image_handle; + unsigned int i, link; + elf_ptrval header_base; + elf_ptrval elf_header_base; + elf_ptrval symtab_base; + elf_ptrval strtab_base; if ( !elf->bsd_symtab_pstart ) return; -#define elf_hdr_elm(_elf, _hdr, _elm, _val) \ -do { \ - if ( elf_64bit(_elf) ) \ - elf_store_field(_elf, _hdr, e64._elm, _val); \ - else \ - elf_store_field(_elf, _hdr, e32._elm, _val); \ +#define elf_store_field_bitness(_elf, _hdr, _elm, _val) \ +do { \ + if ( elf_64bit(_elf) ) \ + elf_store_field(_elf, _hdr, e64._elm, _val); \ + else \ + 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_MAKE_HANDLE(elf_ehdr, symtab_addr); - sz = elf_uval(elf, elf->ehdr, e_ehsize); - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); - maxva += sz; /* no round up */ +#define SYMTAB_INDEX 1 +#define STRTAB_INDEX 2 - elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); - elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize)); - elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0); - elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); + /* Allow elf_memcpy_safe to write to symbol_header. */ + elf->caller_xdest_base = &header; + elf->caller_xdest_size = sizeof(header); - /* Copy Elf section headers. */ - shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); - sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); - elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), - ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), - sz); - maxva = elf_round_up(elf, (unsigned long)maxva + sz); + /* + * Calculate the position of the various elements in GUEST MEMORY SPACE. + * This addresses MUST only be used with elf_load_image. + * + * NB: strtab_base cannot be calculated at this point because we don't + * know the size of the symtab yet, and the strtab will be placed after it. + */ + header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart); + elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) + + sizeof(uint32_t); + symtab_base = elf_round_up(elf, header_base + sizeof(header)); + + /* Fill the ELF header, copied from the original ELF header. */ + header_handle = ELF_MAKE_HANDLE(elf_ehdr, + ELF_REALPTR2PTRVAL(&header.elf_header.header)); + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle), + ELF_HANDLE_PTRVAL(elf->ehdr), + elf_uval(elf, elf->ehdr, e_ehsize)); + + /* Set the offset to the shdr array. */ + elf_store_field_bitness(elf, header_handle, e_shoff, + offsetof(typeof(header.elf_header), section)); + + /* Set the right number of section headers. */ + elf_store_field_bitness(elf, header_handle, e_shnum, 3); + + /* Clear a couple of fields we don't use. */ + elf_store_field_bitness(elf, header_handle, e_phoff, 0); + elf_store_field_bitness(elf, header_handle, e_phentsize, 0); + elf_store_field_bitness(elf, header_handle, e_phnum, 0); + + /* Zero the dummy section. */ + section_handle = ELF_MAKE_HANDLE(elf_shdr, + ELF_REALPTR2PTRVAL(&header.elf_header.section[SHN_UNDEF])); + shdr_size = elf_uval(elf, elf->ehdr, e_shentsize); + elf_memset_safe(elf, ELF_HANDLE_PTRVAL(section_handle), 0, shdr_size); + /* + * Find the actual symtab and strtab in the ELF. + * + * The symtab section header is going to reside in section[SYMTAB_INDEX], + * while the corresponding strtab is going to be placed in + * section[STRTAB_INDEX]. sh_offset is mangled so it points to the offset + * where the sections are actually loaded (relative to the ELF header + * location). + */ + section_handle = ELF_MAKE_HANDLE(elf_shdr, + ELF_REALPTR2PTRVAL(&header.elf_header.section[SYMTAB_INDEX])); 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) ) + image_handle = elf_shdr_by_index(elf, i); + if ( elf_uval(elf, image_handle, sh_type) != SHT_SYMTAB ) + continue; + + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), + ELF_HANDLE_PTRVAL(image_handle), + shdr_size); + + link = elf_uval(elf, section_handle, sh_link); + if ( link == SHN_UNDEF ) { - 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); - 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_round_up(elf, (unsigned long)maxva + sz); + elf_mark_broken(elf, "bad link in symtab"); + break; } - 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 */ + + /* Load symtab into guest memory. */ + elf_load_image(elf, symtab_base, elf_section_start(elf, section_handle), + elf_uval(elf, section_handle, sh_size), + elf_uval(elf, section_handle, sh_size)); + elf_store_field_bitness(elf, section_handle, sh_offset, + symtab_base - elf_header_base); + elf_store_field_bitness(elf, section_handle, sh_link, + STRTAB_INDEX); + + /* Calculate the guest address where strtab is loaded. */ + strtab_base = elf_round_up(elf, symtab_base + + elf_uval(elf, section_handle, sh_size)); + + /* Load strtab section header. */ + section_handle = ELF_MAKE_HANDLE(elf_shdr, + ELF_REALPTR2PTRVAL(&header.elf_header.section[STRTAB_INDEX])); + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle), + ELF_HANDLE_PTRVAL(elf_shdr_by_index(elf, link)), + shdr_size); + + if ( elf_uval(elf, section_handle, sh_type) != SHT_STRTAB ) { - elf_mark_broken(elf, "bad section header length"); + elf_mark_broken(elf, "strtab not found"); break; } - if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */ - break; - shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p); + + /* Load strtab into guest memory. */ + elf_load_image(elf, strtab_base, elf_section_start(elf, section_handle), + elf_uval(elf, section_handle, sh_size), + elf_uval(elf, section_handle, sh_size)); + elf_store_field_bitness(elf, section_handle, sh_offset, + strtab_base - elf_header_base); + + /* Store the whole size (including headers and loaded sections). */ + header.size = strtab_base + elf_uval(elf, section_handle, sh_size) - + elf_header_base; + break; } - /* Write down the actual sym size. */ - elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr); + /* Load the headers. */ + elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header), + sizeof(header), sizeof(header)); + + /* Remove permissions from elf_memcpy_safe. */ + elf->caller_xdest_base = NULL; + elf->caller_xdest_size = 0; -#undef elf_ehdr_elm +#undef SYMTAB_INDEX +#undef STRTAB_INDEX +#undef elf_store_field_bitness } void elf_parse_binary(struct elf_binary *elf)
Current implementation of elf_load_bsdsyms is broken when loading inside of a HVM guest, because it assumes elf_memcpy_safe is able to write into guest memory space, which it is not. Take the oportunity to do some cleanup and properly document how elf_{parse/load}_bsdsyms works. The new implementation uses elf_load_image when dealing with data that needs to be copied to the guest memory space. Also reduce the number of section headers copied to the minimum necessary. This patch also removes the duplication of code found in the libxc ELF loader, since the libelf symtab/strtab loading code will also handle this case without having to duplicate it. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/libxc/xc_dom_elfloader.c | 203 ---------------------------------- xen/common/libelf/libelf-loader.c | 223 ++++++++++++++++++++++++++++---------- 2 files changed, 163 insertions(+), 263 deletions(-)