Re: [PATCH 02/18] arm/altp2m: Add first altp2m HVMOP stubs.

From: Julien Grall <julien.grall@arm.com>
To: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Sergej Proskurin <proskurin@sec.in.tum.de>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <JBeulich@suse.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 02/18] arm/altp2m: Add first altp2m HVMOP stubs.
Date: Wed, 6 Jul 2016 19:35:45 +0100	[thread overview]
Message-ID: <577D4F81.4060706@arm.com> (raw)
In-Reply-To: <CABfawhmHSLXgKEMXmRDd6x3s5x1Swdfgmi2AZTZE6yDACTpjoA@mail.gmail.com>

On 06/07/16 17:35, Tamas K Lengyel wrote:
> On Wed, Jul 6, 2016 at 10:29 AM, Julien Grall <julien.grall@arm.com> wrote:
>>
>>
>> On 06/07/16 17:05, Tamas K Lengyel wrote:
>>>
>>> On Wed, Jul 6, 2016 at 9:54 AM, Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>> Taken aside the VMFUNC, it looks like insecure to expose a HVMOP to the
>>>> guest which could modify the memaccess attribute of a region.
>>>>
>>>> I thought the whole purpose of VM introspection is to avoid trusting the
>>>> guest (kernel + userspace). The first thing a malware will do is trying
>>>> to
>>>> do is getting access to the kernel. Once it gets this access, it could
>>>> remove all the memory access restriction to avoid trapping.
>>>
>>>
>>> That's why I'm saying systems that use this will likely do extra steps
>>> to ensure kernel integrity. In use-cases where this is to be used
>>> exclusively for external monitoring the system can be restricted with
>>> XSM to not allow the guest to issue the hvmops. And remember, on x86
>>> this system is not exclusively used for introspection.
>>
>>
>> I am not aware on how x86 is using alt2pm. And this series didn't give much
>> explanation how this is supposed to work...
>>
>>>>
>>>>> As for ARM - as there is no hardware features like this available -
>>>>> our goal is to use altp2m in purely external usecases so exposing
>>>>> these ops to the guest is not required. For the first prototype it
>>>>> made sense to mirror the x86 side to reduce the possibility of
>>>>> introducing some bug.
>>>>
>>>>
>>>>
>>>> No, this is not the right approach. We should not introduce potential
>>>> security issue just because x86 side does it and it "reduces the
>>>> possibility
>>>> of introducing some bug".
>>>>
>>>> You will have to give me another reason to accept a such patch.
>>>
>>>
>>> The first revision of a large series is highly unlikely to get
>>> accepted on the first run so we have been working with the assumption
>>> that there will be new revisions. The prototype has been working well
>>> enough for our internal tests to warrant not submitting it as PATCH
>>> RFC. Since this is Sergej's first work with Xen it helped to mirror
>>> the x86 to get him up to speed while working on the prototype and
>>> reducing the complexity he has to keep track of. Now that this phase
>>> is complete the adjustments can be made as required, such as not
>>> exposing these hvmops to ARM guests.
>>
>>
>> A such large series is already hard to review, it is even harder when the
>> contributor leaves code unfinished because he assumed there will be a new
>> revision. Actually this is the whole purpose of tagging the series with
>> "RFC". It is used that the series is not in a state where it could
>> potentially be committed.
>>
>
> The code is not in an unfinished state by any means as it passes _our_
> tests and works as expected. I think the assumption we made that there
> will be required adjustments is very reasonable on any patch series.
> So I'm not sure what the problem is.

Well, I am bit surprised that this series is passing your tests (you may 
want to update them?). Before sending a new version, I would recommend 
to go through the locking of each path. I tried to comment on every 
locking issue I have spotted, although I may have miss some.

I would also recommend you to go through the ARM ARM to see how the TLBs 
behaves because switching between page tables with the same VMID could 
be harmful if not doing correctly. I have mentioned that you could use 
different VMID for the page table, however this may have impact on other 
part of the memory such as the cache. (this would need to be investigated).

I know it is not an easy task. The P2M code is complex, so it would 
benefit for all the reviewers to have an explanation in the cover letter 
how this is supposed to work.

Regards,

-- 
Julien Grall

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