[PATCH 0/2] KVM: arm64: Fixes for SMC64 SYSTEM_RESET2 calls

[PATCH 0/2] KVM: arm64: Fixes for SMC64 SYSTEM_RESET2 calls

[Oliver Upton]

This series addresses a couple of issues with how KVM exposes SMC64
calls to its guest. It is currently possible for an AArch32 guest to
discover the SMC64 SYSTEM_RESET2 function (via
PSCI_1_0_FN_PSCI_FEATURES) and even make a call to it. SMCCC does not
allow for 64 bit calls to be made from a 32 bit state.

Patch 1 cleans up the way we filter SMC64 calls in PSCI. Using a switch
with case statements for each possibly-filtered function is asking for
trouble. Instead, pivot off of the bit that indicates the desired
calling convention. This plugs the PSCI_FEATURES hole for SYSTEM_RESET2.

Patch 2 adds a check to the PSCI v1.x call handler in KVM, bailing out
early if the guest is not allowed to use a particular function. This
closes the door on calls to 64-bit SYSTEM_RESET2 from AArch32.

My first crack at this [1] was missing the fix for direct calls to
SYSTEM_RESET2. Taking the patch out of that series and sending
separately.

Applies on top of today's kvmarm pull, commit:

  21ea45784275 ("KVM: arm64: fix typos in comments")

[1]: https://patchwork.kernel.org/project/kvm/patch/20220311174001.605719-3-oupton@google.com/

Oliver Upton (2):
  KVM: arm64: Generally disallow SMC64 for AArch32 guests
  KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32

 arch/arm64/kvm/psci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.35.1.894.gb6a874cedc-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm64: Generally disallow SMC64 for AArch32 guests

[Oliver Upton]

The only valid calling SMC calling convention from an AArch32 state is
SMC32. Disallow any PSCI function that sets the SMC64 function ID bit
when called from AArch32 rather than comparing against known SMC64 PSCI
functions.

Note that without this change KVM advertises the SMC64 flavor of
SYSTEM_RESET2 to AArch32 guests.

Fixes: d43583b890e7 ("KVM: arm64: Expose PSCI SYSTEM_RESET2 call to the guest")
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Reiji Watanabe <reijiw@google.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/psci.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index a433c3eac9b7..cd3ee947485f 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -216,15 +216,11 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu)
 
 static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn)
 {
-	switch(fn) {
-	case PSCI_0_2_FN64_CPU_SUSPEND:
-	case PSCI_0_2_FN64_CPU_ON:
-	case PSCI_0_2_FN64_AFFINITY_INFO:
-		/* Disallow these functions for 32bit guests */
-		if (vcpu_mode_is_32bit(vcpu))
-			return PSCI_RET_NOT_SUPPORTED;
-		break;
-	}
+	/*
+	 * Prevent 32 bit guests from calling 64 bit PSCI functions.
+	 */
+	if ((fn & PSCI_0_2_64BIT) && vcpu_mode_is_32bit(vcpu))
+		return PSCI_RET_NOT_SUPPORTED;
 
 	return 0;
 }
-- 
2.35.1.894.gb6a874cedc-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/2] KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32

[Oliver Upton]

The SMCCC does not allow the SMC64 calling convention to be used from
AArch32. While KVM checks to see if the calling convention is allowed in
PSCI_1_0_FN_PSCI_FEATURES, it does not actually prevent calls to
unadvertised PSCI v1.0+ functions.

Check to see if the requested function is allowed from the guest's
execution state. Deny the call if it is not.

Fixes: d43583b890e7 ("KVM: arm64: Expose PSCI SYSTEM_RESET2 call to the guest")
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/psci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index cd3ee947485f..0d771468b708 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -318,6 +318,10 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 	if (minor > 1)
 		return -EINVAL;
 
+	val = kvm_psci_check_allowed_function(vcpu, psci_fn);
+	if (val)
+		goto out;
+
 	switch(psci_fn) {
 	case PSCI_0_2_FN_PSCI_VERSION:
 		val = minor == 0 ? KVM_ARM_PSCI_1_0 : KVM_ARM_PSCI_1_1;
@@ -378,6 +382,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 		return kvm_psci_0_2_call(vcpu);
 	}
 
