| Message ID | 20191104041800.24527-8-bharata@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | KVM: PPC: Driver to manage pages of secure guest | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (b9ba205b97bda75388e4014914ae0bdc0022464c) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 166 lines checked |
On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > abort an SVM after it has issued the H_SVM_INIT_START and before the > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > encounters security violations or other errors when starting an SVM. > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > is used by HV to terminate/cleanup an SVM. > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > possibly including its text/data would be stuck in secure memory. > Since the SVM did not go secure, its MSR_S bit will be clear and the > VM wont be able to access its pages even to do a clean exit. It seems fragile to me to have one more transfer back into the ultravisor after this call. Why does the UV need to do this call and then get control back again one more time? Why can't the UV defer doing this call until it can do it without expecting to see a return from the hcall? And if it does need to see a return from the hcall, what would happen if a malicious hypervisor doesn't do the return? Paul.
On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote: > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > > abort an SVM after it has issued the H_SVM_INIT_START and before the > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > > encounters security violations or other errors when starting an SVM. > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > > is used by HV to terminate/cleanup an SVM. > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > > possibly including its text/data would be stuck in secure memory. > > Since the SVM did not go secure, its MSR_S bit will be clear and the > > VM wont be able to access its pages even to do a clean exit. > > It seems fragile to me to have one more transfer back into the > ultravisor after this call. Why does the UV need to do this call and > then get control back again one more time? > Why can't the UV defer > doing this call until it can do it without expecting to see a return > from the hcall? Sure. But, what if the hypervisor calls back into the UV through a ucall, asking for some page to be paged-out? If the ultravisor has cleaned up the state associated with the SVM, it wont be able to service that request. H_SVM_INIT_ABORT is invoked to tell the hypervisor that the secure-state-transition for the VM cannot be continued any further. Hypervisor can than choose to do whatever with that information. It can cleanup its state, and/or make ucalls to get some information from the ultravisor. It can also choose not to return control back to the ultravisor. > And if it does need to see a return from the hcall, > what would happen if a malicious hypervisor doesn't do the return? That is fine. At most it will be a denail-of-service attack. RP > > Paul. If the ultravisor cleans up the SVM's state on its side and then informs the Hypervisor to abort the SVM, the hypervisor will not be able to cleanly terminate the VM. Because to terminate the SVM, the hypervisor still needs the services of the Ultravisor. For example: to get the pages back into the hypervisor if needed. Another example is, the hypervisor can call UV_SVM_TERMINATE. Regardless of which ucall gets called, the ultravisor has to hold on to enough state of the SVM to service that request. The current design assumes that the hypervisor explicitly informs the ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE ucall. Till that point the Ultravisor must to be ready to service any ucalls made by the hypervisor on the SVM's behalf. And if the ultravisor has cleaned-up the state of the SVM on it side, any such ucall requests by the hypervisor will return with error. In summary -- for the hypervisor to cleanly terminate an SVM, it needs the services of the ultravisor. Only the hypervisor knows, when it would NOT anymore need the services of the ultravisor for a SVM. Only after the hypervisor communicates that through the UV_SVM_TERMINATE ucall, the ultravisor will be able to confidently clean the state of the SVM on its side. The H_SVM_INIT_ABORT is a mechanism for the UV to inform the HV to do whatever it needs to do to cleanup its state of the SVM; which includes making ucalls to the ultravisor.
On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote: > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote: > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > > > abort an SVM after it has issued the H_SVM_INIT_START and before the > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > > > encounters security violations or other errors when starting an SVM. > > > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > > > is used by HV to terminate/cleanup an SVM. > > > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > > > possibly including its text/data would be stuck in secure memory. > > > Since the SVM did not go secure, its MSR_S bit will be clear and the > > > VM wont be able to access its pages even to do a clean exit. > > > > It seems fragile to me to have one more transfer back into the > > ultravisor after this call. Why does the UV need to do this call and > > then get control back again one more time? > > Why can't the UV defer > > doing this call until it can do it without expecting to see a return > > from the hcall? > > Sure. But, what if the hypervisor calls back into the UV through a > ucall, asking for some page to be paged-out? If the ultravisor has > cleaned up the state associated with the SVM, it wont be able to service > that request. > > H_SVM_INIT_ABORT is invoked to tell the hypervisor that the > secure-state-transition for the VM cannot be continued any further. > Hypervisor can than choose to do whatever with that information. It can > cleanup its state, and/or make ucalls to get some information from the > ultravisor. It can also choose not to return control back to the ultravisor. > > > > And if it does need to see a return from the hcall, > > what would happen if a malicious hypervisor doesn't do the return? > > That is fine. At most it will be a denail-of-service attack. > > RP > > > > > Paul. > > > > > > If the ultravisor cleans up the SVM's state on its side and then informs > the Hypervisor to abort the SVM, the hypervisor will not be able to > cleanly terminate the VM. Because to terminate the SVM, the hypervisor > still needs the services of the Ultravisor. For example: to get the > pages back into the hypervisor if needed. Another example is, the > hypervisor can call UV_SVM_TERMINATE. Regardless of which ucall > gets called, the ultravisor has to hold on to enough state of the > SVM to service that request. OK, that's a good reason. That should be explained in the commit message. > The current design assumes that the hypervisor explicitly informs the > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE > ucall. Till that point the Ultravisor must to be ready to service any > ucalls made by the hypervisor on the SVM's behalf. I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at which point kvm->arch.secure_guest doesn't matter any more), and in kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared explicitly. Hence I don't see any need for clearing it in the assembly code on the next secure guest entry. I think the change to book3s_hv_rmhandlers.S can just be dropped. Paul.
On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote: > On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote: > > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote: > > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > > > > abort an SVM after it has issued the H_SVM_INIT_START and before the > > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > > > > encounters security violations or other errors when starting an SVM. > > > > > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > > > > is used by HV to terminate/cleanup an SVM. > > > > > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > > > > possibly including its text/data would be stuck in secure memory. > > > > Since the SVM did not go secure, its MSR_S bit will be clear and the > > > > VM wont be able to access its pages even to do a clean exit. > > > ...skip... > > > > If the ultravisor cleans up the SVM's state on its side and then informs > > the Hypervisor to abort the SVM, the hypervisor will not be able to > > cleanly terminate the VM. Because to terminate the SVM, the hypervisor > > still needs the services of the Ultravisor. For example: to get the > > pages back into the hypervisor if needed. Another example is, the > > hypervisor can call UV_SVM_TERMINATE. Regardless of which ucall > > gets called, the ultravisor has to hold on to enough state of the > > SVM to service that request. > > OK, that's a good reason. That should be explained in the commit > message. > > > The current design assumes that the hypervisor explicitly informs the > > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE > > ucall. Till that point the Ultravisor must to be ready to service any > > ucalls made by the hypervisor on the SVM's behalf. > > I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at > which point kvm->arch.secure_guest doesn't matter any more), and in > kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared > explicitly. Hence I don't see any need for clearing it in the > assembly code on the next secure guest entry. I think the change to > book3s_hv_rmhandlers.S can just be dropped. There is subtle problem removing that code from the assembly. If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing kvm->arch.secure_guest, the hypervisor will continue to think that the VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT hcall was invoked, was to inform the Hypervisor that it should no longer consider the VM as a Secure VM. So there is a inconsistency there. This is fine, as long as the VM does not invoke any hcall or does not receive any hypervisor-exceptions. The moment either of those happen, the control goes into the hypervisor, the hypervisor services the exception/hcall and while returning, it will see that the kvm->arch.secure_guest flag is set and **incorrectly** return to the ultravisor through a UV_RETURN ucall. Ultravisor will not know what to do with it, because it does not consider that VM as a Secure VM. Bad things happen. ( Sidenote: when H_SVM_INIT_ABORT hcalls returns from the hypervisor, the ultravisor cleans up its internal state corresponding of that aborted-SVM and returns back to the caller with MSR[S]=0 ) RP
On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote: > > On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote: > > > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote: > > > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > > > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > > > > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > > > > > abort an SVM after it has issued the H_SVM_INIT_START and before the > > > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > > > > > encounters security violations or other errors when starting an SVM. > > > > > > > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > > > > > is used by HV to terminate/cleanup an SVM. > > > > > > > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > > > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > > > > > possibly including its text/data would be stuck in secure memory. > > > > > Since the SVM did not go secure, its MSR_S bit will be clear and the > > > > > VM wont be able to access its pages even to do a clean exit. > > > > > ...skip... > > > > > > If the ultravisor cleans up the SVM's state on its side and then informs > > > the Hypervisor to abort the SVM, the hypervisor will not be able to > > > cleanly terminate the VM. Because to terminate the SVM, the hypervisor > > > still needs the services of the Ultravisor. For example: to get the > > > pages back into the hypervisor if needed. Another example is, the > > > hypervisor can call UV_SVM_TERMINATE. Regardless of which ucall > > > gets called, the ultravisor has to hold on to enough state of the > > > SVM to service that request. > > > > OK, that's a good reason. That should be explained in the commit > > message. > > > > > The current design assumes that the hypervisor explicitly informs the > > > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE > > > ucall. Till that point the Ultravisor must to be ready to service any > > > ucalls made by the hypervisor on the SVM's behalf. > > > > I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at > > which point kvm->arch.secure_guest doesn't matter any more), and in > > kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared > > explicitly. Hence I don't see any need for clearing it in the > > assembly code on the next secure guest entry. I think the change to > > book3s_hv_rmhandlers.S can just be dropped. > > There is subtle problem removing that code from the assembly. > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > kvm->arch.secure_guest, the hypervisor will continue to think that the > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > hcall was invoked, was to inform the Hypervisor that it should no longer > consider the VM as a Secure VM. So there is a inconsistency there. Most of the checks that look at whether a VM is a secure VM use code like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will take the false branch once we have set kvm->arch.secure_guest to KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in most places we will treat the VM as a normal VM from then on. If there are any places where we still need to treat the VM as a secure VM while we are processing the abort we can easily do that too. > This is fine, as long as the VM does not invoke any hcall or does not > receive any hypervisor-exceptions. The moment either of those happen, > the control goes into the hypervisor, the hypervisor services > the exception/hcall and while returning, it will see that the > kvm->arch.secure_guest flag is set and **incorrectly** return > to the ultravisor through a UV_RETURN ucall. Ultravisor will > not know what to do with it, because it does not consider that > VM as a Secure VM. Bad things happen. If bad things happen in the ultravisor because the hypervisor did something it shouldn't, then it's game over, you just lost, thanks for playing. The ultravisor MUST be able to cope with bogus UV_RETURN calls for a VM that it doesn't consider to be a secure VM. You need to work out how to handle such calls safely and implement that in the ultravisor. Paul.
On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > On Tue, Nov 12, 2019 at 04:38:36PM +1100, Paul Mackerras wrote: > > > On Mon, Nov 11, 2019 at 05:01:58PM -0800, Ram Pai wrote: > > > > On Mon, Nov 11, 2019 at 03:19:24PM +1100, Paul Mackerras wrote: > > > > > On Mon, Nov 04, 2019 at 09:47:59AM +0530, Bharata B Rao wrote: > > > > > > From: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > > > > > > > > > Implement the H_SVM_INIT_ABORT hcall which the Ultravisor can use to > > > > > > abort an SVM after it has issued the H_SVM_INIT_START and before the > > > > > > H_SVM_INIT_DONE hcalls. This hcall could be used when Ultravisor > > > > > > encounters security violations or other errors when starting an SVM. > > > > > > > > > > > > Note that this hcall is different from UV_SVM_TERMINATE ucall which > > > > > > is used by HV to terminate/cleanup an SVM. > > > > > > > > > > > > In case of H_SVM_INIT_ABORT, we should page-out all the pages back to > > > > > > HV (i.e., we should not skip the page-out). Otherwise the VM's pages, > > > > > > possibly including its text/data would be stuck in secure memory. > > > > > > Since the SVM did not go secure, its MSR_S bit will be clear and the > > > > > > VM wont be able to access its pages even to do a clean exit. > > > > > > > ...skip... > > > > > > > > If the ultravisor cleans up the SVM's state on its side and then informs > > > > the Hypervisor to abort the SVM, the hypervisor will not be able to > > > > cleanly terminate the VM. Because to terminate the SVM, the hypervisor > > > > still needs the services of the Ultravisor. For example: to get the > > > > pages back into the hypervisor if needed. Another example is, the > > > > hypervisor can call UV_SVM_TERMINATE. Regardless of which ucall > > > > gets called, the ultravisor has to hold on to enough state of the > > > > SVM to service that request. > > > > > > OK, that's a good reason. That should be explained in the commit > > > message. > > > > > > > The current design assumes that the hypervisor explicitly informs the > > > > ultravisor, that it is done with the SVM, through the UV_SVM_TERMINATE > > > > ucall. Till that point the Ultravisor must to be ready to service any > > > > ucalls made by the hypervisor on the SVM's behalf. > > > > > > I see that UV_SVM_TERMINATE is done when the VM is being destroyed (at > > > which point kvm->arch.secure_guest doesn't matter any more), and in > > > kvmhv_svm_off(), where kvm->arch.secure_guest gets cleared > > > explicitly. Hence I don't see any need for clearing it in the > > > assembly code on the next secure guest entry. I think the change to > > > book3s_hv_rmhandlers.S can just be dropped. > > > > There is subtle problem removing that code from the assembly. > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > hcall was invoked, was to inform the Hypervisor that it should no longer > > consider the VM as a Secure VM. So there is a inconsistency there. > > Most of the checks that look at whether a VM is a secure VM use code > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > take the false branch once we have set kvm->arch.secure_guest to > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > most places we will treat the VM as a normal VM from then on. If > there are any places where we still need to treat the VM as a secure > VM while we are processing the abort we can easily do that too. Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back to the Ultravisor? Because removing that assembly code will NOT lead the Hypervisor back into the Ultravisor. This is fine with the ultravisor. But then the hypervisor will not know where to return to. If it wants to return directly to the VM, it wont know to which address. It will be in a limbo. > > > This is fine, as long as the VM does not invoke any hcall or does not > > receive any hypervisor-exceptions. The moment either of those happen, > > the control goes into the hypervisor, the hypervisor services > > the exception/hcall and while returning, it will see that the > > kvm->arch.secure_guest flag is set and **incorrectly** return > > to the ultravisor through a UV_RETURN ucall. Ultravisor will > > not know what to do with it, because it does not consider that > > VM as a Secure VM. Bad things happen. > > If bad things happen in the ultravisor because the hypervisor did > something it shouldn't, then it's game over, you just lost, thanks for > playing. The ultravisor MUST be able to cope with bogus UV_RETURN > calls for a VM that it doesn't consider to be a secure VM. You need > to work out how to handle such calls safely and implement that in the > ultravisor. Actually we do handle this gracefully in the ultravisor :). We just retun back to the hypervisor saying "sorry dont know what to do with it, please handle it yourself". However hypervisor would not know what to do with that return, and bad things happen in the hypervisor. RP
On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > There is subtle problem removing that code from the assembly. > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > Most of the checks that look at whether a VM is a secure VM use code > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > take the false branch once we have set kvm->arch.secure_guest to > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > most places we will treat the VM as a normal VM from then on. If > > there are any places where we still need to treat the VM as a secure > > VM while we are processing the abort we can easily do that too. > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > to the Ultravisor? Because removing that assembly code will NOT lead the No. The suggestion is that vcpu->arch.secure_guest stays set to KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. > Hypervisor back into the Ultravisor. This is fine with the > ultravisor. But then the hypervisor will not know where to return to. > If it wants to return directly to the VM, it wont know to > which address. It will be in a limbo. > > > > > > This is fine, as long as the VM does not invoke any hcall or does not > > > receive any hypervisor-exceptions. The moment either of those happen, > > > the control goes into the hypervisor, the hypervisor services > > > the exception/hcall and while returning, it will see that the > > > kvm->arch.secure_guest flag is set and **incorrectly** return > > > to the ultravisor through a UV_RETURN ucall. Ultravisor will > > > not know what to do with it, because it does not consider that > > > VM as a Secure VM. Bad things happen. > > > > If bad things happen in the ultravisor because the hypervisor did > > something it shouldn't, then it's game over, you just lost, thanks for > > playing. The ultravisor MUST be able to cope with bogus UV_RETURN > > calls for a VM that it doesn't consider to be a secure VM. You need > > to work out how to handle such calls safely and implement that in the > > ultravisor. > > Actually we do handle this gracefully in the ultravisor :). > We just retun back to the hypervisor saying "sorry dont know what > to do with it, please handle it yourself". > > However hypervisor would not know what to do with that return, and bad > things happen in the hypervisor. Right. We need something after the "sc 2" to handle the case where the ultravisor returns with an error from the UV_RETURN. Paul.
On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote: > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > > There is subtle problem removing that code from the assembly. > > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > > > Most of the checks that look at whether a VM is a secure VM use code > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > > take the false branch once we have set kvm->arch.secure_guest to > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > > most places we will treat the VM as a normal VM from then on. If > > > there are any places where we still need to treat the VM as a secure > > > VM while we are processing the abort we can easily do that too. > > > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > > to the Ultravisor? Because removing that assembly code will NOT lead the > > No. The suggestion is that vcpu->arch.secure_guest stays set to > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. In the fast_guest_return path, if it finds (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to UV or not? Ideally it should return back to the ultravisor the first time KVMPPC_SECURE_INIT_ABORT is set, and not than onwards. RP
On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote: > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote: > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > > > There is subtle problem removing that code from the assembly. > > > > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > > > > > Most of the checks that look at whether a VM is a secure VM use code > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > > > take the false branch once we have set kvm->arch.secure_guest to > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > > > most places we will treat the VM as a normal VM from then on. If > > > > there are any places where we still need to treat the VM as a secure > > > > VM while we are processing the abort we can easily do that too. > > > > > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > > > to the Ultravisor? Because removing that assembly code will NOT lead the > > > > No. The suggestion is that vcpu->arch.secure_guest stays set to > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. > > In the fast_guest_return path, if it finds > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to > UV or not? > > Ideally it should return back to the ultravisor the first time > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards. What is ideal about that behavior? Why would that be a particularly good thing to do? Paul.
On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote: > On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote: > > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote: > > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > > > > There is subtle problem removing that code from the assembly. > > > > > > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > > > > > > > Most of the checks that look at whether a VM is a secure VM use code > > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > > > > take the false branch once we have set kvm->arch.secure_guest to > > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > > > > most places we will treat the VM as a normal VM from then on. If > > > > > there are any places where we still need to treat the VM as a secure > > > > > VM while we are processing the abort we can easily do that too. > > > > > > > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > > > > to the Ultravisor? Because removing that assembly code will NOT lead the > > > > > > No. The suggestion is that vcpu->arch.secure_guest stays set to > > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. > > > > In the fast_guest_return path, if it finds > > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to > > UV or not? > > > > Ideally it should return back to the ultravisor the first time > > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards. > > What is ideal about that behavior? Why would that be a particularly > good thing to do? It is following the rule -- "return back to the caller". RP
On Wed, Nov 13, 2019 at 01:50:42PM -0800, Ram Pai wrote: > On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote: > > On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote: > > > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote: > > > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > > > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > > > > > There is subtle problem removing that code from the assembly. > > > > > > > > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > > > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > > > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > > > > > > > > > Most of the checks that look at whether a VM is a secure VM use code > > > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > > > > > take the false branch once we have set kvm->arch.secure_guest to > > > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > > > > > most places we will treat the VM as a normal VM from then on. If > > > > > > there are any places where we still need to treat the VM as a secure > > > > > > VM while we are processing the abort we can easily do that too. > > > > > > > > > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > > > > > to the Ultravisor? Because removing that assembly code will NOT lead the > > > > > > > > No. The suggestion is that vcpu->arch.secure_guest stays set to > > > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. > > > > > > In the fast_guest_return path, if it finds > > > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to > > > UV or not? > > > > > > Ideally it should return back to the ultravisor the first time > > > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards. > > > > What is ideal about that behavior? Why would that be a particularly > > good thing to do? > > It is following the rule -- "return back to the caller". That doesn't address the question of why vcpu->arch.secure_guest should be cleared at the point where we do that. Paul.
On Thu, Nov 14, 2019 at 04:08:25PM +1100, Paul Mackerras wrote: > On Wed, Nov 13, 2019 at 01:50:42PM -0800, Ram Pai wrote: > > On Thu, Nov 14, 2019 at 08:18:24AM +1100, Paul Mackerras wrote: > > > On Tue, Nov 12, 2019 at 10:32:33PM -0800, Ram Pai wrote: > > > > On Wed, Nov 13, 2019 at 11:14:27AM +1100, Paul Mackerras wrote: > > > > > On Tue, Nov 12, 2019 at 06:45:55AM -0800, Ram Pai wrote: > > > > > > On Tue, Nov 12, 2019 at 10:32:04PM +1100, Paul Mackerras wrote: > > > > > > > On Mon, Nov 11, 2019 at 11:52:15PM -0800, Ram Pai wrote: > > > > > > > > There is subtle problem removing that code from the assembly. > > > > > > > > > > > > > > > > If the H_SVM_INIT_ABORT hcall returns to the ultravisor without clearing > > > > > > > > kvm->arch.secure_guest, the hypervisor will continue to think that the > > > > > > > > VM is a secure VM. However the primary reason the H_SVM_INIT_ABORT > > > > > > > > hcall was invoked, was to inform the Hypervisor that it should no longer > > > > > > > > consider the VM as a Secure VM. So there is a inconsistency there. > > > > > > > > > > > > > > Most of the checks that look at whether a VM is a secure VM use code > > > > > > > like "if (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_DONE)". Now > > > > > > > since KVMPPC_SECURE_INIT_ABORT is 4, an if statement such as that will > > > > > > > take the false branch once we have set kvm->arch.secure_guest to > > > > > > > KVMPPC_SECURE_INIT_ABORT in kvmppc_h_svm_init_abort. So in fact in > > > > > > > most places we will treat the VM as a normal VM from then on. If > > > > > > > there are any places where we still need to treat the VM as a secure > > > > > > > VM while we are processing the abort we can easily do that too. > > > > > > > > > > > > Is the suggestion -- KVMPPC_SECURE_INIT_ABORT should never return back > > > > > > to the Ultravisor? Because removing that assembly code will NOT lead the > > > > > > > > > > No. The suggestion is that vcpu->arch.secure_guest stays set to > > > > > KVMPPC_SECURE_INIT_ABORT until userspace calls KVM_PPC_SVM_OFF. > > > > > > > > In the fast_guest_return path, if it finds > > > > (kvm->arch.secure_guest & KVMPPC_SECURE_INIT_ABORT) is true, should it return to > > > > UV or not? > > > > > > > > Ideally it should return back to the ultravisor the first time > > > > KVMPPC_SECURE_INIT_ABORT is set, and not than onwards. > > > > > > What is ideal about that behavior? Why would that be a particularly > > > good thing to do? > > > > It is following the rule -- "return back to the caller". > > That doesn't address the question of why vcpu->arch.secure_guest > should be cleared at the point where we do that. Ok. here is the sequence that I expect to happen. ------------------------------------------------------------------- 1) VM makes a ESM(Enter Secure Mode) ucall. 1A) UV does the H_SVM_INIT_START hcall... the chit-chat between UV and HV happens and somewhere in that chit-chat, UV decides that the ESM call cannot be successfully completed. Hence it calls H_SVM_INIT_ABORT to inform the Hypervisor. 1A_a) Hypervisor aborts the VM's transition and returns back to the ultravisor. 1B) UV cleans up the state of the VM on its side and returns back to the VM, with failure. VM is still in a normal VM state. Its MSR(S) is still 0. 2) VM gets a HDEC exception. 2A) Hypervisor receives that HDEC exception. It handles the exception and returns back to the VM. ------------------------------------------------------------------- If you agree with the above sequence than, the patch needs all the proposed changes. At step (1A_a) and step (2A), the hypervisor is faced with the question -- Where should the control be returned to? for step 1A_a it has to return to the UV, and for step 2A it has to return to the VM. In other words the control has to **return back to the caller**. It certainly has to rely on the vcpu->arch.secure_guest flag to do so. If any flag in vcpu->arch.secure_guest is set, it has to return to UV. Otherwise it has to return to VM. This is the reason, the proposed patch in the fast_guest_return path looks at the vcpu->arch.secure_guest. If it finds any flag set, it returns to UV. However before returning to UV, it also clears all the flags if H_SVM_INIT_ABORT is set. This is to ensure that HV does not return to the wrong place; i.e to UV, but returns to the VM at step 2A. RP
diff --git a/Documentation/powerpc/ultravisor.rst b/Documentation/powerpc/ultravisor.rst index 730854f73830..286cabadc566 100644 --- a/Documentation/powerpc/ultravisor.rst +++ b/Documentation/powerpc/ultravisor.rst @@ -948,6 +948,45 @@ Use cases up its internal state for this virtual machine. +H_SVM_INIT_ABORT +---------------- + + Abort the process of securing an SVM. + +Syntax +~~~~~~ + +.. code-block:: c + + uint64_t hypercall(const uint64_t H_SVM_INIT_ABORT) + +Return values +~~~~~~~~~~~~~ + + One of the following values: + + * H_SUCCESS on success. + * H_UNSUPPORTED if called from the wrong context (e.g. + from an SVM or before an H_SVM_INIT_START + hypercall). + +Description +~~~~~~~~~~~ + + Abort the process of securing a virtual machine. This call must + be made after a prior call to ``H_SVM_INIT_START`` hypercall. + +Use cases +~~~~~~~~~ + + + On successfully securing a virtual machine, the Ultravisor informs + If the Ultravisor is unable to secure a virtual machine either due + to lack of resources or because the VM's security information could + not be validated, Ultravisor informs the Hypervisor about it. + Hypervisor can use this call to clean up any internal state for this + virtual machine. + H_SVM_PAGE_IN ------------- diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h index 13bd870609c3..e90c073e437e 100644 --- a/arch/powerpc/include/asm/hvcall.h +++ b/arch/powerpc/include/asm/hvcall.h @@ -350,6 +350,7 @@ #define H_SVM_PAGE_OUT 0xEF04 #define H_SVM_INIT_START 0xEF08 #define H_SVM_INIT_DONE 0xEF0C +#define H_SVM_INIT_ABORT 0xEF14 /* Values for 2nd argument to H_SET_MODE */ #define H_SET_MODE_RESOURCE_SET_CIABR 1 diff --git a/arch/powerpc/include/asm/kvm_book3s_uvmem.h b/arch/powerpc/include/asm/kvm_book3s_uvmem.h index 3cf8425b9838..eaea400ea715 100644 --- a/arch/powerpc/include/asm/kvm_book3s_uvmem.h +++ b/arch/powerpc/include/asm/kvm_book3s_uvmem.h @@ -18,6 +18,7 @@ unsigned long kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long page_shift); unsigned long kvmppc_h_svm_init_start(struct kvm *kvm); unsigned long kvmppc_h_svm_init_done(struct kvm *kvm); +unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm); int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn); void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, struct kvm *kvm, bool skip_page_out); @@ -62,6 +63,11 @@ static inline unsigned long kvmppc_h_svm_init_done(struct kvm *kvm) return H_UNSUPPORTED; } +static inline unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm) +{ + return H_UNSUPPORTED; +} + static inline int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn) { return -EFAULT; diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h index 577ca95fac7c..8310c0407383 100644 --- a/arch/powerpc/include/asm/kvm_host.h +++ b/arch/powerpc/include/asm/kvm_host.h @@ -278,6 +278,7 @@ struct kvm_resize_hpt; /* Flag values for kvm_arch.secure_guest */ #define KVMPPC_SECURE_INIT_START 0x1 /* H_SVM_INIT_START has been called */ #define KVMPPC_SECURE_INIT_DONE 0x2 /* H_SVM_INIT_DONE completed */ +#define KVMPPC_SECURE_INIT_ABORT 0x4 /* H_SVM_INIT_ABORT issued */ struct kvm_arch { unsigned int lpid; diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index d2bc4e9bbe7e..ad4e38ce7b55 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1099,6 +1099,9 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) case H_SVM_INIT_DONE: ret = kvmppc_h_svm_init_done(vcpu->kvm); break; + case H_SVM_INIT_ABORT: + ret = kvmppc_h_svm_init_abort(vcpu->kvm); + break; default: return RESUME_HOST; diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index faebcbb8c4db..8d192c9947cd 100644 --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S @@ -1112,10 +1112,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300) ld r6, VCPU_KVM(r4) lbz r7, KVM_SECURE_GUEST(r6) cmpdi r7, 0 + bne check_svm_abort + ld r6, VCPU_GPR(R6)(r4) ld r7, VCPU_GPR(R7)(r4) - bne ret_to_ultra - lwz r0, VCPU_CR(r4) mtcr r0 @@ -1125,6 +1125,21 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300) ld r4, VCPU_GPR(R4)(r4) HRFI_TO_GUEST b . + +/* + * If SVM is about to abort, return to UV one last time but clear the + * secure_guest state so future fast_guest_returns return to the normal + * VM. We expect following state and we will restore the state. + * R6 = kvm + * R7 = kvm->secure_guest + */ +check_svm_abort: + + cmpdi r7, 4 /* KVMPPC_SECURE_INIT_ABORT */ + bne ret_to_ultra + li r7, 0 + stb r7, KVM_SECURE_GUEST(r6) + /* * Use UV_RETURN ultracall to return control back to the Ultravisor after * processing an hypercall or interrupt that was forwarded (a.k.a. reflected) @@ -1134,8 +1149,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300) * R0 = hcall result * R2 = SRR1, so UV can detect a synthesized interrupt (if any) * R3 = UV_RETURN + * R6 = kvm (to be restored) + * R7 = kvm->secure_guest (to be restored) */ ret_to_ultra: + ld r6, VCPU_GPR(R6)(r4) + ld r7, VCPU_GPR(R7)(r4) lwz r0, VCPU_CR(r4) mtcr r0 diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index 2df0d3f80c60..627dfe4abf08 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -284,6 +284,35 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free, } } +unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm) +{ + int i; + int srcu_idx; + + if (!(kvm->arch.secure_guest & KVMPPC_SECURE_INIT_START)) + return H_UNSUPPORTED; + + srcu_idx = srcu_read_lock(&kvm->srcu); + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { + struct kvm_memory_slot *memslot; + struct kvm_memslots *slots = __kvm_memslots(kvm, i); + + if (!slots) + continue; + + kvm_for_each_memslot(memslot, slots) { + kvmppc_uvmem_drop_pages(memslot, kvm, false); + uv_unregister_mem_slot(kvm->arch.lpid, memslot->id); + kvmppc_uvmem_slot_free(kvm, memslot); + } + } + srcu_read_unlock(&kvm->srcu, srcu_idx); + + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_ABORT; + pr_info("LPID %d: Switching to secure aborted\n", kvm->arch.lpid); + return H_SUCCESS; +} + /* * Get a free device PFN from the pool *