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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jul 5, 2018 at 2:31 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> 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's comment here about the too broad exposure is not without a
point. For a security application to point in using altp2m and
memaccess is to have memory protections that the guest can't alter, so
even if the guest kernel gets compromised, some security checks remain
in place. Otherwise using the in-guest pagetables would be better in
every way - faster, less complexity, etc. #VE and VMFUNC blur this
picture a bit by allowing a guest agent to filter some of the events
that result from EPT violations. This may already be seen as
"weakening" the security of the approach but IMHO that's just the
tradeoff between security and performance. It can be configured on
which page can the in-guest agent act as a filter and which pages go
to the external agent, so at least that tradeoff can be fine-tuned (in
theory). So with that in mind, I would certainly consider an in-guest
application less of a security concern if it was only able to filter
events for which it was explicitly permitted to do so instead of being
able to both see all page permissions and also setting (ie removing
them) at will. The current altp2m interface is certainly not suitable
for making such a fine-grained distinction, and XSM doesn't help with
this either. But that's a separate problem, once we allow a guest
kernel to set these permissions, also allowing it get them makes very
little difference. Perhaps what we should be discussing is splitting
altp2m hvmop into two ops, one that's required for EPTP switching and
receiving #VE events, and one that adds the "rest" in case it's needed
during development/testing.

Tamas

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