| Message ID | 1476115216-1082-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Roger Pau Monne writes ("[PATCH v2 for-4.8] libelf: fix symtab/strtab loading for 32bit domains"):
> Commit ed04ca introduced a bug in the symtab/strtab loading for 32bit
> guests, that corrupted the section headers array due to the padding
> introduced by the elf_shdr union.
>
> The Elf section header array on 32bit should be accessible as an array of
> Elf32_Shdr elements, and the union with Elf64_Shdr done in elf_shdr was
> breaking this due to size differences between Elf32_Shdr and Elf64_Shdr.
>
> Fix this by copying each section header one by one, and using the proper
> size depending on the bitness of the guest kernel. While there, also fix
> elf_parse_bsdsyms so that it takes into account the size of the elf_ehdr
> struct instead of the native Elf header size.
>
> Reported-by: Brian Marcotte <marcotte@panix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>> On 10.10.16 at 18:00, <roger.pau@citrix.com> wrote: > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -174,8 +174,8 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) > /* Space to store the size of the elf image */ > sz = sizeof(uint32_t); > > - /* Space for the elf and elf section headers */ > - sz += elf_uval(elf, elf->ehdr, e_ehsize) + > + /* Space for the elf header and elf section headers */ > + sz += sizeof(elf_ehdr) + > ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize); This really should remain consistent: Either you allocate (and later copy) as much as the respective input fields say, or you use sizeof() (and then update both e_ehsize and e_shentsize accordingly in the loaded image). In particular ... > + /* > + * Load the section headers. > + * > + * NB: this _must_ be done one by one, and taking the bitness into account, > + * so that the guest can treat this as an array of type Elf{32/64}_Shdr. > + */ > + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) > + { > + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, > + ELF_REALPTR2PTRVAL(&header.elf_header.section[i]), > + shdr_size, shdr_size); ... here you may copy more data than there actually is at the source pointer (which has only sizeof(elf_shdr) bytes). Jan
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 2626a40..47699ec 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -174,8 +174,8 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) /* Space to store the size of the elf image */ sz = sizeof(uint32_t); - /* Space for the elf and elf section headers */ - sz += elf_uval(elf, elf->ehdr, e_ehsize) + + /* Space for the elf header and elf section headers */ + sz += sizeof(elf_ehdr) + ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize); sz = elf_round_up(elf, sz); @@ -258,13 +258,13 @@ static void elf_load_bsdsyms(struct elf_binary *elf) struct { elf_ehdr header; elf_shdr section[ELF_BSDSYM_SECTIONS]; - } __attribute__((packed)) elf_header; + } elf_header; } __attribute__((packed)) header; ELF_HANDLE_DECL(elf_ehdr) header_handle; - unsigned long shdr_size; + unsigned long shdr_size, ehdr_size; ELF_HANDLE_DECL(elf_shdr) section_handle; - unsigned int link, rc; + unsigned int link, rc, i; elf_ptrval header_base; elf_ptrval elf_header_base; elf_ptrval symtab_base; @@ -394,15 +394,35 @@ do { \ header.size = strtab_base + elf_uval(elf, section_handle, sh_size) - elf_header_base; - /* Load the headers. */ + /* Load the size plus elf header. */ + ehdr_size = offsetof(typeof(header), elf_header.section); rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header), - sizeof(header), sizeof(header)); + ehdr_size, ehdr_size); if ( rc != 0 ) { elf_mark_broken(elf, "unable to load ELF headers into guest memory"); return; } + /* + * Load the section headers. + * + * NB: this _must_ be done one by one, and taking the bitness into account, + * so that the guest can treat this as an array of type Elf{32/64}_Shdr. + */ + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) + { + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, + ELF_REALPTR2PTRVAL(&header.elf_header.section[i]), + shdr_size, shdr_size); + if ( rc != 0 ) + { + elf_mark_broken(elf, + "unable to load ELF section header into guest memory"); + return; + } + } + /* Remove permissions from elf_memcpy_safe. */ elf->caller_xdest_base = NULL; elf->caller_xdest_size = 0;
Commit ed04ca introduced a bug in the symtab/strtab loading for 32bit guests, that corrupted the section headers array due to the padding introduced by the elf_shdr union. The Elf section header array on 32bit should be accessible as an array of Elf32_Shdr elements, and the union with Elf64_Shdr done in elf_shdr was breaking this due to size differences between Elf32_Shdr and Elf64_Shdr. Fix this by copying each section header one by one, and using the proper size depending on the bitness of the guest kernel. While there, also fix elf_parse_bsdsyms so that it takes into account the size of the elf_ehdr struct instead of the native Elf header size. Reported-by: Brian Marcotte <marcotte@panix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Brian Marcotte <marcotte@panix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> --- Should be backported to Xen 4.7 stable branch. --- Changes since v1: - No need to calculate shdr_size again, it's already fetched from the original elf header. - Remove shdr variable. - Use offsetof instead of subtracting two sizeofs. - Fix elf_parse_bsdsyms so that it takes into account the size of elf_ehdr instead of the size of the native elf header. --- xen/common/libelf/libelf-loader.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)