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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Jul 6, 2016 at 10:29 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 06/07/16 17:05, Tamas K Lengyel wrote:
>>
>> On Wed, Jul 6, 2016 at 9:54 AM, Julien Grall <julien.grall@xxxxxxx> 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.

Tamas

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