Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Roger Pau Monné]

El 22/01/16 a les 9.11, Jan Beulich ha escrit:
 On 21.01.16 at 18:55,  wrote:
>> El 21/01/16 a les 18.29, Ian Jackson ha escrit:
>>> Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab 
>> loading for Dom0"):
 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.
>>> ...
  #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
  do {\
  if ( elf_64bit(_elf) )  \
 -elf_store_field(_elf, _hdr, e64._elm, _val);  \
 +(_hdr).e64._elm = _val; \
>>>
>>> This seems to bypass the safe store mechanism which was introduced to
>>> fix XSA-55.
>>
>> This macro is only used to store fields inside of a structure that's
>> allocated on the stack, and it doesn't involve any kind of pointer
>> magic/arithmetic. The way it was used previously in this function indeed
>> required the use of the _safe mechanism in order to prevent writing into
>> arbitrary memory places, because we were actually modifying guest memory
>> IIRC.
>>
>> I could restore the previous behaviour, but that would mean adding some
>> handlers in order to access the structure. Since this is only used for
>> Dom0, which already makes use of the elf_memcpy_unchecked function as
>> called by elf_store_val which is in turn called from elf_store_field, so
>> it's not like we were protected previously anyway.
> 
> If this is indeed Dom0-only code, could (and perhaps should) it be
> guarded suitably by an #ifdef to make obvious it's not used for
> DomU loading, and hence not security sensitive? From looking at
> the call sites of elf_{parse,load}_bsdsyms() I can't, btw., tell that
> this is Dom0-only ...

You are completely right, TBH I'm not sure what's going on with this.
xc_dom_elfloader.c has it's own functions to load the strtab/symtab, but
it's also calling elf_load_binary which, AFAICT, will call
elf_load_bsdsyms. Am I missing something, or are we loading the
symtab/strtab twice from libxc?

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Jan Beulich]

