Re: [PATCH 02/18] arm/altp2m: Add first altp2m HVMOP stubs.

[Tamas K Lengyel <tamas@tklengyel.com>]

On Wed, Jul 6, 2016 at 9:54 AM, Julien Grall <julien.grall@arm.com> wrote:
> (Add x86 maintainers)
>
>
> On 06/07/16 16:23, Tamas K Lengyel wrote:
>>
>> On Wed, Jul 6, 2016 at 7:43 AM, Julien Grall <julien.grall@arm.com> wrote:
>>>
>>> Hello Sergej,
>>>
>>>
>>> On 06/07/16 10:14, Sergej Proskurin wrote:
>>>>
>>>>
>>>> On 07/05/2016 12:19 PM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> Hello Sergej,
>>>>>
>>>>> On 04/07/16 12:45, Sergej Proskurin wrote:
>>>>>>
>>>>>>
>>>>>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>> +{
>>>>>> +    struct xen_hvm_altp2m_op a;
>>>>>> +    struct domain *d = NULL;
>>>>>> +    int rc = 0;
>>>>>> +
>>>>>> +    if ( !hvm_altp2m_supported() )
>>>>>> +        return -EOPNOTSUPP;
>>>>>> +
>>>>>> +    if ( copy_from_guest(&a, arg, 1) )
>>>>>> +        return -EFAULT;
>>>>>> +
>>>>>> +    if ( a.pad1 || a.pad2 ||
>>>>>> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
>>>>>> +         (a.cmd < HVMOP_altp2m_get_domain_state) ||
>>>>>> +         (a.cmd > HVMOP_altp2m_change_gfn) )
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ?
>>>>>> +        rcu_lock_domain_by_any_id(a.domain) :
>>>>>> rcu_lock_current_domain();
>>>>>> +
>>>>>> +    if ( d == NULL )
>>>>>> +        return -ESRCH;
>>>>>> +
>>>>>> +    if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
>>>>>> +         (a.cmd != HVMOP_altp2m_set_domain_state) &&
>>>>>> +         !d->arch.altp2m_active )
>>>>>> +    {
>>>>>> +        rc = -EOPNOTSUPP;
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +
>>>>>> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
>>>>>> +        goto out;
>>>>>
>>>>>
>>>>>
>>>>> I think this is the best place to ask a couple of questions related to
>>>>> who can access altp2m. Based on this call, a guest is allowed to
>>>>> manage its own altp2m. Can you explain why we would want a guest to do
>>>>> that?
>>>>>
>>>>
>>>> On x86, altp2m might be used by the guest in the #VE (Virtualization
>>>> Exception). On ARM, there is indeed not necessary for a guest to access
>>>> altp2m. Could you provide me with information, how to best restrict
>>>> non-privileged guests (not only dom0) from accessing these HVMOPs? Can
>>>> thisbedone by means of xsm? Thank you.
>>>
>>>
>>>
>>> This does not looks safe for both x86 and ARM. From my understanding a
>>> malware would be able to modify an altp2m, switching between 2 view...
>>> which
>>> would lead to remove the entire purpose of altp2m.
>>
>>
>> Well, the whole purpose of the VMFUNC instruction right now is to be
>> able to switch the hypervisor's tables (like altp2m) from within the
>> guest. So yes, if you have a malicious guest then it could control
>> it's own altp2m views. I would assume there are other safe-guards
>> in-place for systems that use this to ensure kernel-integrity and thus
>> prevent arbitrary use of these functions. AFAIK there are only
>> experimental systems based on this so whether it's less or more secure
>> is debatable and likely depend on your implementation.
>
>
> Taken aside the VMFUNC, it looks like insecure to expose a HVMOP to the
> guest which could modify the memaccess attribute of a region.
>
> I thought the whole purpose of VM introspection is to avoid trusting the
> guest (kernel + userspace). The first thing a malware will do is trying to
> do is getting access to the kernel. Once it gets this access, it could
> remove all the memory access restriction to avoid trapping.

That's why I'm saying systems that use this will likely do extra steps
to ensure kernel integrity. In use-cases where this is to be used
exclusively for external monitoring the system can be restricted with
XSM to not allow the guest to issue the hvmops. And remember, on x86
this system is not exclusively used for introspection.

>
>> As for ARM - as there is no hardware features like this available -
>> our goal is to use altp2m in purely external usecases so exposing
>> these ops to the guest is not required. For the first prototype it
>> made sense to mirror the x86 side to reduce the possibility of
>> introducing some bug.
>
>
> No, this is not the right approach. We should not introduce potential
> security issue just because x86 side does it and it "reduces the possibility
> of introducing some bug".
>
> You will have to give me another reason to accept a such patch.

The first revision of a large series is highly unlikely to get
accepted on the first run so we have been working with the assumption
that there will be new revisions. The prototype has been working well
enough for our internal tests to warrant not submitting it as PATCH
RFC. Since this is Sergej's first work with Xen it helped to mirror
the x86 to get him up to speed while working on the prototype and
reducing the complexity he has to keep track of. Now that this phase
is complete the adjustments can be made as required, such as not
exposing these hvmops to ARM guests.

Cheers,
Tamas

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