linux-kernel - Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2716a4f-58ca-d385-2ae5-d3804b53fac3@suse.com>
Date:   Thu, 14 Sep 2017 13:29:57 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Julien Grall <julien.grall@....com>,
        Tycho Andersen <tycho@...ker.com>,
        Christoph Hellwig <hch@...radead.org>
Cc:     Marco Benatto <marco.antonio.780@...il.com>,
        kernel-hardening@...ts.openwall.com, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org,
        Juerg Haefliger <juerg.haefliger@...onical.com>,
        xen-devel@...ts.xenproject.org,
        linux-arm-kernel@...ts.infradead.org,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Stefano Stabellini <sstabellini@...nel.org>
Subject: Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

On 14/09/17 12:41, Julien Grall wrote:
> Hi,
> 
> CC Juergen, Boris and Stefano.
> 
> On 08/09/17 18:24, Tycho Andersen wrote:
>> On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote:
>>>> +/*
>>>> + * Lookup the page table entry for a virtual address and return a
>>>> pointer to
>>>> + * the entry. Based on x86 tree.
>>>> + */
>>>> +static pte_t *lookup_address(unsigned long addr)
>>>
>>> Seems like this should be moved to common arm64 mm code and used by
>>> kernel_page_present.
>>
>> Sounds good, I'll include something like the patch below in the next
>> series.
>>
>> Unfortunately, adding an implementation of lookup_address seems to be
>> slightly more complicated than necessary, because of the xen piece. We
>> have to define lookup_address() with the level parameter, but it's not
>> obvious to me to name the page levels. So for now I've just left it as
>> a WARN() if someone supplies it.
>>
>> It seems like xen still does need this to be defined, because if I
>> define it without level:
>>
>> drivers/xen/xenbus/xenbus_client.c: In function
>> ‘xenbus_unmap_ring_vfree_pv’:
>> drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to
>> function ‘lookup_address’
>>      lookup_address(addr, &level)).maddr;
>>      ^~~~~~~~~~~~~~
>> In file included from ./arch/arm64/include/asm/page.h:37:0,
>>                   from ./include/linux/mmzone.h:20,
>>                   from ./include/linux/gfp.h:5,
>>                   from ./include/linux/mm.h:9,
>>                   from drivers/xen/xenbus/xenbus_client.c:33:
>> ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here
>>   extern pte_t *lookup_address(unsigned long addr);
>>                 ^~~~~~~~~~~~~~
>>
>> I've cc-d the xen folks, maybe they can suggest a way to untangle it?
>> Alternatively, if someone can suggest a good naming scheme for the
>> page levels, I can just do that.
> 
> The implementation of lookup_address(...) on ARM for Xen (see
> include/xen/arm/page.h) is just a BUG(). This is because this code
> should never be called (only used for x86 PV code).
> 
> Furthermore, xenbus client does not use at all the level. It is just to
> cope with the x86 version of lookup_address.
> 
> So one way to solve the problem would be to introduce
> xen_lookup_address(addr) that would be implemented as:
>     - on x86
>         unsigned int level;
> 
>         return lookup_address(addr, &level).maddr;
>     - on ARM
>         BUG();
> 
> With that there would be no prototype clash and avoid introducing a
> level parameter.

I'd rather add some #ifdef CONFIG_XEN_PV and remove the *_pv functions
from ARM completely this way. I'm sending a patch soon...


Juergen