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

[Tamas K Lengyel <tamas@tklengyel.com>]

On Mon, Dec 11, 2017 at 8:05 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 11.12.17 at 15:51, <george.dunlap@citrix.com> wrote:
>> On 12/11/2017 02:46 PM, Razvan Cojocaru wrote:
>>> On 12/11/2017 03:36 PM, Jan Beulich wrote:
>>>>>>> On 11.12.17 at 13:50, <george.dunlap@citrix.com> wrote:
>>>>> On 12/11/2017 12:12 PM, Jan Beulich wrote:
>>>>>>>>> On 11.12.17 at 12:06, <andrew.cooper3@citrix.com> wrote:
>>>>>>> My suggestion was that we don't break usecases.  The Intel usecase
>>>>>>> specifically is for an in-guest entity to have full control of all
>>>>>>> altp2m functionality, and this is fine (security wise) when permitted to
>>>>>>> do so by the toolstack.
>>>>>>
>>>>>> IOW you mean that such guests would be considered "trusted", i.e.
>>>>>> whatever bad they can do is by definition not a security concern.
>>>>>
>>>>> I'm not sure what you mean by "trusted".  If implemented correctly,
>>>>> altp2m and mem_access shouldn't give the guest any more permissions than
>>>>> it has already.  The main risk would be if there were bugs in the
>>>>> functionality that allowed security issues.
>>>>
>>>> Hmm, maybe I'm mis-reading the code, but
>>>> mem_access.c:set_mem_access() looks to be using the requested
>>>> access rights verbatim, i.e. without applying tool stack imposed
>>>> restrictions (hypervisor ones look to be honored by deriving
>>>> base permissions from the p2m type first).
>>>
>>> Quite likely I'm not grasping the full meaning of your objection,
>>> however the added code is merely another interface to already existing
>>> core code - so while admittedly there's room for improvement for the EPT
>>> code below it, this patch really only extends the scope of altp2m's
>>> existing version of set_mem_access() (which currently works on a single
>>> page). In that, it at least doesn't seem to make things worse (it's
>>> really just an optimization - whatever badness this code can cause with
>>> a single call, can already be achieved exactly with a sequence of
>>> xc_altp2m_set_mem_access() calls).
>>
>> I think Jan was saying that he would ideally like to remove *all* guest
>> access to altp2m functionality, even what's currently there.  The more
>> extra features we make available to guests, the harder it will be in the
>> future to argue to remove it all.
>
> With one slight correction: all _uncontrolled_ access is what I'd like
> to see removed. Right now this could arguably indeed mean all
> access, as it is all uncontrolled (afaict).
>

But it is controlled. Unless you specifically allow the guest access
to the interface (ie altp2m=1 in the xl config) the guest can't do
anything with it. And if you do that, it is likely because you have an
in-guest agent that works in tandem with an out-of-guest agent
coordinating what to protect and how. You use the in-guest agent for
performance-sensitive monitoring while you can use the out-of-guest
agent to protect the in-guest agent. Of course, this is not a
requirement but what I *think* the setup was that the interface was
designed for as there is specific ability to decide which page
permission violation goes to the guest (with #VE) and which goes to
the toolstack. Plus even if the interface is enabled, it is only
available to the guest kernel. If it's a malicious guest kernel the
worst it should be able to do is crash itself (for example by
allocating a ton of altp2m tables and running out of shadow memory).
But I don't think you need the altp2m interface for a guest kernel to
crash itself.

Tamas

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