Re: [Xen-devel] [PATCH v2 00/25] arm/altp2m: Introducing altp2m to ARM.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

On 08/01/2016 09:55 PM, Julien Grall wrote:
> 
> 
> On 01/08/2016 20:20, Tamas K Lengyel wrote:
>> On Mon, Aug 1, 2016 at 12:15 PM, Julien Grall <julien.grall@xxxxxxx>
>> wrote:
>>> On 01/08/16 18:10, Sergej Proskurin wrote:
>>>>
>>>>
>>>> Hello all,
>>>
>>>
>>> Hello Sergej,
>>>
>>>> The following patch series can be found on Github[0] and is part of my
>>>> contribution to this year's Google Summer of Code (GSoC)[1]. My
>>>> project is
>>>> managed by the organization The Honeynet Project. As part of GSoC, I am
>>>> being
>>>> supervised by the Xen developer Tamas K. Lengyel <tamas@xxxxxxxxxxxxx>,
>>>> George
>>>> D. Webster, and Steven Maresca.
>>>>
>>>> In this patch series, we provide an implementation of the altp2m
>>>> subsystem
>>>> for
>>>> ARM. Our implementation is based on the altp2m subsystem for x86,
>>>> providing
>>>> additional --alternate-- views on the guest's physical memory by
>>>> means of
>>>> the
>>>> ARM 2nd stage translation mechanism. The patches introduce new
>>>> HVMOPs and
>>>> extend the p2m subsystem. Also, we extend libxl to support altp2m on
>>>> ARM
>>>> and
>>>> modify xen-access to test the suggested functionality.
>>>>
>>>> To be more precise, altp2m allows to create and switch to additional
>>>> p2m
>>>> views
>>>> (i.e. gfn to mfn mappings). These views can be manipulated and
>>>> activated
>>>> as
>>>> will through the provided HVMOPs. In this way, the active guest
>>>> instance
>>>> in
>>>> question can seamlessly proceed execution without noticing that
>>>> anything
>>>> has
>>>> changed. The prime scope of application of altp2m is Virtual Machine
>>>> Introspection, where guest systems are analyzed from the outside of the
>>>> VM.
>>>>
>>>> Altp2m can be activated by means of the guest control parameter
>>>> "altp2m"
>>>> on x86
>>>> and ARM architectures.  The altp2m functionality by default can also be
>>>> used
>>>> from within the guest by design. For use-cases requiring purely
>>>> external
>>>> access
>>>> to altp2m, a custom XSM policy is necessary on both x86 and ARM.
>>>
>>>
>>> As said on the previous version, altp2m operation *should not* be
>>> exposed to
>>> ARM guest. Any design written for x86 may not fit exactly for ARM
>>> (and vice
>>> versa), you will need to explain why you think we should follow the same
>>> pattern.
>>>
>>> Speaking about security, I skimmed through the series and noticed
>>> that a lot
>>> of my previous comments have not been addressed. For instance there are
>>> still no locking on the altp2m operations and a guest could disable
>>> altp2m.
>>>
>>> I will give a look to the rest of the series once this is fixed.
>>>
>>
>> Julien,
>> we did discuss whether altp2m on ARM should be exposed to guests or
>> not but we did not agree whether restricting it on ARM is absolutely
>> necessary. Altp2m was designed even on the x86 to be accessible from
>> within the guest on all systems irrespective of actual hardware
>> support for it.  Thus, this design fits ARM as well where there is no
>> dedicated hardware support, from the altp2m perspective there is no
>> difference.
> 
> Really? I looked at the design document [1] which is Intel focus.
> Similar think to the code (see p2m_flush_altp2m in arch/x86/mm/p2m.c).
> 
>> The fact that a guest can disable altp2m by default on
>> itself is *not* a security issue - it is by design. There are usecases
>> where the guest is allowed to use altp2m on itself - like having a
>> security kernel module managing VMM memory permissions from within the
>> guest. For use-cases where such operations are undesirable - like in a
>> purely external usecase - the in-guest operations can be readily
>> restricted by XSM. Thus, it is unreasonable to design a completely
>> separate altp2m mode for ARM when the current design from x86 works
>> just as well.
> 
> x86 does not allow to disable the altp2m feature via setparam. Please
> look at the code (see hvmop_set_param in arch/x86/hvm/hvm.c). It is
> clearly an issue that needs to be fixed for ARM which I already pointed
> out in the previous version.
> 

Thank you for pointing that out. The x86 implementation provides an
explicit filter, controlling which HVM params are allowed to be set by
the guest domain. Thus, the implementation restricts re-setting of the
altp2m HVM param.

We have indeed missed that check. I will provide a fix as part of the
next series.

>>
>> If you consider this design unacceptable for ARM we will need the
>> other Maintainers' input on the topic as well before we proceed. It
>> would be possible to introduce another set of domctl's for altp2m and
>> only implement that for ARM, but unless there is strong consensus
>> about that being absolutely necessary, changing the design of altp2m
>> should be avoided. I certainly see no reason to do that right now.
> 
> I don't see any reason to expose the altp2m feature to the guest
> regardless how x86 does. We diverged on some feature between ARM and x86
> because they do not fit (I have in mind the pirq to event channel code).
> Some of the hypercalls are not implemented by ARM for guest...
> 
>>
>> As for locking on altp2m paths, we haven't encountered any issues with
>> the current version during our tests but it is possible we missed
>> something. If you have spotted missed locking, please point it out.
> 
> I already pointed them out in the previous series. Please read the
> comments I made there.
> 
> Regards,
> 
> [1]
> https://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01319.html
> 

Thanks again for the thorough review.

Best regards,
~Sergej

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