| Message ID | 1475680307-66003-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.10.16 at 17:11, <roger.pau@citrix.com> wrote: > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf) > } __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; > elf_ptrval strtab_base; > + void *shdr; I'd appreciate if you moved this into the scope where it's needed. Also I think it could be const. > @@ -394,15 +395,40 @@ 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 = sizeof(header) - sizeof(header.elf_header.section); I think offsetof(typeof(header), elf_header.section) would be the safer expression here, removing the dependency on the packed attribute. > 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. > + */ > + shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) > + { > + if ( elf_64bit(elf) ) > + shdr = &header.elf_header.section[i].e64; > + else > + shdr = &header.elf_header.section[i].e32; > + > + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, > + ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size); You shouldn't read shdr_size bytes here, but only sizeof() ones. Which btw also applies to the earlier memset()ing of the SHN_UNDEF header, which it then also looks like gets overwritten by the loading done here (irrespective of the change you make). Jan
On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote: > >>> On 05.10.16 at 17:11, <roger.pau@citrix.com> wrote: > > --- a/xen/common/libelf/libelf-loader.c > > +++ b/xen/common/libelf/libelf-loader.c > > @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf) > > } __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; > > elf_ptrval strtab_base; > > + void *shdr; > > I'd appreciate if you moved this into the scope where it's needed. > Also I think it could be const. Done. > > @@ -394,15 +395,40 @@ 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 = sizeof(header) - sizeof(header.elf_header.section); > > I think offsetof(typeof(header), elf_header.section) would be the > safer expression here, removing the dependency on the packed > attribute. Right, I was dubious about which one would be better. I've removed the packed attribute of elf_header, but the one from header needs to say (so that the elf_header is just after the uint32_t). > > 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. > > + */ > > + shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); > > + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) > > + { > > + if ( elf_64bit(elf) ) > > + shdr = &header.elf_header.section[i].e64; > > + else > > + shdr = &header.elf_header.section[i].e32; > > + > > + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, > > + ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size); > > You shouldn't read shdr_size bytes here, but only sizeof() ones. Well, shdr_size is just the result of a sizeof. > Which btw also applies to the earlier memset()ing of the SHN_UNDEF > header, which it then also looks like gets overwritten by the loading > done here (irrespective of the change you make). Hm, I'm not sure I follow. Memset()ing of header.elf_header.section[SHN_UNDEF] is required, so that we don't copy contents of the stack into guest memory. This memset is done to the local data, which is then copied into the guest memory space here. Roger.
>>> On 05.10.16 at 18:17, <roger.pau@citrix.com> wrote: > On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote: >> >>> On 05.10.16 at 17:11, <roger.pau@citrix.com> wrote: >> > + /* >> > + * 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. >> > + */ >> > + shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); >> > + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) >> > + { >> > + if ( elf_64bit(elf) ) >> > + shdr = &header.elf_header.section[i].e64; >> > + else >> > + shdr = &header.elf_header.section[i].e32; >> > + >> > + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, >> > + ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size); >> >> You shouldn't read shdr_size bytes here, but only sizeof() ones. > > Well, shdr_size is just the result of a sizeof. Oh, I didn't even spot that - it makes things worse: The value gets retrieved correctly earlier on. Jan
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 2626a40..3faff62 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf) } __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; elf_ptrval strtab_base; + void *shdr; if ( !elf->bsd_symtab_pstart ) return; @@ -394,15 +395,40 @@ 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 = sizeof(header) - sizeof(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. + */ + shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr); + for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ ) + { + if ( elf_64bit(elf) ) + shdr = &header.elf_header.section[i].e64; + else + shdr = &header.elf_header.section[i].e32; + + rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i, + ELF_REALPTR2PTRVAL(shdr), 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. 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. --- xen/common/libelf/libelf-loader.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)