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

["Roger Pau Monné" <roger.pau@citrix.com>]

El 22/01/16 a les 9.11, Jan Beulich ha escrit:
>>>> On 21.01.16 at 18:55, <roger.pau@citrix.com> 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.