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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi  Julien,


On 07/06/2016 08:35 PM, Julien Grall wrote:
>
>
> On 06/07/16 17:35, Tamas K Lengyel wrote:
>> 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.
>
> 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,
>

Thank youJulien for your thorough review. We will try to address all of
the points you have mentioned and of course re-test the code before
re-submission. I will also provide a cover letter that will explain the
altp2m functionality in more detail.

Cheers,
~Sergej

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