>>> On 21.01.16 at 18:55,  wrote:
> El 21/01/16 a les 18.29, Ian Jackson ha escrit:
>> Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab 
> loading for Dom0"):
>>> 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.
>> ...
>>>  #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
>>>  do {\
>>>  if ( elf_64bit(_elf) )  \
>>> -elf_store_field(_elf, _hdr, e64._elm, _val);  \
>>> +(_hdr).e64._elm = _val; \
>> 
>> This seems to bypass the safe store mechanism which was introduced to
>> fix XSA-55.
> 
> This macro is only used to store fields inside of a structure that's
> allocated on the stack, and it doesn't involve any kind of pointer
> magic/arithmetic. The way it was used previously in this function indeed
> required the use of the _safe mechanism in order to prevent writing into
> arbitrary memory places, because we were actually modifying guest memory
> IIRC.
> 
> I could restore the previous behaviour, but that would mean adding some
> handlers in order to access the structure. Since this is only used for
> Dom0, which already makes use of the elf_memcpy_unchecked function as
> called by elf_store_val which is in turn called from elf_store_field, so
> it's not like we were protected previously anyway.

If this is indeed Dom0-only code, could (and perhaps should) it be
guarded suitably by an #ifdef to make obvious it's not used for
DomU loading, and hence not security sensitive? From looking at
the call sites of elf_{parse,load}_bsdsyms() I can't, btw., tell that
this is Dom0-only ...

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab loading 
for Dom0"):
> 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.
...
>  #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
>  do {\
>  if ( elf_64bit(_elf) )  \
> -elf_store_field(_elf, _hdr, e64._elm, _val);  \
> +(_hdr).e64._elm = _val; \

This seems to bypass the safe store mechanism which was introduced to
fix XSA-55.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH v4 1/6] libelf: rewrite symtab/strtab 
loading for Dom0"):
> El 21/01/16 a les 18.29, Ian Jackson ha escrit:
> > Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab 
> > loading for Dom0"):
> >>  #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
> >>  do {\
> >>  if ( elf_64bit(_elf) )  \
> >> -elf_store_field(_elf, _hdr, e64._elm, _val);  \
> >> +(_hdr).e64._elm = _val; \
> > 
> > This seems to bypass the safe store mechanism which was introduced to
> > fix XSA-55.
> 
> This macro is only used to store fields inside of a structure that's
> allocated on the stack, and it doesn't involve any kind of pointer
> magic/arithmetic. The way it was used previously in this function indeed
> required the use of the _safe mechanism in order to prevent writing into
> arbitrary memory places, because we were actually modifying guest memory
> IIRC.

Aha.  Hmm.

The problem is that someone might use this macro to access a structure
that was _not_ stored on the stack.

...

Oh, I see, you have changed the type of _hdr from an ELF_MAKE_HANDLE
to an actual literal structure.  So actually this change is correct.
I think this ought to be (have been) mentioned in the commit message.

> I could restore the previous behaviour, but that would mean adding some
> handlers in order to access the structure. Since this is only used for
> Dom0, which already makes use of the elf_memcpy_unchecked function as
> called by elf_store_val which is in turn called from elf_store_field, so
> it's not like we were protected previously anyway.

elf_store_val calls elf_access_ok before calling elf_memcpy_unchecked.

It wouldn't need `handlers', it would need the struct being registered
as the xdest.  But there is no need if it never needs to be accessed
through any kind of guest-influenceable pointers.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Roger Pau Monné]

El 21/01/16 a les 18.29, Ian Jackson ha escrit:
> Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab loading 
> for Dom0"):
>> 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.
> ...
>>  #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
>>  do {\
>>  if ( elf_64bit(_elf) )  \
>> -elf_store_field(_elf, _hdr, e64._elm, _val);  \
>> +(_hdr).e64._elm = _val; \
> 
> This seems to bypass the safe store mechanism which was introduced to
> fix XSA-55.

This macro is only used to store fields inside of a structure that's
allocated on the stack, and it doesn't involve any kind of pointer
magic/arithmetic. The way it was used previously in this function indeed
required the use of the _safe mechanism in order to prevent writing into
arbitrary memory places, because we were actually modifying guest memory
IIRC.

I could restore the previous behaviour, but that would mean adding some
handlers in order to access the structure. Since this is only used for
Dom0, which already makes use of the elf_memcpy_unchecked function as
called by elf_store_val which is in turn called from elf_store_field, so
it's not like we were protected previously anyway.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0

[Roger Pau Monne]

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.

Signed-off-by: Roger Pau Monné 
---
Cc: Ian Campbell 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Keir Fraser 
Cc: Tim Deegan 
---
 xen/common/libelf/libelf-loader.c | 213 --
 1 file changed, 158 insertions(+), 55 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c 
b/xen/common/libelf/libelf-loader.c
index 6f42bea..9552d4c 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,13 +199,31 @@ 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 __packed {
+elf_ehdr header;
+elf_shdr section[3];
+} symbol_header;
+
+ELF_HANDLE_DECL(elf_ehdr) header_handle;
+unsigned long shdr_size;
+uint32_t symsize;
+ELF_HANDLE_DECL(elf_shdr) section_handle;
+ELF_HANDLE_DECL(elf_shdr) image_handle;
+unsigned int i, link;
+elf_ptrval header_base;
+elf_ptrval symtab_base;
+elf_ptrval strtab_base;
 
 if ( !elf->bsd_symtab_pstart )
 return;
@@ -200,64 +231,136 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
 #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
 do {\
 if ( elf_64bit(_elf) )  \
-elf_store_field(_elf, _hdr, e64._elm, _val);  \
+(_hdr).e64._elm = _val; \
 else\
-elf_store_field(_elf, _hdr, e32._elm, _val);  \
+(_hdr).e32._elm = _val; \
 } while ( 0 )
 
-symbase = elf_get_ptr(elf, elf->bsd_symtab_pstart);
-symtab_addr = maxva = symbase + sizeof(uint32_t);
+#define SYMTAB_INDEX1
+#define STRTAB_INDEX2
 
-/* Set up Elf header. */
-sym_ehdr = ELF_MAKE_HANDLE(elf_ehdr, symtab_addr);
-sz =