| Message ID | 20170718152547.14006-2-apop@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote: > From: Vlad Ioan Topan <itopan@bitdefender.com> > > The default value for the "suppress #VE" bit set by set_mem_access() > currently depends on whether the call is made from the same domain (the > bit is set when called from another domain and cleared if called from > the same domain). This patch changes that behavior to inherit the old > suppress #VE bit value if it is already set and to set it to 1 > otherwise, which is safer and more reliable. With the way things are currently if the in-guest tool calls set_mem_access for an altp2m view, it implies it wants to receive #VE for it. Wouldn't this change in this patch effectively make it impossible for an in-guest tool to decide which pages it wants to receive #VE for? The new HVMOP you are introducing is only accessible from a privileged domain.. Tamas
Hello, On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote: > On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote: > > From: Vlad Ioan Topan <itopan@bitdefender.com> > > > > The default value for the "suppress #VE" bit set by set_mem_access() > > currently depends on whether the call is made from the same domain (the > > bit is set when called from another domain and cleared if called from > > the same domain). This patch changes that behavior to inherit the old > > suppress #VE bit value if it is already set and to set it to 1 > > otherwise, which is safer and more reliable. > > With the way things are currently if the in-guest tool calls > set_mem_access for an altp2m view, it implies it wants to receive #VE > for it. Wouldn't this change in this patch effectively make it > impossible for an in-guest tool to decide which pages it wants to > receive #VE for? The new HVMOP you are introducing is only accessible > from a privileged domain.. Yes, this change, along with the restrictions from the new HVMOP would virtually prevent a guest from changing the suppress #VE bit for its pages. The current set_mem_access functionality, if I'm not mistaken, is a bit odd since the guest can only clear the sve, but to set it, another domain would have to call set_mem_access for it. I think the issue would be whether to allow a domain to set/clear the suppress #VE bit for its pages by calling the new HVMOP on itself.
On Wed, Jul 19, 2017 at 5:47 AM, Adrian Pop <apop@bitdefender.com> wrote: > Hello, > > On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote: >> On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote: >> > From: Vlad Ioan Topan <itopan@bitdefender.com> >> > >> > The default value for the "suppress #VE" bit set by set_mem_access() >> > currently depends on whether the call is made from the same domain (the >> > bit is set when called from another domain and cleared if called from >> > the same domain). This patch changes that behavior to inherit the old >> > suppress #VE bit value if it is already set and to set it to 1 >> > otherwise, which is safer and more reliable. >> >> With the way things are currently if the in-guest tool calls >> set_mem_access for an altp2m view, it implies it wants to receive #VE >> for it. Wouldn't this change in this patch effectively make it >> impossible for an in-guest tool to decide which pages it wants to >> receive #VE for? The new HVMOP you are introducing is only accessible >> from a privileged domain.. > > Yes, this change, along with the restrictions from the new HVMOP would > virtually prevent a guest from changing the suppress #VE bit for its > pages. The current set_mem_access functionality, if I'm not mistaken, > is a bit odd since the guest can only clear the sve, but to set it, > another domain would have to call set_mem_access for it. Stating that change explicitly in the patch message would have been something I would want to see. Calling set_mem_access from the guest itself by design clears the SVE bit, which makes sense. The in-guest tool doesn't know whether there is an external mem_access listener, so the only thing it should be allowed to do is to signal to the hypervisor that when it changes EPT permissions, violations on those pages need to be injected into the guest with #VE. If you don't want to allow a domain to make changes like that, you need to restrict altp2m ops to be issued from the domain completely. > > I think the issue would be whether to allow a domain to set/clear the > suppress #VE bit for its pages by calling the new HVMOP on itself. This problem is not limited to setting the SVE bit. It also applies to swapping altp2m views. Pretty much all altp2m HVMOPs can be issued from a user-space program without any way to check whether that process is allowed to do that or not. If you don't think it is safe for a domain to set SVE, the none of the altp2m ops are safe for the domain to issue on itself. If we could say ensure only the kernel can issue the hvmops, that would be OK. But that's not possible at the moment AFAICT. Tamas
On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >> I think the issue would be whether to allow a domain to set/clear the >> suppress #VE bit for its pages by calling the new HVMOP on itself. > > This problem is not limited to setting the SVE bit. It also applies to > swapping altp2m views. Pretty much all altp2m HVMOPs can be issued > from a user-space program without any way to check whether that > process is allowed to do that or not. If you don't think it is safe > for a domain to set SVE, the none of the altp2m ops are safe for the > domain to issue on itself. If we could say ensure only the kernel can > issue the hvmops, that would be OK. But that's not possible at the > moment AFAICT. Wait, is that right? I think we normally restrict hypercalls to only being made from the guest kernel, don't we? -George
On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>> I think the issue would be whether to allow a domain to set/clear the >>> suppress #VE bit for its pages by calling the new HVMOP on itself. >> >> This problem is not limited to setting the SVE bit. It also applies to >> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >> from a user-space program without any way to check whether that >> process is allowed to do that or not. If you don't think it is safe >> for a domain to set SVE, the none of the altp2m ops are safe for the >> domain to issue on itself. If we could say ensure only the kernel can >> issue the hvmops, that would be OK. But that's not possible at the >> moment AFAICT. > > Wait, is that right? I think we normally restrict hypercalls to only > being made from the guest kernel, don't we? > If that's the case then it's good to know (can you point me where that restriction is done?) I was just referring to the fact that technically a userspace program can issue VMCALL. Tamas
On 07/20/2017 05:46 PM, Tamas K Lengyel wrote: > On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap > <George.Dunlap@eu.citrix.com> wrote: >> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>> I think the issue would be whether to allow a domain to set/clear the >>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>> >>> This problem is not limited to setting the SVE bit. It also applies to >>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>> from a user-space program without any way to check whether that >>> process is allowed to do that or not. If you don't think it is safe >>> for a domain to set SVE, the none of the altp2m ops are safe for the >>> domain to issue on itself. If we could say ensure only the kernel can >>> issue the hvmops, that would be OK. But that's not possible at the >>> moment AFAICT. >> >> Wait, is that right? I think we normally restrict hypercalls to only >> being made from the guest kernel, don't we? >> > > If that's the case then it's good to know (can you point me where that > restriction is done?) I was just referring to the fact that > technically a userspace program can issue VMCALL. Well for vmcall in particular, it's in xen/arch/x86/hvm/hypercall/hvm_hypercall(). The check for PV guests is in xen/arch/x86/x86_64/entry.S:lstar_enter. Other checks are left as an exercise for the reader. :-) -George
On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 07/20/2017 05:46 PM, Tamas K Lengyel wrote: >> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap >> <George.Dunlap@eu.citrix.com> wrote: >>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>>> I think the issue would be whether to allow a domain to set/clear the >>>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>>> >>>> This problem is not limited to setting the SVE bit. It also applies to >>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>>> from a user-space program without any way to check whether that >>>> process is allowed to do that or not. If you don't think it is safe >>>> for a domain to set SVE, the none of the altp2m ops are safe for the >>>> domain to issue on itself. If we could say ensure only the kernel can >>>> issue the hvmops, that would be OK. But that's not possible at the >>>> moment AFAICT. >>> >>> Wait, is that right? I think we normally restrict hypercalls to only >>> being made from the guest kernel, don't we? >>> >> >> If that's the case then it's good to know (can you point me where that >> restriction is done?) I was just referring to the fact that >> technically a userspace program can issue VMCALL. > > Well for vmcall in particular, it's in > xen/arch/x86/hvm/hypercall/hvm_hypercall(). The check for PV guests is > in xen/arch/x86/x86_64/entry.S:lstar_enter. Other checks are left as an > exercise for the reader. :-) Thanks ;) I'm looking through it.
On Thu, Jul 20, 2017 at 11:03 AM, Tamas K Lengyel <tamas@tklengyel.com> wrote: > On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap > <george.dunlap@citrix.com> wrote: >> On 07/20/2017 05:46 PM, Tamas K Lengyel wrote: >>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap >>> <George.Dunlap@eu.citrix.com> wrote: >>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>>>> I think the issue would be whether to allow a domain to set/clear the >>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>>>> >>>>> This problem is not limited to setting the SVE bit. It also applies to >>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>>>> from a user-space program without any way to check whether that >>>>> process is allowed to do that or not. If you don't think it is safe >>>>> for a domain to set SVE, the none of the altp2m ops are safe for the >>>>> domain to issue on itself. If we could say ensure only the kernel can >>>>> issue the hvmops, that would be OK. But that's not possible at the >>>>> moment AFAICT. >>>> >>>> Wait, is that right? I think we normally restrict hypercalls to only >>>> being made from the guest kernel, don't we? >>>> >>> >>> If that's the case then it's good to know (can you point me where that >>> restriction is done?) I was just referring to the fact that >>> technically a userspace program can issue VMCALL. >> >> Well for vmcall in particular, it's in >> xen/arch/x86/hvm/hypercall/hvm_hypercall(). The check for PV guests is >> in xen/arch/x86/x86_64/entry.S:lstar_enter. Other checks are left as an >> exercise for the reader. :-) > > Thanks ;) I'm looking through it. All checks out, we can ignore my concerns above. (Some comments around vmx_get_cpl would have been helpful, took me a bit to find in the manual why the bitshift is needed) Tamas
On 07/20/2017 07:46 PM, Tamas K Lengyel wrote: > On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap > <George.Dunlap@eu.citrix.com> wrote: >> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>> I think the issue would be whether to allow a domain to set/clear the >>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>> >>> This problem is not limited to setting the SVE bit. It also applies to >>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>> from a user-space program without any way to check whether that >>> process is allowed to do that or not. If you don't think it is safe >>> for a domain to set SVE, the none of the altp2m ops are safe for the >>> domain to issue on itself. If we could say ensure only the kernel can >>> issue the hvmops, that would be OK. But that's not possible at the >>> moment AFAICT. >> >> Wait, is that right? I think we normally restrict hypercalls to only >> being made from the guest kernel, don't we? >> > > If that's the case then it's good to know (can you point me where that > restriction is done?) I was just referring to the fact that > technically a userspace program can issue VMCALL. It is the case AFAIK. We had to do this trick to allow guest request hypercalls coming from guest userspace: https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch By default they can only come from the guest kernel. Thanks, Razvan
On Thu, Jul 20, 2017 at 12:25 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 07/20/2017 07:46 PM, Tamas K Lengyel wrote: >> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap >> <George.Dunlap@eu.citrix.com> wrote: >>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>>> I think the issue would be whether to allow a domain to set/clear the >>>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>>> >>>> This problem is not limited to setting the SVE bit. It also applies to >>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>>> from a user-space program without any way to check whether that >>>> process is allowed to do that or not. If you don't think it is safe >>>> for a domain to set SVE, the none of the altp2m ops are safe for the >>>> domain to issue on itself. If we could say ensure only the kernel can >>>> issue the hvmops, that would be OK. But that's not possible at the >>>> moment AFAICT. >>> >>> Wait, is that right? I think we normally restrict hypercalls to only >>> being made from the guest kernel, don't we? >>> >> >> If that's the case then it's good to know (can you point me where that >> restriction is done?) I was just referring to the fact that >> technically a userspace program can issue VMCALL. > > It is the case AFAIK. We had to do this trick to allow guest request > hypercalls coming from guest userspace: > > https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch > > By default they can only come from the guest kernel. Makes sense. Any reason not to have that patch upstreamed? Tamas
On 07/20/2017 09:52 PM, Tamas K Lengyel wrote: > On Thu, Jul 20, 2017 at 12:25 PM, Razvan Cojocaru > <rcojocaru@bitdefender.com> wrote: >> On 07/20/2017 07:46 PM, Tamas K Lengyel wrote: >>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap >>> <George.Dunlap@eu.citrix.com> wrote: >>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>>>> I think the issue would be whether to allow a domain to set/clear the >>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself. >>>>> >>>>> This problem is not limited to setting the SVE bit. It also applies to >>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued >>>>> from a user-space program without any way to check whether that >>>>> process is allowed to do that or not. If you don't think it is safe >>>>> for a domain to set SVE, the none of the altp2m ops are safe for the >>>>> domain to issue on itself. If we could say ensure only the kernel can >>>>> issue the hvmops, that would be OK. But that's not possible at the >>>>> moment AFAICT. >>>> >>>> Wait, is that right? I think we normally restrict hypercalls to only >>>> being made from the guest kernel, don't we? >>>> >>> >>> If that's the case then it's good to know (can you point me where that >>> restriction is done?) I was just referring to the fact that >>> technically a userspace program can issue VMCALL. >> >> It is the case AFAIK. We had to do this trick to allow guest request >> hypercalls coming from guest userspace: >> >> https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch >> >> By default they can only come from the guest kernel. > > Makes sense. Any reason not to have that patch upstreamed? It's been rejected: https://patchwork.kernel.org/patch/9264837/ We need that because we inject a cleanup tool in the guest once a threat is detected, and we want to be able to know what it is doing. So every once in a while, the in-guest tool will do a VMCALL that ends up being an event letting the dom0 application know what it's been up to (a communication protocol if you will). We'd be more than happy to retry upstreaming it if I've managed to explain the need for it better today. :) Thanks, Razvan
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 5adaf6df90..d0b0767855 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, } } - return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, - (current->domain != d)); + return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); } static int set_mem_access(struct domain *d, struct p2m_domain *p2m,