Re: [Xen-devel] [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>> On 09.03.17 at 10:38, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>  
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr 
>> )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> +                                      a.u.set_mem_access_multi.access_list,
>> +                                      a.u.set_mem_access_multi.nr,
>> +                                      a.u.set_mem_access_multi.opaque,
>> +                                      MEMOP_CMD_MASK,
>> +                                      a.u.set_mem_access_multi.view);
>> +        if ( rc > 0 )
>> +        {
>> +            a.u.set_mem_access_multi.opaque = rc;
>> +            if ( __copy_to_guest(arg, &a, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, 
>> "lh",
>> +                                                   HVMOP_altp2m, arg);
>> +        }
>> +        break;
> 
> Okay, so this is a hvmop, in which case I'm fine with the continuation
> model used.
> 
> However - is this interface supposed to be usable by a guest on itself?
> Arguably the same question would apply to some of the other sub-op
> too, but anyway.

Not for any of our use cases. The whole point is for dom0 (or another
suitably privileged domain) to monitor another guest that consequently
can't, by design, evade detection of bad behaviour by acting at a higher
privilege level than the protection software. It wouldn't make sense for
a domain to be doing this on itself.

Maybe Tamas has something in mind, but the short answer is no, it's not.

>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.ht
>> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>>  typedef struct xen_hvm_altp2m_set_mem_access 
>> xen_hvm_altp2m_set_mem_access_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>  
>> +struct xen_hvm_altp2m_set_mem_access_multi {
>> +    /* view */
>> +    uint16_t view;
>> +    uint16_t pad;
>> +    /* Number of pages */
>> +    uint32_t nr;
>> +    /* Used for continuation purposes */
>> +    uint64_t opaque;
>> +    /* List of pfns to set access for */
>> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
>> +    /* Corresponding list of access settings for pfn_list */
>> +    XEN_GUEST_HANDLE(const_uint8) access_list;
> 
> I'm afraid these handles aren't going to work for a 32-bit guest. Note
> how no other hvmop uses handles.

Right, I guess I'll have to pass these through the compat code then,
like the xc_set_mem_access_multi() code does. I'll take a look at what
that entails.


Thanks,
Razvan

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