Re: [Xen-devel] [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.18 at 18:44, <George.Dunlap@xxxxxxxxxx> wrote:

> 
>> On Jul 4, 2018, at 4:38 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> 
>>>>> On 04.07.18 at 16:05, <George.Dunlap@xxxxxxxxxx> wrote:
>>>> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 29.06.18 at 18:39, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>>>>> On 28.06.18 at 15:00, <apop@xxxxxxxxxxxxxxx> wrote:
>>>>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>>>>        }
>>>>>>>        break;
>>>>>>> 
>>>>>>> +    case HVMOP_altp2m_get_mem_access:
>>>>>>> +        if ( a.u.mem_access.pad )
>>>>>>> +            rc = -EINVAL;
>>>>>>> +        else
>>>>>>> +        {
>>>>>>> +            xenmem_access_t access;
>>>>>>> +
>>>>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), 
>>>>>>> &access,
>>>>>>> +                                    a.u.mem_access.view);
>>>>>>> +            if ( !rc )
>>>>>>> +            {
>>>>>>> +                a.u.mem_access.hvmmem_access = access;
>>>>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>>>>> 
>>>>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>>>>> handle of void.
>>>>>> 
>>>>>> And then - here we are again: Is it reasonable to permit a domain 
>>>>>> inquiring
>>>>>> for itself?
>>>>> 
>>>>> A good question. Perhaps the following are decision factors:
>>>>> 
>>>>> 1. It is already possible for a domain to set mem_access restrictions
>>>>> (via HVMOP_altp2m_set_mem_access) on itself.
>>>> 
>>>> Which, as before, I consider a flaw.
>>> 
>>> How many times do we have to go over this?  Here is my recollection from 
>>> the 
> 
>>> last time we had a discussion on this topic:
>>> 
>>> * The original authors of this code probably thought having guests set 
>>> their 
> 
>>> own memaccess would be a potential use case
>>> * The maintainers and main users of the code (Tamas and Razvan) think it’s 
>>> a 
> 
>>> useful use case
>>> * The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a 
>>> useful use case.
>>> 
>>> (Correct me if I’ve misremembered anywhere.)
>>> 
>>> Do we need to have a formal vote by the committers for you to accept that 
>>> this should be a supported use case, and stop making objections any time 
>>> someone wants to improve it?
>> 
>> There's no need for a vote, since - as before - I won't object to the
>> addition, but I consider it to widen the badness (once again).
> 
> But you did object.  This whole thread is you re-objecting to the original 
> decision that we’ve discussed twice before.  Either the altp2m functionality 
> should be exposed to the guest, or it shouldn’t.  If we do expose 
> functionality to the guest, then the interface exposed should be useful; and 
> being able to read what you wrote, rather than keeping a separate copy of it, 
> is part of a useful interface.

No, that's not the right way to put it. If this was an addition to the
interface not having the potential to weaken security, I wouldn't have
re-raised the point of there being an original (apparent, i.e. at least to
me) weakness. 

> I mean, I understand objecting to the idea of building an extension to your 
> house.  But what doesn’t make sense to me is, once the extension is built, 
> then objecting to the idea of painting it; and then objecting to the idea of 
> adding electrical sockets; and then objecting to the idea of adding heat.  
> Why not just accept that you have an extra room and make the best of it?  I 
> understand that you’d prefer extra garden space to the utility room that’s 
> there now, but given that you can’t have the garden, why is a utility room 
> with no paint and no electricity and no heat better than a utility room with 
> all those things?

And this is not a proper analogy either: I'm questioning whether adding
something that increases the risk of the house to crash or burn is a good
idea.

>> In all
>> the "think it's a valid use case" it was never really made clear to me
>> how this "valid" implies "still secure”.
> 
> That was never the argument.  The argument is that the behavior is off by 
> default, and that host administrators should be treated as adults and allowed 
> to judge for themselves whether it’s safe to turn it on or not — just like 
> nested virt, PCI pass-through, COLO, or the host of other features that 
> aren’t security supported.

Even for other security unsupported pieces of code I would always at
least raise concerns if security was further weakened by a change.

> I mean, I’d understand if supporting that use case this meant add tons of 
> extra functionality that was likely to be fragile and introduce new bugs; but 
> it’s not — all the complexity of memaccess would be there even if we only 
> allowed dom0 access to this functionality.
> 
> Would you feel better if we had a line covering memaccess in SUPPORT.md?

That would imo make a difference only if altp2m itself was already
security supported. And anyway - why memaccess? The issue is with the
too broad exposure of altp2m ops in general, irrespective of this not
being the default mode.

Jan


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