|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |