Re: [Xen-devel] [PATCH] Libxc: fix xc_translate_foreign_address()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/04/2017 08:04 PM, Andrew Cooper wrote:
> On 04/04/17 17:45, Razvan Cojocaru wrote:
>> On 04/04/2017 07:08 PM, Tamas K Lengyel wrote:
>>>
>>> On Tue, Apr 4, 2017 at 9:58 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx
>>> <mailto:andrew.cooper3@xxxxxxxxxx>> wrote:
>>>
>>>     On 04/04/17 16:39, Tamas K Lengyel wrote:
>>>>
>>>>     On Tue, Apr 4, 2017 at 9:11 AM, Andrew Cooper
>>>>     <andrew.cooper3@xxxxxxxxxx <mailto:andrew.cooper3@xxxxxxxxxx>> wrote:
>>>>
>>>>         On 04/04/17 13:14, Razvan Cojocaru wrote:
>>>>         > Currently xc_translate_foreign_address only checks for PSE
>>>>         bit only on
>>>>         > level 2 entries (that's 2 MB pages on x64 and 32-bit with
>>>>         PAE, and 4 MB
>>>>         > pages on 32-bit). But linux kernel sometimes uses 1 GB
>>>>         pages. This patch
>>>>         > fixes that, and checks the PSE bit on level 3 entries if the
>>>>         guest has 4
>>>>         > translation levels (that means 64-bit guests only).
>>>>         >
>>>>         > Signed-off-by: Cristian-Bogdan Sirb <csirb@xxxxxxxxxxxxxxx
>>>>         <mailto:csirb@xxxxxxxxxxxxxxx>>
>>>>
>>>>         This function is in a rather tragic state.  Lucky it isn't
>>>>         used by code
>>>>         covered by Xen's security statement.
>>>>
>>>>         This patch reintroduces XSA-176, and the existing code doesn't
>>>>         work for
>>>>         4M superpages, or guests using PSE36.  (I might be acutely
>>>>         aware of
>>>>         pagetable issues, following c/s
>>>>         4c5d78a10dc89).  Furthermore, the map/unmap overhead must be a
>>>>         large
>>>>         overhead.
>>>>
>>>>
>>>>     Indeed it is, that's why in LibVMI there is actually a cache
>>>>     implemented for mapped pages so we throw them away only after they
>>>>     have been idle for a while.
>>>>      
>>>>
>>>>
>>>>         How often is this used by introspection?  To properly walk the
>>>>         pagetables, you need access to the CPUID information (as well
>>>>         as the
>>>>         control register state), and that isn't yet available to the
>>>>         toolstack
>>>>         in Xen.
>>>>
>>>>
>>>>     Control register state is certainly available.
>>>>      
>>>>
>>>>
>>>>         I'm wondering whether it might be better to have a way of
>>>>         asking Xen to
>>>>         perform a virtual to linear translation in the context of a
>>>>         specific
>>>>         vcpu.  My gut feeling is that it might be quicker than running
>>>>         this
>>>>         logic, and is definitely more simple than trying to fix this
>>>>         code not to
>>>>         have vulnerabilities in it.
>>>>
>>>>
>>>>     I don't think it would be necessary. IMHO doing this in Xen
>>>>     wouldn't really net us much performance-wise. Furthermore, there
>>>>     are certain PTE bits that are OS specific and Xen wouldn't
>>>>     necessary have the required information to do the translation
>>>>     properly (ie. present bit unset but transition bits used for
>>>>     Windows guests).
>>>     Ok.  Xen needs to care only about the behaviour of real pointers in
>>>     guest context, and whether they raise #PF.
>>>
>>>     It sounds like the best thing to do is to provide userspace with all
>>>     information it needs to actually perform the walk safely, and let
>>>     the library apply guest-specific knowledge if it wants.
>>>
>>>     Off the top of my head, the control information needed is:
>>>
>>>     Hap/Shadow, hardwares views towards page1gb and pse36, whether
>>>     EFER.NX is leaked from Xen, EFER.NX, CR0.{PG,WP},
>>>     CR4.{PAE,PSE,PCID,SMEP,SMAP,PKE}, and the PDPTRs for the 32bit PAE
>>>     case, because the translation in use isn't necessary the value you
>>>     would read by following CR3 in memory.
>>>
>>>
>>> The userspace already has access to these informations and we use them
>>> in LibVMI already (see
>>> https://github.com/libvmi/libvmi/blob/master/libvmi/memory.c#L37). It's
>>> only this libxc function that has not really been touched in a long time
>>> because I don't think anyone uses it..
>> Should it then be removed altogether, or at least be marked with a
>> #warning or a comment?
> 
> Removing it entirely will break the gdbsx build.
> 
> As its current user is only for debugging, I think this functional fix
> as proposed is fine, as long as it also adds a comment at the top
> indicating that the use of this function is hazardous for your health in
> production scenarios.

Right, so should we then submit V2 with a comment stating this in the
header file?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel