* [PATCH 0/2] Fix SVM hang at startup @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc This series is fixing a SVM hang occurring when starting a SVM requiring more secure memory than available. The hang happens in the SVM when calling UV_ESM. The following is happening: 1. SVM calls UV_ESM 2. Ultravisor (UV) calls H_SVM_INIT_START 3. Hypervisor (HV) calls UV_REGISTER_MEM_SLOT 4. UV returns error because there is not enough free secure memory 5. HV enter the error path in kvmppc_h_svm_init_start() 6. In the return path, since kvm->arch.secure_guest is not yet set hrfid is called 7. As the HV doesn't know the SVM calling context hrfid is jumping to unknown address in the SVM leading to various expections. This series fixes the setting of kvm->arch.secure_guest in kvmppc_h_svm_init_start() to ensure that UV_RETURN is called on the return path to get back to the UV. In addition to ensure that a malicious VM will not call UV reserved Hcall, a check of the Secure bit in the calling MSR is addded to reject such a call. It is assumed that the UV will filtered out such Hcalls made by a malicious SVM. Laurent Dufour (2): KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++---------- arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- 2 files changed, 23 insertions(+), 12 deletions(-) -- 2.25.2 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] Fix SVM hang at startup @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc This series is fixing a SVM hang occurring when starting a SVM requiring more secure memory than available. The hang happens in the SVM when calling UV_ESM. The following is happening: 1. SVM calls UV_ESM 2. Ultravisor (UV) calls H_SVM_INIT_START 3. Hypervisor (HV) calls UV_REGISTER_MEM_SLOT 4. UV returns error because there is not enough free secure memory 5. HV enter the error path in kvmppc_h_svm_init_start() 6. In the return path, since kvm->arch.secure_guest is not yet set hrfid is called 7. As the HV doesn't know the SVM calling context hrfid is jumping to unknown address in the SVM leading to various expections. This series fixes the setting of kvm->arch.secure_guest in kvmppc_h_svm_init_start() to ensure that UV_RETURN is called on the return path to get back to the UV. In addition to ensure that a malicious VM will not call UV reserved Hcall, a check of the Secure bit in the calling MSR is addded to reject such a call. It is assumed that the UV will filtered out such Hcalls made by a malicious SVM. Laurent Dufour (2): KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++---------- arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- 2 files changed, 23 insertions(+), 12 deletions(-) -- 2.25.2 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-20 10:26 ` Laurent Dufour -1 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing prevent a malicious VM or SVM to call them. This could lead to weird result and should be filtered out. Checking the Secure bit of the calling MSR ensure that the call is coming from either the Ultravisor or a SVM. But any system call made from a SVM are going through the Ultravisor, and the Ultravisor should filter out these malicious call. This way, only the Ultravisor is able to make such a Hcall. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 33be4d93248a..43773182a737 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_IN: - ret = kvmppc_h_svm_page_in(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_in(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_OUT: - ret = kvmppc_h_svm_page_out(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_out(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_INIT_START: - ret = kvmppc_h_svm_init_start(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_start(vcpu->kvm); break; case H_SVM_INIT_DONE: - ret = kvmppc_h_svm_init_done(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_done(vcpu->kvm); break; case H_SVM_INIT_ABORT: - ret = kvmppc_h_svm_init_abort(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_abort(vcpu->kvm); break; default: -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing prevent a malicious VM or SVM to call them. This could lead to weird result and should be filtered out. Checking the Secure bit of the calling MSR ensure that the call is coming from either the Ultravisor or a SVM. But any system call made from a SVM are going through the Ultravisor, and the Ultravisor should filter out these malicious call. This way, only the Ultravisor is able to make such a Hcall. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 33be4d93248a..43773182a737 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_IN: - ret = kvmppc_h_svm_page_in(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_in(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_OUT: - ret = kvmppc_h_svm_page_out(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_out(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_INIT_START: - ret = kvmppc_h_svm_init_start(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_start(vcpu->kvm); break; case H_SVM_INIT_DONE: - ret = kvmppc_h_svm_init_done(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_done(vcpu->kvm); break; case H_SVM_INIT_ABORT: - ret = kvmppc_h_svm_init_abort(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_abort(vcpu->kvm); break; default: -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc; +Cc: Bharata B Rao The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing prevent a malicious VM or SVM to call them. This could lead to weird result and should be filtered out. Checking the Secure bit of the calling MSR ensure that the call is coming from either the Ultravisor or a SVM. But any system call made from a SVM are going through the Ultravisor, and the Ultravisor should filter out these malicious call. This way, only the Ultravisor is able to make such a Hcall. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index 33be4d93248a..43773182a737 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_IN: - ret = kvmppc_h_svm_page_in(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_in(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_PAGE_OUT: - ret = kvmppc_h_svm_page_out(vcpu->kvm, - kvmppc_get_gpr(vcpu, 4), - kvmppc_get_gpr(vcpu, 5), - kvmppc_get_gpr(vcpu, 6)); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_page_out(vcpu->kvm, + kvmppc_get_gpr(vcpu, 4), + kvmppc_get_gpr(vcpu, 5), + kvmppc_get_gpr(vcpu, 6)); break; case H_SVM_INIT_START: - ret = kvmppc_h_svm_init_start(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_start(vcpu->kvm); break; case H_SVM_INIT_DONE: - ret = kvmppc_h_svm_init_done(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_done(vcpu->kvm); break; case H_SVM_INIT_ABORT: - ret = kvmppc_h_svm_init_abort(vcpu->kvm); + ret = H_UNSUPPORTED; + if (kvmppc_get_srr1(vcpu) & MSR_S) + ret = kvmppc_h_svm_init_abort(vcpu->kvm); break; default: -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-20 12:22 ` Greg Kurz -1 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-20 12:22 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, 20 Mar 2020 11:26:42 +0100 Laurent Dufour <ldufour@linux.ibm.com> wrote: > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > prevent a malicious VM or SVM to call them. This could lead to weird result > and should be filtered out. > > Checking the Secure bit of the calling MSR ensure that the call is coming > from either the Ultravisor or a SVM. But any system call made from a SVM > are going through the Ultravisor, and the Ultravisor should filter out > these malicious call. This way, only the Ultravisor is able to make such a > Hcall. "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? Shouldn't we also check the HV bit of the calling MSR as well to disambiguate SVM and UV ? > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 33be4d93248a..43773182a737 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_PAGE_IN: > - ret = kvmppc_h_svm_page_in(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_in(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); If calling kvmppc_h_svm_page_in() produces a "weird result" when the MSR_S bit isn't set, then I think it should do the checking itself, ie. pass vcpu. This would also prevent adding that many lines in kvmppc_pseries_do_hcall() which is a big enough function already. The checking could be done in a helper in book3s_hv_uvmem.c and used by all UV specific hcalls. > break; > case H_SVM_PAGE_OUT: > - ret = kvmppc_h_svm_page_out(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_out(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_INIT_START: > - ret = kvmppc_h_svm_init_start(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_start(vcpu->kvm); > break; > case H_SVM_INIT_DONE: > - ret = kvmppc_h_svm_init_done(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_abort(vcpu->kvm); > break; > > default: ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 12:22 ` Greg Kurz 0 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-20 12:22 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, 20 Mar 2020 11:26:42 +0100 Laurent Dufour <ldufour@linux.ibm.com> wrote: > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > prevent a malicious VM or SVM to call them. This could lead to weird result > and should be filtered out. > > Checking the Secure bit of the calling MSR ensure that the call is coming > from either the Ultravisor or a SVM. But any system call made from a SVM > are going through the Ultravisor, and the Ultravisor should filter out > these malicious call. This way, only the Ultravisor is able to make such a > Hcall. "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? Shouldn't we also check the HV bit of the calling MSR as well to disambiguate SVM and UV ? > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 33be4d93248a..43773182a737 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_PAGE_IN: > - ret = kvmppc_h_svm_page_in(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_in(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); If calling kvmppc_h_svm_page_in() produces a "weird result" when the MSR_S bit isn't set, then I think it should do the checking itself, ie. pass vcpu. This would also prevent adding that many lines in kvmppc_pseries_do_hcall() which is a big enough function already. The checking could be done in a helper in book3s_hv_uvmem.c and used by all UV specific hcalls. > break; > case H_SVM_PAGE_OUT: > - ret = kvmppc_h_svm_page_out(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_out(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_INIT_START: > - ret = kvmppc_h_svm_init_start(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_start(vcpu->kvm); > break; > case H_SVM_INIT_DONE: > - ret = kvmppc_h_svm_init_done(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_abort(vcpu->kvm); > break; > > default: ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 12:22 ` Greg Kurz 0 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-20 12:22 UTC (permalink / raw) To: Laurent Dufour; +Cc: linux-kernel, kvm-ppc, Bharata B Rao, linuxppc-dev On Fri, 20 Mar 2020 11:26:42 +0100 Laurent Dufour <ldufour@linux.ibm.com> wrote: > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > prevent a malicious VM or SVM to call them. This could lead to weird result > and should be filtered out. > > Checking the Secure bit of the calling MSR ensure that the call is coming > from either the Ultravisor or a SVM. But any system call made from a SVM > are going through the Ultravisor, and the Ultravisor should filter out > these malicious call. This way, only the Ultravisor is able to make such a > Hcall. "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? Shouldn't we also check the HV bit of the calling MSR as well to disambiguate SVM and UV ? > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index 33be4d93248a..43773182a737 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) > kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_PAGE_IN: > - ret = kvmppc_h_svm_page_in(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_in(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); If calling kvmppc_h_svm_page_in() produces a "weird result" when the MSR_S bit isn't set, then I think it should do the checking itself, ie. pass vcpu. This would also prevent adding that many lines in kvmppc_pseries_do_hcall() which is a big enough function already. The checking could be done in a helper in book3s_hv_uvmem.c and used by all UV specific hcalls. > break; > case H_SVM_PAGE_OUT: > - ret = kvmppc_h_svm_page_out(vcpu->kvm, > - kvmppc_get_gpr(vcpu, 4), > - kvmppc_get_gpr(vcpu, 5), > - kvmppc_get_gpr(vcpu, 6)); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_page_out(vcpu->kvm, > + kvmppc_get_gpr(vcpu, 4), > + kvmppc_get_gpr(vcpu, 5), > + kvmppc_get_gpr(vcpu, 6)); > break; > case H_SVM_INIT_START: > - ret = kvmppc_h_svm_init_start(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_start(vcpu->kvm); > break; > case H_SVM_INIT_DONE: > - ret = kvmppc_h_svm_init_done(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_done(vcpu->kvm); > break; > case H_SVM_INIT_ABORT: > - ret = kvmppc_h_svm_init_abort(vcpu->kvm); > + ret = H_UNSUPPORTED; > + if (kvmppc_get_srr1(vcpu) & MSR_S) > + ret = kvmppc_h_svm_init_abort(vcpu->kvm); > break; > > default: ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-20 12:22 ` Greg Kurz (?) @ 2020-03-20 14:43 ` Laurent Dufour -1 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:43 UTC (permalink / raw) To: Greg Kurz Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Le 20/03/2020 à 13:22, Greg Kurz a écrit : > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > >> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >> prevent a malicious VM or SVM to call them. This could lead to weird result >> and should be filtered out. >> >> Checking the Secure bit of the calling MSR ensure that the call is coming >> from either the Ultravisor or a SVM. But any system call made from a SVM >> are going through the Ultravisor, and the Ultravisor should filter out >> these malicious call. This way, only the Ultravisor is able to make such a >> Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? If it doesn't, a malicious SVM would be able to call UV reserved Hcall like H_SVM_INIT_ABORT, etc... which is not a good idea. > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? That's another way to do so, but since the SVM Hcall are going through the UV, it seems the right place (the UV) to do the filtering. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- >> 1 file changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 33be4d93248a..43773182a737 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) >> kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_PAGE_IN: >> - ret = kvmppc_h_svm_page_in(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_in(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); > > If calling kvmppc_h_svm_page_in() produces a "weird result" when > the MSR_S bit isn't set, then I think it should do the checking > itself, ie. pass vcpu. > > This would also prevent adding that many lines in kvmppc_pseries_do_hcall() > which is a big enough function already. The checking could be done in a > helper in book3s_hv_uvmem.c and used by all UV specific hcalls. I'm not convinced that would be better, and I followed the way checks for other Hcalls has been made (see H_TLB_INVALIDATE,..). I agree kvmppc_pseries_do_hcall() is long but this is just a big switch(), quite linear. > >> break; >> case H_SVM_PAGE_OUT: >> - ret = kvmppc_h_svm_page_out(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_out(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_INIT_START: >> - ret = kvmppc_h_svm_init_start(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_start(vcpu->kvm); >> break; >> case H_SVM_INIT_DONE: >> - ret = kvmppc_h_svm_init_done(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_done(vcpu->kvm); >> break; >> case H_SVM_INIT_ABORT: >> - ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> break; >> >> default: > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 14:43 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:43 UTC (permalink / raw) To: Greg Kurz Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Le 20/03/2020 à 13:22, Greg Kurz a écrit : > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > >> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >> prevent a malicious VM or SVM to call them. This could lead to weird result >> and should be filtered out. >> >> Checking the Secure bit of the calling MSR ensure that the call is coming >> from either the Ultravisor or a SVM. But any system call made from a SVM >> are going through the Ultravisor, and the Ultravisor should filter out >> these malicious call. This way, only the Ultravisor is able to make such a >> Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? If it doesn't, a malicious SVM would be able to call UV reserved Hcall like H_SVM_INIT_ABORT, etc... which is not a good idea. > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? That's another way to do so, but since the SVM Hcall are going through the UV, it seems the right place (the UV) to do the filtering. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- >> 1 file changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 33be4d93248a..43773182a737 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) >> kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_PAGE_IN: >> - ret = kvmppc_h_svm_page_in(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_in(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); > > If calling kvmppc_h_svm_page_in() produces a "weird result" when > the MSR_S bit isn't set, then I think it should do the checking > itself, ie. pass vcpu. > > This would also prevent adding that many lines in kvmppc_pseries_do_hcall() > which is a big enough function already. The checking could be done in a > helper in book3s_hv_uvmem.c and used by all UV specific hcalls. I'm not convinced that would be better, and I followed the way checks for other Hcalls has been made (see H_TLB_INVALIDATE,..). I agree kvmppc_pseries_do_hcall() is long but this is just a big switch(), quite linear. > >> break; >> case H_SVM_PAGE_OUT: >> - ret = kvmppc_h_svm_page_out(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_out(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_INIT_START: >> - ret = kvmppc_h_svm_init_start(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_start(vcpu->kvm); >> break; >> case H_SVM_INIT_DONE: >> - ret = kvmppc_h_svm_init_done(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_done(vcpu->kvm); >> break; >> case H_SVM_INIT_ABORT: >> - ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> break; >> >> default: > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-20 14:43 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:43 UTC (permalink / raw) To: Greg Kurz; +Cc: linux-kernel, kvm-ppc, Bharata B Rao, linuxppc-dev Le 20/03/2020 à 13:22, Greg Kurz a écrit : > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > >> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >> prevent a malicious VM or SVM to call them. This could lead to weird result >> and should be filtered out. >> >> Checking the Secure bit of the calling MSR ensure that the call is coming >> from either the Ultravisor or a SVM. But any system call made from a SVM >> are going through the Ultravisor, and the Ultravisor should filter out >> these malicious call. This way, only the Ultravisor is able to make such a >> Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? If it doesn't, a malicious SVM would be able to call UV reserved Hcall like H_SVM_INIT_ABORT, etc... which is not a good idea. > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? That's another way to do so, but since the SVM Hcall are going through the UV, it seems the right place (the UV) to do the filtering. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- >> 1 file changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 33be4d93248a..43773182a737 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -1074,25 +1074,35 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu) >> kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_PAGE_IN: >> - ret = kvmppc_h_svm_page_in(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_in(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); > > If calling kvmppc_h_svm_page_in() produces a "weird result" when > the MSR_S bit isn't set, then I think it should do the checking > itself, ie. pass vcpu. > > This would also prevent adding that many lines in kvmppc_pseries_do_hcall() > which is a big enough function already. The checking could be done in a > helper in book3s_hv_uvmem.c and used by all UV specific hcalls. I'm not convinced that would be better, and I followed the way checks for other Hcalls has been made (see H_TLB_INVALIDATE,..). I agree kvmppc_pseries_do_hcall() is long but this is just a big switch(), quite linear. > >> break; >> case H_SVM_PAGE_OUT: >> - ret = kvmppc_h_svm_page_out(vcpu->kvm, >> - kvmppc_get_gpr(vcpu, 4), >> - kvmppc_get_gpr(vcpu, 5), >> - kvmppc_get_gpr(vcpu, 6)); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_page_out(vcpu->kvm, >> + kvmppc_get_gpr(vcpu, 4), >> + kvmppc_get_gpr(vcpu, 5), >> + kvmppc_get_gpr(vcpu, 6)); >> break; >> case H_SVM_INIT_START: >> - ret = kvmppc_h_svm_init_start(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_start(vcpu->kvm); >> break; >> case H_SVM_INIT_DONE: >> - ret = kvmppc_h_svm_init_done(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_done(vcpu->kvm); >> break; >> case H_SVM_INIT_ABORT: >> - ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> + ret = H_UNSUPPORTED; >> + if (kvmppc_get_srr1(vcpu) & MSR_S) >> + ret = kvmppc_h_svm_init_abort(vcpu->kvm); >> break; >> >> default: > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-20 12:22 ` Greg Kurz (?) @ 2020-03-23 23:43 ` Paul Mackerras -1 siblings, 0 replies; 42+ messages in thread From: Paul Mackerras @ 2020-03-23 23:43 UTC (permalink / raw) To: Greg Kurz Cc: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > prevent a malicious VM or SVM to call them. This could lead to weird result > > and should be filtered out. > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > from either the Ultravisor or a SVM. But any system call made from a SVM > > are going through the Ultravisor, and the Ultravisor should filter out > > these malicious call. This way, only the Ultravisor is able to make such a > > Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? The trouble with doing that (checking the HV bit) is that KVM does not expect to see the HV bit set on an interrupt that occurred while we were in the guest, and if it is set, it indicates a serious problem, i.e. that an interrupt occurred while we were in the code that transitions from host context to guest context, or from guest context to host context. In those cases we don't know how much of the transition has been completed and therefore whether we have guest values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). If we do see HV set then KVM reports a severe error to userspace which should cause userspace to terminate the guest. Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 when transitioning to KVM. Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-23 23:43 ` Paul Mackerras 0 siblings, 0 replies; 42+ messages in thread From: Paul Mackerras @ 2020-03-23 23:43 UTC (permalink / raw) To: Greg Kurz Cc: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > prevent a malicious VM or SVM to call them. This could lead to weird result > > and should be filtered out. > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > from either the Ultravisor or a SVM. But any system call made from a SVM > > are going through the Ultravisor, and the Ultravisor should filter out > > these malicious call. This way, only the Ultravisor is able to make such a > > Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? The trouble with doing that (checking the HV bit) is that KVM does not expect to see the HV bit set on an interrupt that occurred while we were in the guest, and if it is set, it indicates a serious problem, i.e. that an interrupt occurred while we were in the code that transitions from host context to guest context, or from guest context to host context. In those cases we don't know how much of the transition has been completed and therefore whether we have guest values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). If we do see HV set then KVM reports a severe error to userspace which should cause userspace to terminate the guest. Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 when transitioning to KVM. Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-23 23:43 ` Paul Mackerras 0 siblings, 0 replies; 42+ messages in thread From: Paul Mackerras @ 2020-03-23 23:43 UTC (permalink / raw) To: Greg Kurz Cc: linux-kernel, kvm-ppc, Bharata B Rao, Laurent Dufour, linuxppc-dev On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > On Fri, 20 Mar 2020 11:26:42 +0100 > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > prevent a malicious VM or SVM to call them. This could lead to weird result > > and should be filtered out. > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > from either the Ultravisor or a SVM. But any system call made from a SVM > > are going through the Ultravisor, and the Ultravisor should filter out > > these malicious call. This way, only the Ultravisor is able to make such a > > Hcall. > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > Shouldn't we also check the HV bit of the calling MSR as well to > disambiguate SVM and UV ? The trouble with doing that (checking the HV bit) is that KVM does not expect to see the HV bit set on an interrupt that occurred while we were in the guest, and if it is set, it indicates a serious problem, i.e. that an interrupt occurred while we were in the code that transitions from host context to guest context, or from guest context to host context. In those cases we don't know how much of the transition has been completed and therefore whether we have guest values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). If we do see HV set then KVM reports a severe error to userspace which should cause userspace to terminate the guest. Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 when transitioning to KVM. Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-23 23:43 ` Paul Mackerras (?) @ 2020-03-24 12:00 ` Greg Kurz -1 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-24 12:00 UTC (permalink / raw) To: Paul Mackerras Cc: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman On Tue, 24 Mar 2020 10:43:23 +1100 Paul Mackerras <paulus@ozlabs.org> wrote: > On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > > On Fri, 20 Mar 2020 11:26:42 +0100 > > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > > prevent a malicious VM or SVM to call them. This could lead to weird result > > > and should be filtered out. > > > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > > from either the Ultravisor or a SVM. But any system call made from a SVM > > > are going through the Ultravisor, and the Ultravisor should filter out > > > these malicious call. This way, only the Ultravisor is able to make such a > > > Hcall. > > > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > > > Shouldn't we also check the HV bit of the calling MSR as well to > > disambiguate SVM and UV ? > > The trouble with doing that (checking the HV bit) is that KVM does not > expect to see the HV bit set on an interrupt that occurred while we > were in the guest, and if it is set, it indicates a serious problem, > i.e. that an interrupt occurred while we were in the code that > transitions from host context to guest context, or from guest context > to host context. In those cases we don't know how much of the > transition has been completed and therefore whether we have guest > values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). > If we do see HV set then KVM reports a severe error to userspace which > should cause userspace to terminate the guest. > > Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 > when transitioning to KVM. > Indeed... thanks for the clarification. So I guess we'll just assume that the UV doesn't reflect these SVM specific hcalls if they happened to be issued by the guest then. Cheers, -- Greg > Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-24 12:00 ` Greg Kurz 0 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-24 12:00 UTC (permalink / raw) To: Paul Mackerras Cc: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman On Tue, 24 Mar 2020 10:43:23 +1100 Paul Mackerras <paulus@ozlabs.org> wrote: > On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > > On Fri, 20 Mar 2020 11:26:42 +0100 > > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > > prevent a malicious VM or SVM to call them. This could lead to weird result > > > and should be filtered out. > > > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > > from either the Ultravisor or a SVM. But any system call made from a SVM > > > are going through the Ultravisor, and the Ultravisor should filter out > > > these malicious call. This way, only the Ultravisor is able to make such a > > > Hcall. > > > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > > > Shouldn't we also check the HV bit of the calling MSR as well to > > disambiguate SVM and UV ? > > The trouble with doing that (checking the HV bit) is that KVM does not > expect to see the HV bit set on an interrupt that occurred while we > were in the guest, and if it is set, it indicates a serious problem, > i.e. that an interrupt occurred while we were in the code that > transitions from host context to guest context, or from guest context > to host context. In those cases we don't know how much of the > transition has been completed and therefore whether we have guest > values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). > If we do see HV set then KVM reports a severe error to userspace which > should cause userspace to terminate the guest. > > Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 > when transitioning to KVM. > Indeed... thanks for the clarification. So I guess we'll just assume that the UV doesn't reflect these SVM specific hcalls if they happened to be issued by the guest then. Cheers, -- Greg > Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-24 12:00 ` Greg Kurz 0 siblings, 0 replies; 42+ messages in thread From: Greg Kurz @ 2020-03-24 12:00 UTC (permalink / raw) To: Paul Mackerras Cc: linux-kernel, kvm-ppc, Bharata B Rao, Laurent Dufour, linuxppc-dev On Tue, 24 Mar 2020 10:43:23 +1100 Paul Mackerras <paulus@ozlabs.org> wrote: > On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: > > On Fri, 20 Mar 2020 11:26:42 +0100 > > Laurent Dufour <ldufour@linux.ibm.com> wrote: > > > > > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > > > prevent a malicious VM or SVM to call them. This could lead to weird result > > > and should be filtered out. > > > > > > Checking the Secure bit of the calling MSR ensure that the call is coming > > > from either the Ultravisor or a SVM. But any system call made from a SVM > > > are going through the Ultravisor, and the Ultravisor should filter out > > > these malicious call. This way, only the Ultravisor is able to make such a > > > Hcall. > > > > "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? > > > > Shouldn't we also check the HV bit of the calling MSR as well to > > disambiguate SVM and UV ? > > The trouble with doing that (checking the HV bit) is that KVM does not > expect to see the HV bit set on an interrupt that occurred while we > were in the guest, and if it is set, it indicates a serious problem, > i.e. that an interrupt occurred while we were in the code that > transitions from host context to guest context, or from guest context > to host context. In those cases we don't know how much of the > transition has been completed and therefore whether we have guest > values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). > If we do see HV set then KVM reports a severe error to userspace which > should cause userspace to terminate the guest. > > Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 > when transitioning to KVM. > Indeed... thanks for the clarification. So I guess we'll just assume that the UV doesn't reflect these SVM specific hcalls if they happened to be issued by the guest then. Cheers, -- Greg > Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-24 12:00 ` Greg Kurz (?) @ 2020-03-24 13:13 ` Laurent Dufour -1 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-24 13:13 UTC (permalink / raw) To: Greg Kurz, Paul Mackerras Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman Le 24/03/2020 à 13:00, Greg Kurz a écrit : > On Tue, 24 Mar 2020 10:43:23 +1100 > Paul Mackerras <paulus@ozlabs.org> wrote: > >> On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: >>> On Fri, 20 Mar 2020 11:26:42 +0100 >>> Laurent Dufour <ldufour@linux.ibm.com> wrote: >>> >>>> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >>>> prevent a malicious VM or SVM to call them. This could lead to weird result >>>> and should be filtered out. >>>> >>>> Checking the Secure bit of the calling MSR ensure that the call is coming >>>> from either the Ultravisor or a SVM. But any system call made from a SVM >>>> are going through the Ultravisor, and the Ultravisor should filter out >>>> these malicious call. This way, only the Ultravisor is able to make such a >>>> Hcall. >>> >>> "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? >>> >>> Shouldn't we also check the HV bit of the calling MSR as well to >>> disambiguate SVM and UV ? >> >> The trouble with doing that (checking the HV bit) is that KVM does not >> expect to see the HV bit set on an interrupt that occurred while we >> were in the guest, and if it is set, it indicates a serious problem, >> i.e. that an interrupt occurred while we were in the code that >> transitions from host context to guest context, or from guest context >> to host context. In those cases we don't know how much of the >> transition has been completed and therefore whether we have guest >> values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). >> If we do see HV set then KVM reports a severe error to userspace which >> should cause userspace to terminate the guest. >> >> Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 >> when transitioning to KVM. >> > > Indeed... thanks for the clarification. So I guess we'll just assume > that the UV doesn't reflect these SVM specific hcalls if they happened > to be issued by the guest then. As mentioned in the series's description: "It is assumed that the UV will filtered out such Hcalls made by a malicious SVM." > Cheers, > > -- > Greg > >> Paul. > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-24 13:13 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-24 13:13 UTC (permalink / raw) To: Greg Kurz, Paul Mackerras Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao, Benjamin Herrenschmidt, Michael Ellerman Le 24/03/2020 à 13:00, Greg Kurz a écrit : > On Tue, 24 Mar 2020 10:43:23 +1100 > Paul Mackerras <paulus@ozlabs.org> wrote: > >> On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: >>> On Fri, 20 Mar 2020 11:26:42 +0100 >>> Laurent Dufour <ldufour@linux.ibm.com> wrote: >>> >>>> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >>>> prevent a malicious VM or SVM to call them. This could lead to weird result >>>> and should be filtered out. >>>> >>>> Checking the Secure bit of the calling MSR ensure that the call is coming >>>> from either the Ultravisor or a SVM. But any system call made from a SVM >>>> are going through the Ultravisor, and the Ultravisor should filter out >>>> these malicious call. This way, only the Ultravisor is able to make such a >>>> Hcall. >>> >>> "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? >>> >>> Shouldn't we also check the HV bit of the calling MSR as well to >>> disambiguate SVM and UV ? >> >> The trouble with doing that (checking the HV bit) is that KVM does not >> expect to see the HV bit set on an interrupt that occurred while we >> were in the guest, and if it is set, it indicates a serious problem, >> i.e. that an interrupt occurred while we were in the code that >> transitions from host context to guest context, or from guest context >> to host context. In those cases we don't know how much of the >> transition has been completed and therefore whether we have guest >> values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). >> If we do see HV set then KVM reports a severe error to userspace which >> should cause userspace to terminate the guest. >> >> Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 >> when transitioning to KVM. >> > > Indeed... thanks for the clarification. So I guess we'll just assume > that the UV doesn't reflect these SVM specific hcalls if they happened > to be issued by the guest then. As mentioned in the series's description: "It is assumed that the UV will filtered out such Hcalls made by a malicious SVM." > Cheers, > > -- > Greg > >> Paul. > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-24 13:13 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-24 13:13 UTC (permalink / raw) To: Greg Kurz, Paul Mackerras Cc: linux-kernel, kvm-ppc, Bharata B Rao, linuxppc-dev Le 24/03/2020 à 13:00, Greg Kurz a écrit : > On Tue, 24 Mar 2020 10:43:23 +1100 > Paul Mackerras <paulus@ozlabs.org> wrote: > >> On Fri, Mar 20, 2020 at 01:22:48PM +0100, Greg Kurz wrote: >>> On Fri, 20 Mar 2020 11:26:42 +0100 >>> Laurent Dufour <ldufour@linux.ibm.com> wrote: >>> >>>> The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing >>>> prevent a malicious VM or SVM to call them. This could lead to weird result >>>> and should be filtered out. >>>> >>>> Checking the Secure bit of the calling MSR ensure that the call is coming >>>> from either the Ultravisor or a SVM. But any system call made from a SVM >>>> are going through the Ultravisor, and the Ultravisor should filter out >>>> these malicious call. This way, only the Ultravisor is able to make such a >>>> Hcall. >>> >>> "Ultravisor should filter" ? And what if it doesn't (eg. because of a bug) ? >>> >>> Shouldn't we also check the HV bit of the calling MSR as well to >>> disambiguate SVM and UV ? >> >> The trouble with doing that (checking the HV bit) is that KVM does not >> expect to see the HV bit set on an interrupt that occurred while we >> were in the guest, and if it is set, it indicates a serious problem, >> i.e. that an interrupt occurred while we were in the code that >> transitions from host context to guest context, or from guest context >> to host context. In those cases we don't know how much of the >> transition has been completed and therefore whether we have guest >> values or host values in the CPU registers (GPRs, FPRs/VSRs, SPRs). >> If we do see HV set then KVM reports a severe error to userspace which >> should cause userspace to terminate the guest. >> >> Therefore the UV should *always* have the HV bit clear in HSRR1/SRR1 >> when transitioning to KVM. >> > > Indeed... thanks for the clarification. So I guess we'll just assume > that the UV doesn't reflect these SVM specific hcalls if they happened > to be issued by the guest then. As mentioned in the series's description: "It is assumed that the UV will filtered out such Hcalls made by a malicious SVM." > Cheers, > > -- > Greg > >> Paul. > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls 2020-03-20 10:26 ` Laurent Dufour @ 2020-03-21 0:40 ` Ram Pai -1 siblings, 0 replies; 42+ messages in thread From: Ram Pai @ 2020-03-21 0:40 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao On Fri, Mar 20, 2020 at 11:26:42AM +0100, Laurent Dufour wrote: > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > prevent a malicious VM or SVM to call them. This could lead to weird result > and should be filtered out. > > Checking the Secure bit of the calling MSR ensure that the call is coming > from either the Ultravisor or a SVM. But any system call made from a SVM > are going through the Ultravisor, and the Ultravisor should filter out > these malicious call. This way, only the Ultravisor is able to make such a > Hcall. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibnm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls @ 2020-03-21 0:40 ` Ram Pai 0 siblings, 0 replies; 42+ messages in thread From: Ram Pai @ 2020-03-21 0:40 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao On Fri, Mar 20, 2020 at 11:26:42AM +0100, Laurent Dufour wrote: > The Hcall named H_SVM_* are reserved to the Ultravisor. However, nothing > prevent a malicious VM or SVM to call them. This could lead to weird result > and should be filtered out. > > Checking the Secure bit of the calling MSR ensure that the call is coming > from either the Ultravisor or a SVM. But any system call made from a SVM > are going through the Ultravisor, and the Ultravisor should filter out > these malicious call. This way, only the Ultravisor is able to make such a > Hcall. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibnm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-20 10:26 ` Laurent Dufour -1 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman When the call to UV_REGISTER_MEM_SLOT is failing, for instance because there is not enough free secured memory, the Hypervisor (HV) has to call UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. If the kvm->arch.secure_guest is not set, in the return path rfid is called but there is no valid context to get back to the SVM since the Hcall has been routed by the Ultravisor. Move the setting of kvm->arch.secure_guest earlier in kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called instead of rfid. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index 79b1202b1c62..68dff151315c 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) int ret = H_SUCCESS; int srcu_idx; + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; + if (!kvmppc_uvmem_bitmap) return H_UNSUPPORTED; @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) goto out; } } - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; out: srcu_read_unlock(&kvm->srcu, srcu_idx); return ret; -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman When the call to UV_REGISTER_MEM_SLOT is failing, for instance because there is not enough free secured memory, the Hypervisor (HV) has to call UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. If the kvm->arch.secure_guest is not set, in the return path rfid is called but there is no valid context to get back to the SVM since the Hcall has been routed by the Ultravisor. Move the setting of kvm->arch.secure_guest earlier in kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called instead of rfid. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index 79b1202b1c62..68dff151315c 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) int ret = H_SUCCESS; int srcu_idx; + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; + if (!kvmppc_uvmem_bitmap) return H_UNSUPPORTED; @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) goto out; } } - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; out: srcu_read_unlock(&kvm->srcu, srcu_idx); return ret; -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 10:26 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 10:26 UTC (permalink / raw) To: linuxppc-dev, linux-kernel, kvm-ppc; +Cc: Bharata B Rao When the call to UV_REGISTER_MEM_SLOT is failing, for instance because there is not enough free secured memory, the Hypervisor (HV) has to call UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. If the kvm->arch.secure_guest is not set, in the return path rfid is called but there is no valid context to get back to the SVM since the Hcall has been routed by the Ultravisor. Move the setting of kvm->arch.secure_guest earlier in kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called instead of rfid. Cc: Bharata B Rao <bharata@linux.ibm.com> Cc: Paul Mackerras <paulus@ozlabs.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c index 79b1202b1c62..68dff151315c 100644 --- a/arch/powerpc/kvm/book3s_hv_uvmem.c +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) int ret = H_SUCCESS; int srcu_idx; + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; + if (!kvmppc_uvmem_bitmap) return H_UNSUPPORTED; @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) goto out; } } - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; out: srcu_read_unlock(&kvm->srcu, srcu_idx); return ret; -- 2.25.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-20 11:24 ` Bharata B Rao -1 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-20 11:24 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; There is an assumption that memory slots would have been registered with UV if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START hasn't been done. Have you checked if that path isn't affected by this change? Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 11:24 ` Bharata B Rao 0 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-20 11:36 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; There is an assumption that memory slots would have been registered with UV if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START hasn't been done. Have you checked if that path isn't affected by this change? Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 11:24 ` Bharata B Rao 0 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-20 11:24 UTC (permalink / raw) To: Laurent Dufour; +Cc: linux-kernel, kvm-ppc, linuxppc-dev On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; There is an assumption that memory slots would have been registered with UV if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START hasn't been done. Have you checked if that path isn't affected by this change? Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 11:24 ` Bharata B Rao (?) @ 2020-03-20 14:36 ` Laurent Dufour -1 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:36 UTC (permalink / raw) To: bharata Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: >> When the call to UV_REGISTER_MEM_SLOT is failing, for instance because >> there is not enough free secured memory, the Hypervisor (HV) has to call >> UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call >> H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. >> >> If the kvm->arch.secure_guest is not set, in the return path rfid is called >> but there is no valid context to get back to the SVM since the Hcall has >> been routed by the Ultravisor. >> >> Move the setting of kvm->arch.secure_guest earlier in >> kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called >> instead of rfid. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c >> index 79b1202b1c62..68dff151315c 100644 >> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c >> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c >> @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> int ret = H_SUCCESS; >> int srcu_idx; >> >> + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; >> + >> if (!kvmppc_uvmem_bitmap) >> return H_UNSUPPORTED; >> >> @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> goto out; >> } >> } >> - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > There is an assumption that memory slots would have been registered with UV > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > hasn't been done. > > Have you checked if that path isn't affected by this change? I checked that and didn't find any issue there. My only concern was that block: kvm_for_each_vcpu(i, vcpu, kvm) { spin_lock(&vcpu->arch.vpa_update_lock); unpin_vpa_reset(kvm, &vcpu->arch.dtl); unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); unpin_vpa_reset(kvm, &vcpu->arch.vpa); spin_unlock(&vcpu->arch.vpa_update_lock); } But that seems to be safe. However I'm not a familiar with the KVM's code, do you think an additional KVMPPC_SECURE_INIT_* value needed here? Thanks, Laurent. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 14:36 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:36 UTC (permalink / raw) To: bharata Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: >> When the call to UV_REGISTER_MEM_SLOT is failing, for instance because >> there is not enough free secured memory, the Hypervisor (HV) has to call >> UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call >> H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. >> >> If the kvm->arch.secure_guest is not set, in the return path rfid is called >> but there is no valid context to get back to the SVM since the Hcall has >> been routed by the Ultravisor. >> >> Move the setting of kvm->arch.secure_guest earlier in >> kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called >> instead of rfid. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c >> index 79b1202b1c62..68dff151315c 100644 >> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c >> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c >> @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> int ret = H_SUCCESS; >> int srcu_idx; >> >> + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; >> + >> if (!kvmppc_uvmem_bitmap) >> return H_UNSUPPORTED; >> >> @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> goto out; >> } >> } >> - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > There is an assumption that memory slots would have been registered with UV > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > hasn't been done. > > Have you checked if that path isn't affected by this change? I checked that and didn't find any issue there. My only concern was that block: kvm_for_each_vcpu(i, vcpu, kvm) { spin_lock(&vcpu->arch.vpa_update_lock); unpin_vpa_reset(kvm, &vcpu->arch.dtl); unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); unpin_vpa_reset(kvm, &vcpu->arch.vpa); spin_unlock(&vcpu->arch.vpa_update_lock); } But that seems to be safe. However I'm not a familiar with the KVM's code, do you think an additional KVMPPC_SECURE_INIT_* value needed here? Thanks, Laurent. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-20 14:36 ` Laurent Dufour 0 siblings, 0 replies; 42+ messages in thread From: Laurent Dufour @ 2020-03-20 14:36 UTC (permalink / raw) To: bharata; +Cc: linux-kernel, kvm-ppc, linuxppc-dev Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: >> When the call to UV_REGISTER_MEM_SLOT is failing, for instance because >> there is not enough free secured memory, the Hypervisor (HV) has to call >> UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call >> H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. >> >> If the kvm->arch.secure_guest is not set, in the return path rfid is called >> but there is no valid context to get back to the SVM since the Hcall has >> been routed by the Ultravisor. >> >> Move the setting of kvm->arch.secure_guest earlier in >> kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called >> instead of rfid. >> >> Cc: Bharata B Rao <bharata@linux.ibm.com> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c >> index 79b1202b1c62..68dff151315c 100644 >> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c >> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c >> @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> int ret = H_SUCCESS; >> int srcu_idx; >> >> + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; >> + >> if (!kvmppc_uvmem_bitmap) >> return H_UNSUPPORTED; >> >> @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) >> goto out; >> } >> } >> - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > There is an assumption that memory slots would have been registered with UV > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > hasn't been done. > > Have you checked if that path isn't affected by this change? I checked that and didn't find any issue there. My only concern was that block: kvm_for_each_vcpu(i, vcpu, kvm) { spin_lock(&vcpu->arch.vpa_update_lock); unpin_vpa_reset(kvm, &vcpu->arch.dtl); unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); unpin_vpa_reset(kvm, &vcpu->arch.vpa); spin_unlock(&vcpu->arch.vpa_update_lock); } But that seems to be safe. However I'm not a familiar with the KVM's code, do you think an additional KVMPPC_SECURE_INIT_* value needed here? Thanks, Laurent. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 14:36 ` Laurent Dufour (?) @ 2020-03-23 4:21 ` Bharata B Rao -1 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-23 4:21 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 03:36:05PM +0100, Laurent Dufour wrote: > Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > > > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > > > there is not enough free secured memory, the Hypervisor (HV) has to call > > > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > > > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > > > > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > > > but there is no valid context to get back to the SVM since the Hcall has > > > been routed by the Ultravisor. > > > > > > Move the setting of kvm->arch.secure_guest earlier in > > > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > > > instead of rfid. > > > > > > Cc: Bharata B Rao <bharata@linux.ibm.com> > > > Cc: Paul Mackerras <paulus@ozlabs.org> > > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > > > --- > > > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > index 79b1202b1c62..68dff151315c 100644 > > > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > > > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > int ret = H_SUCCESS; > > > int srcu_idx; > > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > > > + > > > if (!kvmppc_uvmem_bitmap) > > > return H_UNSUPPORTED; > > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > goto out; > > > } > > > } > > > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > > > There is an assumption that memory slots would have been registered with UV > > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > > hasn't been done. > > > > Have you checked if that path isn't affected by this change? > > I checked that and didn't find any issue there. > > My only concern was that block: > kvm_for_each_vcpu(i, vcpu, kvm) { > spin_lock(&vcpu->arch.vpa_update_lock); > unpin_vpa_reset(kvm, &vcpu->arch.dtl); > unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); > unpin_vpa_reset(kvm, &vcpu->arch.vpa); > spin_unlock(&vcpu->arch.vpa_update_lock); > } > > But that seems to be safe. Yes, looks like. > > However I'm not a familiar with the KVM's code, do you think an additional > KVMPPC_SECURE_INIT_* value needed here? May be not as long as UV can handle the unexpected uv_unregister_mem_slot() calls, we are good. Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-23 4:21 ` Bharata B Rao 0 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-23 4:33 UTC (permalink / raw) To: Laurent Dufour Cc: linuxppc-dev, linux-kernel, kvm-ppc, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman On Fri, Mar 20, 2020 at 03:36:05PM +0100, Laurent Dufour wrote: > Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > > > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > > > there is not enough free secured memory, the Hypervisor (HV) has to call > > > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > > > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > > > > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > > > but there is no valid context to get back to the SVM since the Hcall has > > > been routed by the Ultravisor. > > > > > > Move the setting of kvm->arch.secure_guest earlier in > > > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > > > instead of rfid. > > > > > > Cc: Bharata B Rao <bharata@linux.ibm.com> > > > Cc: Paul Mackerras <paulus@ozlabs.org> > > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > > > --- > > > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > index 79b1202b1c62..68dff151315c 100644 > > > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > > > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > int ret = H_SUCCESS; > > > int srcu_idx; > > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > > > + > > > if (!kvmppc_uvmem_bitmap) > > > return H_UNSUPPORTED; > > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > goto out; > > > } > > > } > > > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > > > There is an assumption that memory slots would have been registered with UV > > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > > hasn't been done. > > > > Have you checked if that path isn't affected by this change? > > I checked that and didn't find any issue there. > > My only concern was that block: > kvm_for_each_vcpu(i, vcpu, kvm) { > spin_lock(&vcpu->arch.vpa_update_lock); > unpin_vpa_reset(kvm, &vcpu->arch.dtl); > unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); > unpin_vpa_reset(kvm, &vcpu->arch.vpa); > spin_unlock(&vcpu->arch.vpa_update_lock); > } > > But that seems to be safe. Yes, looks like. > > However I'm not a familiar with the KVM's code, do you think an additional > KVMPPC_SECURE_INIT_* value needed here? May be not as long as UV can handle the unexpected uv_unregister_mem_slot() calls, we are good. Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-23 4:21 ` Bharata B Rao 0 siblings, 0 replies; 42+ messages in thread From: Bharata B Rao @ 2020-03-23 4:21 UTC (permalink / raw) To: Laurent Dufour; +Cc: linux-kernel, kvm-ppc, linuxppc-dev On Fri, Mar 20, 2020 at 03:36:05PM +0100, Laurent Dufour wrote: > Le 20/03/2020 à 12:24, Bharata B Rao a écrit : > > On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > > > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > > > there is not enough free secured memory, the Hypervisor (HV) has to call > > > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > > > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > > > > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > > > but there is no valid context to get back to the SVM since the Hcall has > > > been routed by the Ultravisor. > > > > > > Move the setting of kvm->arch.secure_guest earlier in > > > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > > > instead of rfid. > > > > > > Cc: Bharata B Rao <bharata@linux.ibm.com> > > > Cc: Paul Mackerras <paulus@ozlabs.org> > > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > Cc: Michael Ellerman <mpe@ellerman.id.au> > > > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > > > --- > > > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > index 79b1202b1c62..68dff151315c 100644 > > > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > > > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > > > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > int ret = H_SUCCESS; > > > int srcu_idx; > > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > > > + > > > if (!kvmppc_uvmem_bitmap) > > > return H_UNSUPPORTED; > > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > > > goto out; > > > } > > > } > > > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > > > > There is an assumption that memory slots would have been registered with UV > > if KVMPPC_SECURE_INIT_START has been done. KVM_PPC_SVM_OFF ioctl will skip > > unregistration and other steps during reboot if KVMPPC_SECURE_INIT_START > > hasn't been done. > > > > Have you checked if that path isn't affected by this change? > > I checked that and didn't find any issue there. > > My only concern was that block: > kvm_for_each_vcpu(i, vcpu, kvm) { > spin_lock(&vcpu->arch.vpa_update_lock); > unpin_vpa_reset(kvm, &vcpu->arch.dtl); > unpin_vpa_reset(kvm, &vcpu->arch.slb_shadow); > unpin_vpa_reset(kvm, &vcpu->arch.vpa); > spin_unlock(&vcpu->arch.vpa_update_lock); > } > > But that seems to be safe. Yes, looks like. > > However I'm not a familiar with the KVM's code, do you think an additional > KVMPPC_SECURE_INIT_* value needed here? May be not as long as UV can handle the unexpected uv_unregister_mem_slot() calls, we are good. Regards, Bharata. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-21 0:47 ` Ram Pai -1 siblings, 0 replies; 42+ messages in thread From: Ram Pai @ 2020-03-21 0:47 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call secure memory, > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibm.com> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-21 0:47 ` Ram Pai 0 siblings, 0 replies; 42+ messages in thread From: Ram Pai @ 2020-03-21 0:47 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call secure memory, > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibm.com> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-21 0:47 ` Ram Pai 0 siblings, 0 replies; 42+ messages in thread From: Ram Pai @ 2020-03-21 0:47 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc, Bharata B Rao On Fri, Mar 20, 2020 at 11:26:43AM +0100, Laurent Dufour wrote: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call secure memory, > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> Reviewed-by: Ram Pai <linuxram@us.ibm.com> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN 2020-03-20 10:26 ` Laurent Dufour (?) @ 2020-03-23 14:09 ` Fabiano Rosas -1 siblings, 0 replies; 42+ messages in thread From: Fabiano Rosas @ 2020-03-23 14:09 UTC (permalink / raw) To: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Laurent Dufour <ldufour@linux.ibm.com> writes: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- I tested this along with the code that rejects the secure transition when it is not enabled in KVM. I have also forced a KVM_PPC_SVM_OFF (via system_reset) right after setting kvm->arch.secure_guest and nothing bad happened. Tested-by: Fabiano Rosas <farosas@linux.ibm.com> > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > out: > srcu_read_unlock(&kvm->srcu, srcu_idx); > return ret; ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-23 14:09 ` Fabiano Rosas 0 siblings, 0 replies; 42+ messages in thread From: Fabiano Rosas @ 2020-03-23 14:09 UTC (permalink / raw) To: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc Cc: Bharata B Rao, Paul Mackerras, Benjamin Herrenschmidt, Michael Ellerman Laurent Dufour <ldufour@linux.ibm.com> writes: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- I tested this along with the code that rejects the secure transition when it is not enabled in KVM. I have also forced a KVM_PPC_SVM_OFF (via system_reset) right after setting kvm->arch.secure_guest and nothing bad happened. Tested-by: Fabiano Rosas <farosas@linux.ibm.com> > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > out: > srcu_read_unlock(&kvm->srcu, srcu_idx); > return ret; ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN @ 2020-03-23 14:09 ` Fabiano Rosas 0 siblings, 0 replies; 42+ messages in thread From: Fabiano Rosas @ 2020-03-23 14:09 UTC (permalink / raw) To: Laurent Dufour, linuxppc-dev, linux-kernel, kvm-ppc; +Cc: Bharata B Rao Laurent Dufour <ldufour@linux.ibm.com> writes: > When the call to UV_REGISTER_MEM_SLOT is failing, for instance because > there is not enough free secured memory, the Hypervisor (HV) has to call > UV_RETURN to report the error to the Ultravisor (UV). Then the UV will call > H_SVM_INIT_ABORT to abort the securing phase and go back to the calling VM. > > If the kvm->arch.secure_guest is not set, in the return path rfid is called > but there is no valid context to get back to the SVM since the Hcall has > been routed by the Ultravisor. > > Move the setting of kvm->arch.secure_guest earlier in > kvmppc_h_svm_init_start() so in the return path, UV_RETURN will be called > instead of rfid. > > Cc: Bharata B Rao <bharata@linux.ibm.com> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- I tested this along with the code that rejects the secure transition when it is not enabled in KVM. I have also forced a KVM_PPC_SVM_OFF (via system_reset) right after setting kvm->arch.secure_guest and nothing bad happened. Tested-by: Fabiano Rosas <farosas@linux.ibm.com> > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c > index 79b1202b1c62..68dff151315c 100644 > --- a/arch/powerpc/kvm/book3s_hv_uvmem.c > +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c > @@ -209,6 +209,8 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > int ret = H_SUCCESS; > int srcu_idx; > > + kvm->arch.secure_guest = KVMPPC_SECURE_INIT_START; > + > if (!kvmppc_uvmem_bitmap) > return H_UNSUPPORTED; > > @@ -233,7 +235,6 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm) > goto out; > } > } > - kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_START; > out: > srcu_read_unlock(&kvm->srcu, srcu_idx); > return ret; ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Fix SVM hang at startup 2020-03-20 10:26 ` Laurent Dufour @ 2020-03-24 2:54 ` Paul Mackerras -1 siblings, 0 replies; 42+ messages in thread From: Paul Mackerras @ 2020-03-24 2:54 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc On Fri, Mar 20, 2020 at 11:26:41AM +0100, Laurent Dufour wrote: > This series is fixing a SVM hang occurring when starting a SVM requiring > more secure memory than available. The hang happens in the SVM when calling > UV_ESM. > > The following is happening: > > 1. SVM calls UV_ESM > 2. Ultravisor (UV) calls H_SVM_INIT_START > 3. Hypervisor (HV) calls UV_REGISTER_MEM_SLOT > 4. UV returns error because there is not enough free secure memory > 5. HV enter the error path in kvmppc_h_svm_init_start() > 6. In the return path, since kvm->arch.secure_guest is not yet set hrfid is > called > 7. As the HV doesn't know the SVM calling context hrfid is jumping to > unknown address in the SVM leading to various expections. > > This series fixes the setting of kvm->arch.secure_guest in > kvmppc_h_svm_init_start() to ensure that UV_RETURN is called on the return > path to get back to the UV. > > In addition to ensure that a malicious VM will not call UV reserved Hcall, > a check of the Secure bit in the calling MSR is addded to reject such a > call. > > It is assumed that the UV will filtered out such Hcalls made by a malicious > SVM. > > Laurent Dufour (2): > KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls > KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN > > arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++---------- > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 2 files changed, 23 insertions(+), 12 deletions(-) Thanks, series applied to my kvm-ppc-next branch. Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] Fix SVM hang at startup @ 2020-03-24 2:54 ` Paul Mackerras 0 siblings, 0 replies; 42+ messages in thread From: Paul Mackerras @ 2020-03-24 2:54 UTC (permalink / raw) To: Laurent Dufour; +Cc: linuxppc-dev, linux-kernel, kvm-ppc On Fri, Mar 20, 2020 at 11:26:41AM +0100, Laurent Dufour wrote: > This series is fixing a SVM hang occurring when starting a SVM requiring > more secure memory than available. The hang happens in the SVM when calling > UV_ESM. > > The following is happening: > > 1. SVM calls UV_ESM > 2. Ultravisor (UV) calls H_SVM_INIT_START > 3. Hypervisor (HV) calls UV_REGISTER_MEM_SLOT > 4. UV returns error because there is not enough free secure memory > 5. HV enter the error path in kvmppc_h_svm_init_start() > 6. In the return path, since kvm->arch.secure_guest is not yet set hrfid is > called > 7. As the HV doesn't know the SVM calling context hrfid is jumping to > unknown address in the SVM leading to various expections. > > This series fixes the setting of kvm->arch.secure_guest in > kvmppc_h_svm_init_start() to ensure that UV_RETURN is called on the return > path to get back to the UV. > > In addition to ensure that a malicious VM will not call UV reserved Hcall, > a check of the Secure bit in the calling MSR is addded to reject such a > call. > > It is assumed that the UV will filtered out such Hcalls made by a malicious > SVM. > > Laurent Dufour (2): > KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls > KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN > > arch/powerpc/kvm/book3s_hv.c | 32 ++++++++++++++++++++---------- > arch/powerpc/kvm/book3s_hv_uvmem.c | 3 ++- > 2 files changed, 23 insertions(+), 12 deletions(-) Thanks, series applied to my kvm-ppc-next branch. Paul. ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2020-03-24 14:29 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-20 10:26 [PATCH 0/2] Fix SVM hang at startup Laurent Dufour 2020-03-20 10:26 ` Laurent Dufour 2020-03-20 10:26 ` [PATCH 1/2] KVM: PPC: Book3S HV: check caller of H_SVM_* Hcalls Laurent Dufour 2020-03-20 10:26 ` Laurent Dufour 2020-03-20 10:26 ` Laurent Dufour 2020-03-20 12:22 ` Greg Kurz 2020-03-20 12:22 ` Greg Kurz 2020-03-20 12:22 ` Greg Kurz 2020-03-20 14:43 ` Laurent Dufour 2020-03-20 14:43 ` Laurent Dufour 2020-03-20 14:43 ` Laurent Dufour 2020-03-23 23:43 ` Paul Mackerras 2020-03-23 23:43 ` Paul Mackerras 2020-03-23 23:43 ` Paul Mackerras 2020-03-24 12:00 ` Greg Kurz 2020-03-24 12:00 ` Greg Kurz 2020-03-24 12:00 ` Greg Kurz 2020-03-24 13:13 ` Laurent Dufour 2020-03-24 13:13 ` Laurent Dufour 2020-03-24 13:13 ` Laurent Dufour 2020-03-21 0:40 ` Ram Pai 2020-03-21 0:40 ` Ram Pai 2020-03-20 10:26 ` [PATCH 2/2] KVM: PPC: Book3S HV: H_SVM_INIT_START must call UV_RETURN Laurent Dufour 2020-03-20 10:26 ` Laurent Dufour 2020-03-20 10:26 ` Laurent Dufour 2020-03-20 11:24 ` Bharata B Rao 2020-03-20 11:36 ` Bharata B Rao 2020-03-20 11:24 ` Bharata B Rao 2020-03-20 14:36 ` Laurent Dufour 2020-03-20 14:36 ` Laurent Dufour 2020-03-20 14:36 ` Laurent Dufour 2020-03-23 4:21 ` Bharata B Rao 2020-03-23 4:33 ` Bharata B Rao 2020-03-23 4:21 ` Bharata B Rao 2020-03-21 0:47 ` Ram Pai 2020-03-21 0:47 ` Ram Pai 2020-03-21 0:47 ` Ram Pai 2020-03-23 14:09 ` Fabiano Rosas 2020-03-23 14:09 ` Fabiano Rosas 2020-03-23 14:09 ` Fabiano Rosas 2020-03-24 2:54 ` [PATCH 0/2] Fix SVM hang at startup Paul Mackerras 2020-03-24 2:54 ` Paul Mackerras
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.