* [PATCH XSA-followup for-4.8] libelf: fix symtab/strtab loading for 32bit domains
@ 2016-11-22 17:39 Roger Pau Monne
2016-11-23 2:47 ` Wei Liu
0 siblings, 1 reply; 2+ messages in thread
From: Roger Pau Monne @ 2016-11-22 17:39 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Brian Marcotte, Ian Jackson, Tim Deegan, Jan Beulich,
Roger Pau Monne
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
a couple of consistency issues, by making sure we always use the sizes of
our local versions of the ELF header and the ELF sections headers.
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>
Reviewed-by: Jan Beulich <jbeulich@suse.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 v4:
- Fix consistency issues: make sure the sizes of our local copy of the ELF
header and the ELF section headers are always used.
Changes since v3:
- Move the definition of elf_sym_header into libelf-loader.c.
Changes since v2:
- Use offsetof to correctly account for the memory used by the elf header.
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 | 67 ++++++++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 19 deletions(-)
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index d67e0a7..eb7569d 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -21,10 +21,17 @@
#include "libelf-private.h"
+/* ------------------------------------------------------------------------ */
+
/* Number of section header needed in order to fit the SYMTAB and STRTAB. */
#define ELF_BSDSYM_SECTIONS 3
-
-/* ------------------------------------------------------------------------ */
+struct elf_sym_header {
+ uint32_t size;
+ struct {
+ elf_ehdr header;
+ elf_shdr section[ELF_BSDSYM_SECTIONS];
+ } elf_header;
+} __attribute__((packed));
elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t size)
{
@@ -172,9 +179,10 @@ 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) +
- ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize);
+ /* Space for the ELF header and section headers */
+ sz += offsetof(struct elf_sym_header, elf_header.section) +
+ ELF_BSDSYM_SECTIONS * (elf_64bit(elf) ? sizeof(Elf64_Shdr) :
+ sizeof(Elf32_Shdr));
sz = elf_round_up(elf, sz);
/*
@@ -251,18 +259,11 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
* strtab, so we only need three section headers in our fake ELF
* header (first section header is always the undefined section).
*/
- struct {
- uint32_t size;
- struct {
- elf_ehdr header;
- elf_shdr section[ELF_BSDSYM_SECTIONS];
- } __attribute__((packed)) elf_header;
- } __attribute__((packed)) header;
-
+ struct elf_sym_header header;
ELF_HANDLE_DECL(elf_ehdr) header_handle;
- unsigned long shdr_size;
+ unsigned long shdr_size, ehdr_size, header_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;
@@ -301,8 +302,15 @@ do { \
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));
+ ELF_HANDLE_PTRVAL(elf->ehdr), ehdr_size);
+
+ /*
+ * Set the ELF header size, section header entry size and version
+ * (in case we are dealing with an input ELF header that has extensions).
+ */
+ elf_store_field_bitness(elf, header_handle, e_ehsize, ehdr_size);
+ elf_store_field_bitness(elf, header_handle, e_shentsize, shdr_size);
+ elf_store_field_bitness(elf, header_handle, e_version, EV_CURRENT);
/* Set the offset to the shdr array. */
elf_store_field_bitness(elf, header_handle, e_shoff,
@@ -315,6 +323,7 @@ do { \
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);
+ elf_store_field_bitness(elf, header_handle, e_shstrndx, 0);
shdr_size = elf_uval(elf, elf->ehdr, e_shentsize);
@@ -387,15 +396,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. */
+ header_size = offsetof(typeof(header), elf_header.section);
rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header),
- sizeof(header), sizeof(header));
+ header_size, header_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 + header_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_set_xdest(elf, NULL, 0);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH XSA-followup for-4.8] libelf: fix symtab/strtab loading for 32bit domains
2016-11-22 17:39 [PATCH XSA-followup for-4.8] libelf: fix symtab/strtab loading for 32bit domains Roger Pau Monne
@ 2016-11-23 2:47 ` Wei Liu
0 siblings, 0 replies; 2+ messages in thread
From: Wei Liu @ 2016-11-23 2:47 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Brian Marcotte, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel
On Tue, Nov 22, 2016 at 05:39:37PM +0000, Roger Pau Monne wrote:
> 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
> a couple of consistency issues, by making sure we always use the sizes of
> our local versions of the ELF header and the ELF sections headers.
>
> 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
This doesn't seem to build for me.
64 bit:
../../xen/common/libelf/libelf-loader.c: In function ‘elf_load_bsdsyms’:
../../xen/common/libelf/libelf-loader.c:304:5: error: ‘ehdr_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle),
^
../../xen/common/libelf/libelf-loader.c:264:30: note: ‘ehdr_size’ was declared here
unsigned long shdr_size, ehdr_size, header_size;
^
../../xen/common/libelf/libelf-loader.c:312:99: error: ‘shdr_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
elf_store_field_bitness(elf, header_handle, e_shentsize, shdr_size);
^
../../xen/common/libelf/libelf-loader.c:264:19: note: ‘shdr_size’ was declared here
unsigned long shdr_size, ehdr_size, header_size;
^
cc1: all warnings being treated as errors
32 bit:
../../xen/common/libelf/libelf-loader.c:304:20: error: 'ehdr_size' may be used uninitialized in this function [-Werror=uninitialized]
../../xen/common/libelf/libelf-loader.c:312:5: error: 'shdr_size' may be used uninitialized in this function [-Werror=uninitialized]
This patch is applied on top of 986f790e0184d4bf575462077378e14fa9f85aa9.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-11-23 2:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-22 17:39 [PATCH XSA-followup for-4.8] libelf: fix symtab/strtab loading for 32bit domains Roger Pau Monne
2016-11-23 2:47 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.