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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel@lists.xenproject.org, Keir Fraser <keir@xen.org>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>, Tim Deegan <tim@xen.org>
Subject: Re: [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0
Date: Fri, 22 Jan 2016 10:58:44 +0100	[thread overview]
Message-ID: <56A1FD54.108@citrix.com> (raw)
In-Reply-To: <56A1F23C02000078000C9E2C@prv-mh.provo.novell.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.