| Message ID | 20250210184150.2145093-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Revamp Fine Grained Trap handling | expand |
On Mon, Feb 10, 2025 at 06:41:37PM +0000, Marc Zyngier wrote: > We don't seem to be handling the GCS-specific exception class. > Handle it by delivering an UNDEF to the guest, and populate the > relevant trap bits. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/handle_exit.c | 11 +++++++++++ > arch/arm64/kvm/sys_regs.c | 8 ++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 4f8354bf7dc5f..624a78a99e38a 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -294,6 +294,16 @@ static int handle_svc(struct kvm_vcpu *vcpu) > return 1; > } > > +static int kvm_handle_gcs(struct kvm_vcpu *vcpu) > +{ > + /* We don't expect GCS, so treat it with contempt */ > + if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP)) > + WARN_ON_ONCE(1); Just to check / better my understanging, do we enforce that this can't be exposed to the guest somewhere? I see __kvm_read_sanitised_id_reg() masks it out, and the sys_reg_descs table has it filtered, but I'm not immediately sure whether that prevents host userspace maliciously setting this? Otherwise this looks good to me. Mark. > + > + kvm_inject_undefined(vcpu); > + return 1; > +} > + > static int handle_ls64b(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -384,6 +394,7 @@ static exit_handle_fn arm_exit_handlers[] = { > [ESR_ELx_EC_BRK64] = kvm_handle_guest_debug, > [ESR_ELx_EC_FP_ASIMD] = kvm_handle_fpasimd, > [ESR_ELx_EC_PAC] = kvm_handle_ptrauth, > + [ESR_ELx_EC_GCS] = kvm_handle_gcs, > }; > > static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 18721c773475d..2ecd0d51a2dae 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -5056,6 +5056,14 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu) > HFGITR_EL2_nBRBIALL); > } > > + if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP)) { > + kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nGCS_EL0 | > + HFGxTR_EL2_nGCS_EL1); > + kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_nGCSPUSHM_EL1 | > + HFGITR_EL2_nGCSSTR_EL1 | > + HFGITR_EL2_nGCSEPP); > + } > + > set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); > out: > mutex_unlock(&kvm->arch.config_lock); > -- > 2.39.2 >
On Tue, 11 Feb 2025 12:36:35 +0000, Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Feb 10, 2025 at 06:41:37PM +0000, Marc Zyngier wrote: > > We don't seem to be handling the GCS-specific exception class. > > Handle it by delivering an UNDEF to the guest, and populate the > > relevant trap bits. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/handle_exit.c | 11 +++++++++++ > > arch/arm64/kvm/sys_regs.c | 8 ++++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > > index 4f8354bf7dc5f..624a78a99e38a 100644 > > --- a/arch/arm64/kvm/handle_exit.c > > +++ b/arch/arm64/kvm/handle_exit.c > > @@ -294,6 +294,16 @@ static int handle_svc(struct kvm_vcpu *vcpu) > > return 1; > > } > > > > +static int kvm_handle_gcs(struct kvm_vcpu *vcpu) > > +{ > > + /* We don't expect GCS, so treat it with contempt */ > > + if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP)) > > + WARN_ON_ONCE(1); > > Just to check / better my understanging, do we enforce that this can't > be exposed to the guest somewhere? > > I see __kvm_read_sanitised_id_reg() masks it out, and the sys_reg_descs > table has it filtered, but I'm not immediately sure whether that > prevents host userspace maliciously setting this? On writing to the idreg, you end-up in set_id_aa64pfr1_el1(), which calls into set_id_reg(). There, arm64_check_features() compares each and every feature in that register with the mask and limits that have been established. Since GCS is not part of the writable mask, and that it has been disabled, the only valid value for ID_AA64PFR1_EL1.GCS is 0. A non-zero value provided by userspace will be caught by the last check in arm64_check_features(), and an error be returned. HTH, M.
On Tue, Feb 11, 2025 at 01:35:54PM +0000, Marc Zyngier wrote: > On Tue, 11 Feb 2025 12:36:35 +0000, > Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Feb 10, 2025 at 06:41:37PM +0000, Marc Zyngier wrote: > > > +static int kvm_handle_gcs(struct kvm_vcpu *vcpu) > > > +{ > > > + /* We don't expect GCS, so treat it with contempt */ > > > + if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP)) > > > + WARN_ON_ONCE(1); > > > > Just to check / better my understanging, do we enforce that this can't > > be exposed to the guest somewhere? > > > > I see __kvm_read_sanitised_id_reg() masks it out, and the sys_reg_descs > > table has it filtered, but I'm not immediately sure whether that > > prevents host userspace maliciously setting this? > > On writing to the idreg, you end-up in set_id_aa64pfr1_el1(), which > calls into set_id_reg(). There, arm64_check_features() compares each > and every feature in that register with the mask and limits that have > been established. > > Since GCS is not part of the writable mask, and that it has been > disabled, the only valid value for ID_AA64PFR1_EL1.GCS is 0. A > non-zero value provided by userspace will be caught by the last check > in arm64_check_features(), and an error be returned. Perfect, thanks! Mark.
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 4f8354bf7dc5f..624a78a99e38a 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -294,6 +294,16 @@ static int handle_svc(struct kvm_vcpu *vcpu) return 1; } +static int kvm_handle_gcs(struct kvm_vcpu *vcpu) +{ + /* We don't expect GCS, so treat it with contempt */ + if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, GCS, IMP)) + WARN_ON_ONCE(1); + + kvm_inject_undefined(vcpu); + return 1; +} + static int handle_ls64b(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -384,6 +394,7 @@ static exit_handle_fn arm_exit_handlers[] = { [ESR_ELx_EC_BRK64] = kvm_handle_guest_debug, [ESR_ELx_EC_FP_ASIMD] = kvm_handle_fpasimd, [ESR_ELx_EC_PAC] = kvm_handle_ptrauth, + [ESR_ELx_EC_GCS] = kvm_handle_gcs, }; static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 18721c773475d..2ecd0d51a2dae 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -5056,6 +5056,14 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu) HFGITR_EL2_nBRBIALL); } + if (!kvm_has_feat(kvm, ID_AA64PFR1_EL1, GCS, IMP)) { + kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nGCS_EL0 | + HFGxTR_EL2_nGCS_EL1); + kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_nGCSPUSHM_EL1 | + HFGITR_EL2_nGCSSTR_EL1 | + HFGITR_EL2_nGCSEPP); + } + set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags); out: mutex_unlock(&kvm->arch.config_lock);
We don't seem to be handling the GCS-specific exception class. Handle it by delivering an UNDEF to the guest, and populate the relevant trap bits. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/handle_exit.c | 11 +++++++++++ arch/arm64/kvm/sys_regs.c | 8 ++++++++ 2 files changed, 19 insertions(+)