[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.