From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Subject: Re: [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0 Date: Fri, 22 Jan 2016 10:58:44 +0100 Message-ID: <56A1FD54.108@citrix.com> References: <1453395092-88090-1-git-send-email-roger.pau@citrix.com> <1453395092-88090-2-git-send-email-roger.pau@citrix.com> <22177.5520.10709.9657@mariner.uk.xensource.com> <56A11B91.30909@citrix.com> <56A1F23C02000078000C9E2C@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aMYWk-0001ho-Du for xen-devel@lists.xenproject.org; Fri, 22 Jan 2016 10:01:10 +0000 In-Reply-To: <56A1F23C02000078000C9E2C@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xenproject.org, Keir Fraser , Ian Jackson , Ian Campbell , Tim Deegan List-Id: xen-devel@lists.xenproject.org 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.