* [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
@ 2020-05-20 16:51 Laurent Dufour
2020-05-20 17:32 ` Greg Kurz
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Dufour @ 2020-05-20 16:51 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev, linux-kernel, paulus; +Cc: mpe, sukadev, linuxram
The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
reserved to the Ultravisor.
However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
context of the VM calling UV_ESM. This allows the Hypervisor to return to
the guest without going through the Ultravisor. Thus the Secure bit of SRR1
is not set in that particular case.
In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
not set in that case.
Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
arch/powerpc/kvm/book3s_hv.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 93493f0cbfe8..eb1f96cb7b72 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
ret = kvmppc_h_svm_init_done(vcpu->kvm);
break;
case H_SVM_INIT_ABORT:
- ret = H_UNSUPPORTED;
- if (kvmppc_get_srr1(vcpu) & MSR_S)
- ret = kvmppc_h_svm_init_abort(vcpu->kvm);
+ ret = kvmppc_h_svm_init_abort(vcpu->kvm);
break;
default:
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 16:51 [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT Laurent Dufour
@ 2020-05-20 17:32 ` Greg Kurz
2020-05-20 17:35 ` Laurent Dufour
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
0 siblings, 2 replies; 8+ messages in thread
From: Greg Kurz @ 2020-05-20 17:32 UTC (permalink / raw)
To: Laurent Dufour
Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram
On Wed, 20 May 2020 18:51:10 +0200
Laurent Dufour <ldufour@linux.ibm.com> wrote:
> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
> reserved to the Ultravisor.
>
> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
> context of the VM calling UV_ESM. This allows the Hypervisor to return to
> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
> is not set in that particular case.
>
> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
> not set in that case.
>
Why not checking vcpu->kvm->arch.secure_guest then ?
> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 93493f0cbfe8..eb1f96cb7b72 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> ret = kvmppc_h_svm_init_done(vcpu->kvm);
> break;
> case H_SVM_INIT_ABORT:
> - ret = H_UNSUPPORTED;
> - if (kvmppc_get_srr1(vcpu) & MSR_S)
> - ret = kvmppc_h_svm_init_abort(vcpu->kvm);
or at least put a comment to explain why H_SVM_INIT_ABORT
doesn't have the same sanity check as the other SVM hcalls.
> + ret = kvmppc_h_svm_init_abort(vcpu->kvm);
> break;
>
> default:
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 17:32 ` Greg Kurz
@ 2020-05-20 17:35 ` Laurent Dufour
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
1 sibling, 0 replies; 8+ messages in thread
From: Laurent Dufour @ 2020-05-20 17:35 UTC (permalink / raw)
To: Greg Kurz
Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram
Le 20/05/2020 à 19:32, Greg Kurz a écrit :
> On Wed, 20 May 2020 18:51:10 +0200
> Laurent Dufour <ldufour@linux.ibm.com> wrote:
>
>> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
>> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
>> reserved to the Ultravisor.
>>
>> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
>> context of the VM calling UV_ESM. This allows the Hypervisor to return to
>> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
>> is not set in that particular case.
>>
>> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
>> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
>> not set in that case.
>>
>
> Why not checking vcpu->kvm->arch.secure_guest then ?
I don't think that's the right place.
>
>> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 93493f0cbfe8..eb1f96cb7b72 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1099,9 +1099,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
>> ret = kvmppc_h_svm_init_done(vcpu->kvm);
>> break;
>> case H_SVM_INIT_ABORT:
>> - ret = H_UNSUPPORTED;
>> - if (kvmppc_get_srr1(vcpu) & MSR_S)
>> - ret = kvmppc_h_svm_init_abort(vcpu->kvm);
>
> or at least put a comment to explain why H_SVM_INIT_ABORT
> doesn't have the same sanity check as the other SVM hcalls.
I agree that might help. I'll send a v2 with a comment there.
>
>> + ret = kvmppc_h_svm_init_abort(vcpu->kvm);
>> break;
>>
>> default:
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 17:32 ` Greg Kurz
2020-05-20 17:35 ` Laurent Dufour
@ 2020-05-20 17:43 ` Laurent Dufour
2020-05-20 18:23 ` Greg Kurz
` (2 more replies)
1 sibling, 3 replies; 8+ messages in thread
From: Laurent Dufour @ 2020-05-20 17:43 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev, linux-kernel, paulus; +Cc: groug, mpe, sukadev, linuxram
The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
reserved to the Ultravisor.
However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
context of the VM calling UV_ESM. This allows the Hypervisor to return to
the guest without going through the Ultravisor. Thus the Secure bit of SRR1
is not set in that particular case.
In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
not set in that case.
Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
---
arch/powerpc/kvm/book3s_hv.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 93493f0cbfe8..6ad1a3b14300 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
ret = kvmppc_h_svm_init_done(vcpu->kvm);
break;
case H_SVM_INIT_ABORT:
- ret = H_UNSUPPORTED;
- if (kvmppc_get_srr1(vcpu) & MSR_S)
- ret = kvmppc_h_svm_init_abort(vcpu->kvm);
+ /*
+ * Even if that call is made by the Ultravisor, the SSR1 value
+ * is the guest context one, with the secure bit clear as it has
+ * not yet been secured. So we can't check it here.
+ */
+ ret = kvmppc_h_svm_init_abort(vcpu->kvm);
break;
default:
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
@ 2020-05-20 18:23 ` Greg Kurz
2020-05-21 6:08 ` Ram Pai
2020-05-27 4:16 ` Paul Mackerras
2 siblings, 0 replies; 8+ messages in thread
From: Greg Kurz @ 2020-05-20 18:23 UTC (permalink / raw)
To: Laurent Dufour
Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, mpe, sukadev, linuxram
On Wed, 20 May 2020 19:43:08 +0200
Laurent Dufour <ldufour@linux.ibm.com> wrote:
> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
> reserved to the Ultravisor.
>
> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
> context of the VM calling UV_ESM. This allows the Hypervisor to return to
> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
> is not set in that particular case.
>
> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
> not set in that case.
>
> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> arch/powerpc/kvm/book3s_hv.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 93493f0cbfe8..6ad1a3b14300 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> ret = kvmppc_h_svm_init_done(vcpu->kvm);
> break;
> case H_SVM_INIT_ABORT:
> - ret = H_UNSUPPORTED;
> - if (kvmppc_get_srr1(vcpu) & MSR_S)
> - ret = kvmppc_h_svm_init_abort(vcpu->kvm);
> + /*
> + * Even if that call is made by the Ultravisor, the SSR1 value
> + * is the guest context one, with the secure bit clear as it has
> + * not yet been secured. So we can't check it here.
> + */
> + ret = kvmppc_h_svm_init_abort(vcpu->kvm);
> break;
>
> default:
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
2020-05-20 18:23 ` Greg Kurz
@ 2020-05-21 6:08 ` Ram Pai
2020-05-27 4:16 ` Paul Mackerras
2 siblings, 0 replies; 8+ messages in thread
From: Ram Pai @ 2020-05-21 6:08 UTC (permalink / raw)
To: Laurent Dufour
Cc: kvm-ppc, linuxppc-dev, linux-kernel, paulus, groug, mpe, sukadev
On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote:
> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
> reserved to the Ultravisor.
>
> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
> context of the VM calling UV_ESM. This allows the Hypervisor to return to
> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
> is not set in that particular case.
>
> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
> not set in that case.
>
> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 93493f0cbfe8..6ad1a3b14300 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1099,9 +1099,12 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> ret = kvmppc_h_svm_init_done(vcpu->kvm);
> break;
> case H_SVM_INIT_ABORT:
> - ret = H_UNSUPPORTED;
> - if (kvmppc_get_srr1(vcpu) & MSR_S)
> - ret = kvmppc_h_svm_init_abort(vcpu->kvm);
> + /*
> + * Even if that call is made by the Ultravisor, the SSR1 value
> + * is the guest context one, with the secure bit clear as it has
> + * not yet been secured. So we can't check it here.
> + */
Frankly speaking, the comment above when read in isolation; i.e without
the delete code above, feels out of place. The reasoning for change is
anyway captured in the changelog. So, I think, we should delete this
comment.
Also the comment above assumes the Ultravisor will call H_SVM_INIT_ABORT
with SRR1(S) bit not set; which may or may not be true. Regardless of
who and how H_SVM_INIT_ABORT is called, we should just call
kvmppc_h_svm_init_abort() and let it deal with the complexities.
RP
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
2020-05-20 18:23 ` Greg Kurz
2020-05-21 6:08 ` Ram Pai
@ 2020-05-27 4:16 ` Paul Mackerras
2020-05-27 9:23 ` Laurent Dufour
2 siblings, 1 reply; 8+ messages in thread
From: Paul Mackerras @ 2020-05-27 4:16 UTC (permalink / raw)
To: Laurent Dufour
Cc: kvm-ppc, linuxppc-dev, linux-kernel, groug, mpe, sukadev, linuxram
On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote:
> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
> reserved to the Ultravisor.
>
> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
> context of the VM calling UV_ESM. This allows the Hypervisor to return to
> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
> is not set in that particular case.
>
> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
> not set in that case.
>
> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Thanks, applied to my kvm-ppc-next branch. I expanded the comment in
the code a little.
Paul.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT
2020-05-27 4:16 ` Paul Mackerras
@ 2020-05-27 9:23 ` Laurent Dufour
0 siblings, 0 replies; 8+ messages in thread
From: Laurent Dufour @ 2020-05-27 9:23 UTC (permalink / raw)
To: Paul Mackerras
Cc: kvm-ppc, linuxppc-dev, linux-kernel, groug, mpe, sukadev, linuxram
Le 27/05/2020 à 06:16, Paul Mackerras a écrit :
> On Wed, May 20, 2020 at 07:43:08PM +0200, Laurent Dufour wrote:
>> The commit 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_*
>> Hcalls") added checks of secure bit of SRR1 to filter out the Hcall
>> reserved to the Ultravisor.
>>
>> However, the Hcall H_SVM_INIT_ABORT is made by the Ultravisor passing the
>> context of the VM calling UV_ESM. This allows the Hypervisor to return to
>> the guest without going through the Ultravisor. Thus the Secure bit of SRR1
>> is not set in that particular case.
>>
>> In the case a regular VM is calling H_SVM_INIT_ABORT, this hcall will be
>> filtered out in kvmppc_h_svm_init_abort() because kvm->arch.secure_guest is
>> not set in that case.
>>
>> Fixes: 8c47b6ff29e3 ("KVM: PPC: Book3S HV: Check caller of H_SVM_* Hcalls")
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>
> Thanks, applied to my kvm-ppc-next branch. I expanded the comment in
> the code a little.
Thanks, the comment is more explicit now.
Laurent.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-05-27 9:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 16:51 [PATCH] KVM: PPC: Book3S HV: relax check on H_SVM_INIT_ABORT Laurent Dufour
2020-05-20 17:32 ` Greg Kurz
2020-05-20 17:35 ` Laurent Dufour
2020-05-20 17:43 ` [PATCH v2] " Laurent Dufour
2020-05-20 18:23 ` Greg Kurz
2020-05-21 6:08 ` Ram Pai
2020-05-27 4:16 ` Paul Mackerras
2020-05-27 9:23 ` Laurent Dufour
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).