| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ca5d8db3-b980-464f-8069-86bf769128e0@arm.com>
Date: Wed, 18 Dec 2024 16:07:21 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Marc Zyngier <maz@...nel.org>
Cc: linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, ryan.roberts@....com,
Oliver Upton <oliver.upton@...ux.dev>, James Morse <james.morse@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH V2 46/46] KVM: arm64: nv: Add trap forwarding for
FEAT_FGT2 described registers
On 12/10/24 14:35, Marc Zyngier wrote:
> On Tue, 10 Dec 2024 05:53:11 +0000,
> Anshuman Khandual <anshuman.khandual@....com> wrote:
>>
>> Describe remaining MDCR_EL2 register, and associate that with all FEAT_FGT2
>> exposed system registers it allows to trap.
>
> MDCR_EL2 register *bits*? How is that related to FGT2 at all?
>
>>
>> Cc: Marc Zyngier <maz@...nel.org>
>> Cc: Oliver Upton <oliver.upton@...ux.dev>
>> Cc: James Morse <james.morse@....com>
>> Cc: Suzuki K Poulose <suzuki.poulose@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: kvmarm@...ts.linux.dev
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>> Changes in V2:
>>
>> - Dropped check_cntr_accessible_N and CGT_CNTR_ACCESSIBLE_N constructs
>> - SYS_PMEVCNTSVR_EL1(N) access traps have been forwarded to CGT_MDCR_HPMN
>> - Updated check_mdcr_hpmn() to handle SYS_PMEVCNTSVR_EL1(N) registers
>> - Changed behaviour as BEHAVE_FORWARD_RW for CGT_MDCR_EnSPM
>>
>> arch/arm64/include/asm/kvm_host.h | 2 +
>> arch/arm64/kvm/emulate-nested.c | 158 ++++++++++++++++++++++++++++++
>> 2 files changed, 160 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index c80c07be3358..4cdce62642d1 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -441,6 +441,7 @@ enum vcpu_sysreg {
>> PMINTENSET_EL1, /* Interrupt Enable Set Register */
>> PMOVSSET_EL0, /* Overflow Flag Status Set Register */
>> PMUSERENR_EL0, /* User Enable Register */
>> + SPMSELR_EL0, /* System PMU Select Register */
>
> How could a system PMU be relevant to a VM? What is the point of
> bloating the vcpu for something that we will hopefully *never* make
> visible to guests?
>
>>
>> /* Pointer Authentication Registers in a strict increasing order. */
>> APIAKEYLO_EL1,
>> @@ -501,6 +502,7 @@ enum vcpu_sysreg {
>> CNTHP_CVAL_EL2,
>> CNTHV_CTL_EL2,
>> CNTHV_CVAL_EL2,
>> + SPMACCESSR_EL2, /* System PMU Access Register */
>
> Same here. It is pretty striking that these registers are never
> saved/restored or handled as traps, which is a good indication that
> this is pretty pointless.
>
>>
>> /* Anything from this can be RES0/RES1 sanitised */
>> MARKER(__SANITISED_REG_START__),
>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>> index 6c63cbfc11ea..c7d6d2034f27 100644
>> --- a/arch/arm64/kvm/emulate-nested.c
>> +++ b/arch/arm64/kvm/emulate-nested.c
>> @@ -79,6 +79,7 @@ enum cgt_group_id {
>> CGT_MDCR_TDRA,
>> CGT_MDCR_E2PB,
>> CGT_MDCR_TPMS,
>> + CGT_MDCR_EnSPM,
>> CGT_MDCR_TTRF,
>> CGT_MDCR_E2TB,
>> CGT_MDCR_TDCC,
>> @@ -125,6 +126,7 @@ enum cgt_group_id {
>> CGT_CNTHCTL_EL1PCTEN = __COMPLEX_CONDITIONS__,
>> CGT_CNTHCTL_EL1PTEN,
>>
>> + CGT_SPMSEL_SPMACCESS,
>> CGT_CPTR_TTA,
>> CGT_MDCR_HPMN,
>>
>> @@ -351,6 +353,12 @@ static const struct trap_bits coarse_trap_bits[] = {
>> .mask = MDCR_EL2_TPMS,
>> .behaviour = BEHAVE_FORWARD_RW,
>> },
>> + [CGT_MDCR_EnSPM] = {
>> + .index = MDCR_EL2,
>> + .value = MDCR_EL2_EnSPM,
>> + .mask = MDCR_EL2_EnSPM,
>> + .behaviour = BEHAVE_FORWARD_RW,
>> + },
>> [CGT_MDCR_TTRF] = {
>> .index = MDCR_EL2,
>> .value = MDCR_EL2_TTRF,
>> @@ -509,6 +517,7 @@ static enum trap_behaviour check_mdcr_hpmn(struct kvm_vcpu *vcpu)
>> switch (sysreg) {
>> case SYS_PMEVTYPERn_EL0(0) ... SYS_PMEVTYPERn_EL0(30):
>> case SYS_PMEVCNTRn_EL0(0) ... SYS_PMEVCNTRn_EL0(30):
>> + case SYS_PMEVCNTSVR_EL1(0) ... SYS_PMEVCNTSVR_EL1(30):
>> idx = (sys_reg_CRm(sysreg) & 0x3) << 3 | sys_reg_Op2(sysreg);
>> break;
>> case SYS_PMXEVTYPER_EL0:
>> @@ -528,6 +537,22 @@ static enum trap_behaviour check_mdcr_hpmn(struct kvm_vcpu *vcpu)
>> return BEHAVE_HANDLE_LOCALLY;
>> }
>>
>> +static enum trap_behaviour check_spmsel_spmaccess(struct kvm_vcpu *vcpu)
>> +{
>> + u64 spmaccessr_el2, spmselr_el2;
>> + int syspmusel;
>> +
>> + if (__vcpu_sys_reg(vcpu, MDCR_EL2) & MDCR_EL2_EnSPM) {
>
> I don't mind the test, but I don't see any sanitising of MDCR_EL2 to
> make EnSPM as RES0 when FEAT_SPMU is not implemented, which will be
> 100% of the cases.
>
>> + spmselr_el2 = __vcpu_sys_reg(vcpu, SPMSELR_EL0);
>> + spmaccessr_el2 = __vcpu_sys_reg(vcpu, SPMACCESSR_EL2);
>
> So these two values are *guaranteed* to be zero. At this stage, what
> is the point?
>
>> + syspmusel = FIELD_GET(SPMSELR_EL0_SYSPMUSEL_MASK, spmselr_el2);
>> +
>> + if (((spmaccessr_el2 >> (syspmusel * 2)) & 0x3) == 0x0)
>> + return BEHAVE_FORWARD_RW;
>
> What about value 0b01, which causes *writes* to be trapped?
>
>> + }
>> + return BEHAVE_HANDLE_LOCALLY;
>
> And then what? How do we handle this locally?
>
> Honestly, short of any additional handling, we would be better off
> just injecting an UNDEF back into the guest.
>
> M.
>
Hello Marc,
Thanks for your review and apologies for the delayed response. I am still
processing some of your review comments, and need some more time to fully
understand them in the given context. I will get back to you after doing
some more reading.
- Anshuman
Powered by blists - more mailing lists