+out:
 	smccc_set_retval(vcpu, val, 0, 0, 0);
 	return ret;
 }
-- 
2.35.1.894.gb6a874cedc-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32

[Reiji Watanabe]

On 3/18/22 12:38 PM, Oliver Upton wrote:
> The SMCCC does not allow the SMC64 calling convention to be used from
> AArch32. While KVM checks to see if the calling convention is allowed in
> PSCI_1_0_FN_PSCI_FEATURES, it does not actually prevent calls to
> unadvertised PSCI v1.0+ functions.
>
> Check to see if the requested function is allowed from the guest's
> execution state. Deny the call if it is not.
>
> Fixes: d43583b890e7 ("KVM: arm64: Expose PSCI SYSTEM_RESET2 call to the guest")
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

BTW, considering the new kvm_psci_check_allowed_function()implementation
in the patch-1, it might be better to call kvm_psci_check_allowed_function()
from kvm_psci_call() instead?  Then, we could avoid the similar issue
next time we support a newer PSCI version.

Thanks,
Reiji


> ---
>   arch/arm64/kvm/psci.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index cd3ee947485f..0d771468b708 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -318,6 +318,10 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
>       if (minor > 1)
>               return -EINVAL;
>
> +     val = kvm_psci_check_allowed_function(vcpu, psci_fn);
> +     if (val)
> +             goto out;
> +
>       switch(psci_fn) {
>       case PSCI_0_2_FN_PSCI_VERSION:
>               val = minor == 0 ? KVM_ARM_PSCI_1_0 : KVM_ARM_PSCI_1_1;
> @@ -378,6 +382,7 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
>               return kvm_psci_0_2_call(vcpu);
>       }
>
> +out:
>       smccc_set_retval(vcpu, val, 0, 0, 0);
>       return ret;
>   }
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32

[Oliver Upton]

On Mon, Mar 21, 2022 at 09:41:39PM -0700, Reiji Watanabe wrote:
> On 3/18/22 12:38 PM, Oliver Upton wrote:
> > The SMCCC does not allow the SMC64 calling convention to be used from
> > AArch32. While KVM checks to see if the calling convention is allowed in
> > PSCI_1_0_FN_PSCI_FEATURES, it does not actually prevent calls to
> > unadvertised PSCI v1.0+ functions.
> >
> > Check to see if the requested function is allowed from the guest's
> > execution state. Deny the call if it is not.
> >
> > Fixes: d43583b890e7 ("KVM: arm64: Expose PSCI SYSTEM_RESET2 call to the guest")
> > Cc: Will Deacon <will@kernel.org>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>

Appreciated :-)

> BTW, considering the new kvm_psci_check_allowed_function()implementation
> in the patch-1, it might be better to call kvm_psci_check_allowed_function()
> from kvm_psci_call() instead?  Then, we could avoid the similar issue
> next time we support a newer PSCI version.

Good point. If Marc doesn't bite in the next day or two I'll address
this with a new spin, otherwise I'll do a separate cleanup. Just want to
avoid spamming on this topic since I already replied with yet another
patch [1].

Thanks!

[1] https://lore.kernel.org/kvmarm/20220322013310.1880100-1-oupton@google.com

--
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/2] KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32

[Marc Zyngier]

On Tue, 22 Mar 2022 05:49:29 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> On Mon, Mar 21, 2022 at 09:41:39PM -0700, Reiji Watanabe wrote:
> > On 3/18/22 12:38 PM, Oliver Upton wrote:
> > > The SMCCC does not allow the SMC64 calling convention to be used from
> > > AArch32. While KVM checks to see if the calling convention is allowed in
> > > PSCI_1_0_FN_PSCI_FEATURES, it does not actually prevent calls to
> > > unadvertised PSCI v1.0+ functions.
> > >
> > > Check to see if the requested function is allowed from the guest's
> > > execution state. Deny the call if it is not.
> > >
> > > Fixes: d43583b890e7 ("KVM: arm64: Expose PSCI SYSTEM_RESET2 call to the guest")
> > > Cc: Will Deacon <will@kernel.org>
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > 
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> Appreciated :-)
> 
> > BTW, considering the new kvm_psci_check_allowed_function()implementation
> > in the patch-1, it might be better to call kvm_psci_check_allowed_function()
> > from kvm_psci_call() instead?  Then, we could avoid the similar issue
> > next time we support a newer PSCI version.
> 
> Good point. If Marc doesn't bite in the next day or two I'll address
> this with a new spin, otherwise I'll do a separate cleanup. Just want to
> avoid spamming on this topic since I already replied with yet another
> patch [1].

Please do, and I'll queue that for -rc1.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/2] KVM: arm64: Fixes for SMC64 SYSTEM_RESET2 calls

[Will Deacon]

On Fri, Mar 18, 2022 at 07:38:29PM +0000, Oliver Upton wrote:
> This series addresses a couple of issues with how KVM exposes SMC64
> calls to its guest. It is currently possible for an AArch32 guest to
> discover the SMC64 SYSTEM_RESET2 function (via
> PSCI_1_0_FN_PSCI_FEATURES) and even make a call to it. SMCCC does not
> allow for 64 bit calls to be made from a 32 bit state.
> 
> Patch 1 cleans up the way we filter SMC64 calls in PSCI. Using a switch
> with case statements for each possibly-filtered function is asking for
> trouble. Instead, pivot off of the bit that indicates the desired
> calling convention. This plugs the PSCI_FEATURES hole for SYSTEM_RESET2.
> 
> Patch 2 adds a check to the PSCI v1.x call handler in KVM, bailing out
> early if the guest is not allowed to use a particular function. This
> closes the door on calls to 64-bit SYSTEM_RESET2 from AArch32.
> 
> My first crack at this [1] was missing the fix for direct calls to
> SYSTEM_RESET2. Taking the patch out of that series and sending
> separately.
> 
> Applies on top of today's kvmarm pull, commit:
> 
>   21ea45784275 ("KVM: arm64: fix typos in comments")
> 
> [1]: https://patchwork.kernel.org/project/kvm/patch/20220311174001.605719-3-oupton@google.com/
> 
> Oliver Upton (2):
>   KVM: arm64: Generally disallow SMC64 for AArch32 guests
>   KVM: arm64: Actually prevent SMC64 SYSTEM_RESET2 from AArch32
> 
>  arch/arm64/kvm/psci.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

For both patches:

Acked-by: Will Deacon <will@kernel.org>

Thanks for fixing this!

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH] KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler

[Oliver Upton]

We already sanitize the guest's PSCI version when it is being written by
userspace, rejecting unsupported version numbers. Additionally, the
'minor' parameter to kvm_psci_1_x_call() is a constant known at compile
time for all callsites.

Though it is benign, the additional check against the
PSCI kvm_psci_1_x_call() is unnecessary and likely to be missed the next
time KVM raises its maximum PSCI version. Drop the check altogether and
rely on sanitization when the PSCI version is set by userspace.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---

Sorry for not sending this with the other ones. I took another read and
do not believe this check is necessary + might hurt when we raise the
PSCI version again.

Applies on top of the series [1], which itself is based on kvmarm/next
at commit:

  21ea45784275 ("KVM: arm64: fix typos in comments")

[1]: http://lore.kernel.org/r/20220318193831.482349-1-oupton@google.com

 arch/arm64/kvm/psci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
index 0d771468b708..7cd3fe62275f 100644
--- a/arch/arm64/kvm/psci.c
+++ b/arch/arm64/kvm/psci.c
@@ -315,9 +315,6 @@ static int kvm_psci_1_x_call(struct kvm_vcpu *vcpu, u32 minor)
 	unsigned long val;
 	int ret = 1;
 
-	if (minor > 1)
-		return -EINVAL;
-
 	val = kvm_psci_check_allowed_function(vcpu, psci_fn);
 	if (val)
 		goto out;
-- 
2.35.1.894.gb6a874cedc-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm