| 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: <87sevk4kgr.wl-maz@kernel.org>
Date: Sun, 04 Aug 2024 12:05:08 +0100
From: Marc Zyngier <maz@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
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 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.
> +
> + SR_TRAP(SYS_SPMCGCR0_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMCGCR1_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMACCESSR_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMCFGR_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMCNTENCLR_EL0, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMCNTENSET_EL0, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMCR_EL0, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMDEVAFF_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMDEVARCH_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMIIDR_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMINTENCLR_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMINTENSET_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMOVSCLR_EL0, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMOVSSET_EL0, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMSCR_EL1, CGT_MDCR_EnSPM),
> + SR_TRAP(SYS_SPMSELR_EL0, CGT_MDCR_EnSPM),
So please look at the pseudocode for each and every register you
add. If there is a trap to EL2 that is described, you must have the
corresponding handling. None of that is optional.
> ........
>
> >
> >> Afraid, did not understand this. Could you please give some pointers
> >> on similar existing code.
> >
> > See above. And if you want some example, just took at the file you are
> > patching already. Look at how MDCR_EL2 conditions the trapping of all
> > the debug, PMU, AMU registers, for example. There is no shortage of
> > them.
>
> Right, I understand your point now. I am planning to send out the following
> patches (in reply to the current thread) for your review, before sending out
> the entire series as V1.
>
> - Patch adding VNCR details for virtual EL2 access
> - Patch adding FEAT_FGT2 based FGU handling
> - Patch adding corresponding CGT handling for registers being added above
>
> Although, can skip that step and just send out the series directly if you so
> prefer. Please do suggest. Thanks for all the detailed information above.
My suggestion is that you take a good look at the pseudocode and
implement all the existing requirements. Yes, this is undesirably
complicated, but I'm not the one making the rules here...
Once you think you have all these requirements in place, please post
the series. But not before that. If you have any question, just ask.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists