|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/4] libelf: rewrite symtab/strtab loading
El 26/2/16 a les 14:15, Jan Beulich ha escrit:
>>>> On 16.02.16 at 18:37, <roger.pau@xxxxxxxxxx> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |