| 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: <6c479aba-10b9-46ba-92fa-d3b20cc82d61@arm.com>
Date: Tue, 13 Aug 2024 12:23:31 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Marc Zyngier <maz@...nel.org>
Cc: linux-arm-kernel@...ts.infradead.org,
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>, kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [RFC 10/10] KVM: arm64: nv: Add new HDFGRTR2_GROUP &
HDFGRTR2_GROUP based FGU handling
On 8/4/24 16:35, Marc Zyngier wrote:
> On Sat, 03 Aug 2024 11:38:11 +0100,
> Anshuman Khandual <anshuman.khandual@....com> wrote:
>>
>> On 8/2/24 16:29, Marc Zyngier wrote:
>>> On Fri, 02 Aug 2024 10:25:44 +0100,
>>> Anshuman Khandual <anshuman.khandual@....com> wrote:
>>>> On 8/1/24 21:33, Marc Zyngier wrote:
>>>>> On Thu, 01 Aug 2024 11:46:22 +0100,
>>>>> Anshuman Khandual <anshuman.khandual@....com> wrote:
>>> [...]
>>>
>>>>>> + SR_FGT(SYS_SPMACCESSR_EL1, HDFGRTR2, nSPMACCESSR_EL1, 0),
>>>>> This (and I take it most of the stuff here) is also gated by
>>>>> MDCR_EL2.SPM, which is a coarse grained trap. That needs to be
>>>>> described as well. For every new register that you add here.
>>>> I did not find a SPM field in MDCR_EL2 either in latest ARM ARM or in
>>>> the latest XML. But as per current HDFGRTR2_EL2 description the field
>>>> nSPMACCESSR_EL1 is gated by FEAT_SPMU feature, which is being checked
>>>> via ID_AA64DFR1_EL1.PMU when required. So could you please give some
>>>> more details.
>>> I misspelled it. It is MDCR_EL2.EnSPM.
>>>
>>> And you are completely missing the point. It is not about
>>> HDFGRTR2_EL2, but about SPMACCESSR_EL1 (and all its little friends).
>>>
>>> To convince yourself, just look at the pseudocode for SPMACCESSR_EL1,
>>> limited to an EL1 access:
>>>
>>> elsif PSTATE.EL == EL1 then
>>> if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then
>>> UNDEFINED;
>>> elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMACCESSR_EL1 == '0') then
>>> AArch64.SystemAccessTrap(EL2, 0x18);
>>> elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then
>>> AArch64.SystemAccessTrap(EL2, 0x18);
>>> elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then
>>> if EL3SDDUndef() then
>>> UNDEFINED;
>>> else
>>> AArch64.SystemAccessTrap(EL3, 0x18);
>>> elsif EffectiveHCR_EL2_NVx() IN {'111'} then
>>> X[t, 64] = NVMem[0x8E8];
>>> else
>>> X[t, 64] = SPMACCESSR_EL1;
>>>
>>>
>>> Can you spot the *TWO* conditions where we take an exception to EL2
>>> with 0x18 as the EC?
>>>
>>> - One is when HDFGxTR2_EL2.nSPMACCESSR_EL1 == '0': that's a fine
>>> grained trap.
>>> - The other is when MDCR_EL2.EnSPM == '0': that's a coarse grained
>>> trap.
>>>
>>> Both conditions need to be captured in the various tables in this
>>> file, for each and every register that you describe.
>> Ahh, got it now. Because now KVM knows about SPMACCESSR_EL1 register,
>> access to that register must also be enabled via both fine grained trap
>> (HDFGxTR2_EL2.nSPMACCESSR_EL1) and coarse grained trap (MDCR_EL2.EnSPM).
>>
>> For all the registers that are being added via FEAT_FGT2 here in this
>> patch, their corresponding CGT based path also needs to be enabled via
>> corresponding CGT_MDCR_XXX groups.
> Exactly.
>
>>> [...]
>>>
>>>>> Now, the main issues are that:
>>>>>
>>>>> - you're missing the coarse grained trapping for all the stuff you
>>>>> have just added. It's not a huge amount of work, but you need, for
>>>>> each register, to describe what traps apply to it. The fine grained
>>>>> stuff is most, but not all of it. There should be enough of it
>>>>> already to guide you through it.
>>>> Coarse grained trapping for FEAT_FGT2 based fine grained registers ?
>>> Not for FEAT_FGT2. For the registers that FEAT_FGT2 traps. Can you see
>>> the difference?
>> Understood, for example PMIAR_EL1 register which FEAT_FGT2 now traps via
>>
>> SR_FGT(SYS_PMIAR_EL1, HDFGRTR2, nPMIAR_EL1, 0),
>>
>> also needs to have corresponding coarse grained trap.
>>
>> SR_TRAP(SYS_PMIAR_EL1, CGT_MDCR_TPM),
> Yup.
>
>> Similarly corresponding SR_TRAP() needs to be covered for all registers
>> that are now being trapped with FEAT_FGT2.
>>
>> Example code snippet.
>>
>> ........
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(8), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(9), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(10), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(11), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(12), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(13), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(14), CGT_MDCR_EnSPM),
>> + SR_TRAP(SYS_SPMEVFILT2R_EL0(15), CGT_MDCR_EnSPM),
> I think it is a bit more complicated than just that. Again, look at
> the pseudocode:
>
> elsif PSTATE.EL == EL1 then
> if HaveEL(EL3) && EL3SDDUndefPriority() && MDCR_EL3.EnPM2 == '0' then
> UNDEFINED;
> elsif HaveEL(EL3) && EL3SDDUndefPriority() && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then
> UNDEFINED;
> elsif EL2Enabled() && IsFeatureImplemented(FEAT_FGT2) && ((HaveEL(EL3) && SCR_EL3.FGTEn2 == '0') || HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0') then
> AArch64.SystemAccessTrap(EL2, 0x18);
> elsif EL2Enabled() && MDCR_EL2.EnSPM == '0' then
> AArch64.SystemAccessTrap(EL2, 0x18);
> elsif EL2Enabled() && SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then
> AArch64.SystemAccessTrap(EL2, 0x18);
> elsif HaveEL(EL3) && MDCR_EL3.EnPM2 == '0' then
> if EL3SDDUndef() then
> UNDEFINED;
> else
> AArch64.SystemAccessTrap(EL3, 0x18);
> elsif HaveEL(EL3) && SPMACCESSR_EL3<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00' then
> if EL3SDDUndef() then
> UNDEFINED;
> else
> AArch64.SystemAccessTrap(EL3, 0x18);
> elsif !IsSPMUCounterImplemented(UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m) then
> X[t, 64] = Zeros(64);
> else
> X[t, 64] = SPMEVFILT2R_EL0[UInt(SPMSELR_EL0.SYSPMUSEL), (UInt(SPMSELR_EL0.BANK) * 16) + m];
>
> which shows that an SPMEVFILT2Rn_EL0 access from EL1 traps to EL2 if:
>
> - either HDFGRTR2_EL2.nSPMEVTYPERn_EL0 == '0', (check)
> - or MDCR_EL2.EnSPM == '0', (check)
> - or SPMACCESSR_EL2<(UInt(SPMSELR_EL0.SYSPMUSEL) * 2) + 1:UInt(SPMSELR_EL0.SYSPMUSEL) * 2> == '00'
>
> and that last condition requires some more handling as you need to
> evaluate both SPMSELR_EL0.SYSPMUSEL and the corresponding field of
> SPMACCESSR_EL2 to make a decision. It's not majorly complicated, but
> it isn't solved by simply setting a static attribute.
So IIUC you are suggesting to handle SYS_SPMEVFILT2R_EL0() registers via
complex condition checks where the CGT_XXX can be directed to a function
callback instead ? For example, something like the following (untested).
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -116,6 +116,7 @@ enum cgt_group_id {
__COMPLEX_CONDITIONS__,
CGT_CNTHCTL_EL1PCTEN = __COMPLEX_CONDITIONS__,
CGT_CNTHCTL_EL1PTEN,
+ CGT_TEST,
CGT_CPTR_TTA,
@@ -486,6 +487,23 @@ static enum trap_behaviour check_cptr_tta(struct kvm_vcpu *vcpu)
return BEHAVE_HANDLE_LOCALLY;
}
+static enum trap_behaviour check_test(struct kvm_vcpu *vcpu)
+{
+ u64 spmaccessr_el2 = __vcpu_sys_reg(vcpu, SPMACCESSR_EL2);
+ u64 spmselr_el2 = __vcpu_sys_reg(vcpu, SPMSELR_EL0);
+ u64 mdcr_el2 = __vcpu_sys_reg(vcpu, MDCR_EL2);
+ int syspmusel, spmaccessr_idx;
+
+ if (!(mdcr_el2 & MDCR_EL2_EnSPM)) {
+ syspmusel = FIELD_GET(SPMSELR_EL0_SYSPMUSEL_MASK, spmselr_el2);
+ spmaccessr_idx = syspmusel * 2;
+
+ if (((spmaccessr_el2 >> spmaccessr_idx) & 0x3) == 0x0)
+ return BEHAVE_FORWARD_ANY;
+ }
+ return BEHAVE_HANDLE_LOCALLY;
+}
+
#define CCC(id, fn) \
[id - __COMPLEX_CONDITIONS__] = fn
@@ -493,6 +511,7 @@ static const complex_condition_check ccc[] = {
CCC(CGT_CNTHCTL_EL1PCTEN, check_cnthctl_el1pcten),
CCC(CGT_CNTHCTL_EL1PTEN, check_cnthctl_el1pten),
CCC(CGT_CPTR_TTA, check_cptr_tta),
+ CCC(CGT_TEST, check_test),
};
/*
@@ -1163,7 +1182,7 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
SR_TRAP(SYS_SPMEVFILTR_EL0(14), CGT_MDCR_EnSPM),
SR_TRAP(SYS_SPMEVFILTR_EL0(15), CGT_MDCR_EnSPM),
- SR_TRAP(SYS_SPMEVFILT2R_EL0(0), CGT_MDCR_EnSPM),
+ SR_TRAP(SYS_SPMEVFILT2R_EL0(0), CGT_TEST),
SR_TRAP(SYS_SPMEVFILT2R_EL0(1), CGT_MDCR_EnSPM),
SR_TRAP(SYS_SPMEVFILT2R_EL0(2), CGT_MDCR_EnSPM),
SR_TRAP(SYS_SPMEVFILT2R_EL0(3), CGT_MDCR_EnSPM),
Powered by blists - more mailing lists