Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Mark Rutland <mark.rutland@arm.com>]

Hi Suzuki,

On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote:
> On 27/02/2024 11:13, Anshuman Khandual wrote:
> > On 2/27/24 15:34, Mark Rutland wrote:
> > > On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
> > > > On 2/21/24 19:31, Mark Rutland wrote:
> > > > > On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
> > > > > > Currently BRBE feature is not supported in a guest environment. This hides
> > > > > > BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
> > > > > 
> > > > > Does that means that a guest can currently see BRBE advertised in the
> > > > > ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
> > > > > today?
> > > > 
> > > > IIRC it is hidden, but will have to double check. When experimenting for BRBE
> > > > guest support enablement earlier, following changes were need for the feature
> > > > to be visible in ID_AA64DFR0_EL1.
> > > > 
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 646591c67e7a..f258568535a8 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
> > > >   };
> > > >   static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> > > > +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
> > > >          S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
> > > >          ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
> > > >          ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
> > > > 
> > > > Should we add the following entry - explicitly hiding BRBE from the guest
> > > > as a prerequisite patch ?
> 
> This has nothing to do with the Guest visibility of the BRBE. This is
> specifically for host "userspace" (via MRS emulation).
> 
> > > > 
> > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
> > > 
> > > Is it visbile currently, or is it hidden currently?
> > > 
> > > * If it is visible before this patch, that's a latent bug that we need to go
> > >    fix first, and that'll require more coordination.
> > > 
> > > * If it is not visible before this patch, there's no problem in the code, but
> > >    the commit message needs to explicitly mention that's the case as the commit
> > >    message currently implies it is visible by only mentioning hiding it.
> > > 
> > > ... so can you please double check as you suggested above? We should be able to
> > > explain why it is or is not visible today.
> > 
> > It is currently hidden i.e following code returns 1 in the host
> > but returns 0 inside the guest.
> > 
> > aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> > brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
> > 
> > Hence - will update the commit message here as suggested.
> 
> This is by virtue of the masking we do in the kvm/sysreg.c below.

Yep, once this patch is applied.

I think we might have some crossed wires here; I'm only really asking for the
commit message (and title) to be updated and clarified.

Ignoring the patchlet above, and just considering the original patch:

IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for
the guest because we don't have an arm64_ftr_bits entry for the
ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero
in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1()
calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.

This series as-is doesn't add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE,
so it'd still be hidden from a guest regardless of whether we add explicit
masking in read_sanitised_id_aa64dfr0_el1(). The reason to add that masking is
to be explicit, so that if/when we add an arm64_ftr_bits entry for
ID_AA64DFR0_EL1.BRBE, it isn't exposed to a guest unexpectedly.

Similarly, IIUC the BRBE register accesses are *already* trapped, and
emulate_sys_reg() will log a warning an inject an UNDEFINED exception into the
guest if the guest tries to access the BRBE registers. Any well-behaved guest
*shouldn't* do that, but a poorly-behaved guest could do that and (slowly) spam
dmesg with messages about the unhandled sysreg traps. The reasons to handle
thos regs is largely to suppress that warning, and to make it clear that we
intend for those to be handled as undef.

So the commit title should be something like:

  KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED

... and the message should mention the key points from the above.

Suzuki, does that sound right to you?

Anshuman, can you go re-write the commit message with that in mind?

Mark.