Re: [PATCH v4] altp2m: Allow specifying external-only use-case

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Tue, Mar 21, 2017 at 3:54 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.03.17 at 20:27, <tamas.lengyel@zentific.com> wrote:
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I'll need to make this conditional upon a few more adjustments:
>
>> @@ -4370,18 +4370,19 @@ static int do_altp2m_op(
>>          goto out;
>>      }
>>
>> -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
>> +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )
>
> I think it would be better for readers if you compared against
> XEN_ALTP2M_disabled here.
>
>> +    {
>> +        rc = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
>> +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>
> Note the implicit truncation here. Not a problem at present, but
> at the very least I'd like to ask for the function parameter to
> become unsigned int.

Sure.

>
> Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify
> supposed to always be available to the guest (as long as altp2m
> is enabled)? You don't allow this here anymore.

Absolutely not, that's one of the main reasons why I want the
external_only option to be available in the first place. For malware
analysis it is a huge hole if the guest can decide that it wants
certain EPT violations to be handled by the guest without first going
to the hypervisor or if it can start switching its EPT tables around.

>
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -228,8 +228,16 @@
>>  /* Location of the VM Generation ID in guest physical address space. */
>>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>>
>> -/* Boolean: Enable altp2m */
>> +/*
>> + * Set mode for altp2m:
>> + *  disabled: don't activate altp2m (default)
>> + *  mixed: allow access to altp2m for both in-guest and external tools
>> + *  external_only: allow access to external privileged tools only
>
> This needs to be more precise: VMFUNC is an access mechanism too,
> and aiui this isn't meant to be disabled by external_only.

Yes, it is meant to be disabled by external_only, same as with #VE.

>
>> + */
>>  #define HVM_PARAM_ALTP2M       35
>> +#define XEN_ALTP2M_disabled      0
>> +#define XEN_ALTP2M_mixed         1
>> +#define XEN_ALTP2M_external_only 2
>
> I'd drop the _only suffix.

Sure.

>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -555,10 +555,18 @@ static XSM_INLINE int
>> xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
>>      return xsm_default_action(action, current->domain, d);
>>  }
>>
>> -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d)
>> +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, int mode)
>>  {
>> -    XSM_ASSERT_ACTION(XSM_TARGET);
>> -    return xsm_default_action(action, current->domain, d);
>> +    XSM_ASSERT_ACTION(XSM_OTHER);
>> +    switch ( mode )
>> +    {
>> +    case XEN_ALTP2M_mixed:
>> +        return xsm_default_action(XSM_TARGET, current->domain, d);
>> +    case XEN_ALTP2M_external_only:
>> +        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>> +    default:
>> +        return -EPERM;
>
> I think -EPERM is correct at most for XEN_ALTP2M_disabled, all
> others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps
> that also doesn't really belong here, but rather into the caller (which
> right now produces -EINVAL for XEN_ALTP2M_disabled only).
>

The reason I want -EPERM here is so that a malicious guest can't
differentiate between a guest being created with "external_only"
altp2m and one that has an XSM policy that denies these operations.
What you propose would lead to information a leak that would make such
differentiation possible to a malicious guest.

Tamas

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