Re: [Xen-devel] [PATCH v8] x86/altp2m: support for setting restrictions for an array of pages

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 12/08/2017 02:18 PM, Jan Beulich wrote:
>>>> On 24.10.17 at 12:19, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
>> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
>> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
>> hence with the original altp2m design, where domains are allowed - with the
>> proper altp2m access rights - to alter these settings), in the absence of an
>> official position on the issue from the original altp2m designers.
> 
> I continue to disagree with this reasoning. I'm afraid I'm not really
> willing to allow widening the badness, unless altp2m was formally
> documented security-unsupported.

Going the DOMCTL route here would have been the (much easier) solution,
and in fact, as stated before, there has been an attempt to do so -
however, IIRC Andrew has insisted that we should take care to use
consistent access privilege across altp2m operations.

This was followed by a lengthy xen-devel discussion and several
unsuccessful attempts to obtain an official position from the original
contributors, at which point (after several months), as also discussed
at the Xen Developer Summit in Budapest, we decided to press on in the
direction that had seemed the most compatible with the original altp2m
design. (Please correct me if I'm misremembering or misunderstanding
something.)

So at this point it looks like we're stuck again: we're happy to go in
any direction the maintainers decide is the best, but we do need to
decide on one.

FWIW, Tamas (CC added) has added code to restrict where altp2m calls can
come from (although that's not XSM code).

Please let us know how to proceed.

>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_t domid,
>> +                                   uint16_t view_id, uint8_t *access,
>> +                                   uint64_t *pages, uint32_t nr)
>> +{
>> +    int rc;
>> +
>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> +    DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
> 
> This is confusing: Why "* sizeof()" in the latter expression, but not
> in the former. It would anyway be better to use sizeof(*pages) (and
> then also sizeof(*access)) to clearly make the connection.

I've opted for less clutter here, since of course sizeof(uint8_t) == 1,
so nr == nr * sizeof(uint8_t). But we're happy to make the change, it
will indeed make the code clearer.

>> @@ -4568,6 +4571,37 @@ 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;
>> +        }
> 
> Just like in a number of other recent cases: This operation should not
> fail when .nr is zero. Yet as per above it will.

We'll test that .nr != 0 before the >= comparison.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel