Re: [PATCH 01/13] KVM: arm64: Harden __ctxt_sys_reg() against out-of-range values

From: Marc Zyngier <maz@kernel.org>
To: Joey Gouly <joey.gouly@arm.com>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH 01/13] KVM: arm64: Harden __ctxt_sys_reg() against out-of-range values
Date: Tue, 20 Feb 2024 11:57:04 +0000	[thread overview]
Message-ID: <8634tn4acv.wl-maz@kernel.org> (raw)
In-Reply-To: <20240220112031.GA16168@e124191.cambridge.arm.com>

On Tue, 20 Feb 2024 11:20:31 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Mon, Feb 19, 2024 at 09:20:02AM +0000, Marc Zyngier wrote:
> > The unsuspecting kernel tinkerer can be easily confused into
> > writing something that looks like this:
> > 
> > 	ikey.lo = __vcpu_sys_reg(vcpu, SYS_APIAKEYLO_EL1);
> > 
> > which seems vaguely sensible, until you realise that the second
> > parameter is the encoding of a sysreg, and not the index into
> > the vcpu sysreg file... Debugging what happens in this case is
> 
> type safety :(

Are you advocating for making everything a struct? Or something else?

> 
> > an interesting exercise in head<->wall interactions.
> > 
> > As they often say: "Any resemblance to actual persons, living
> > or dead, or actual events is purely coincidental".
> > 
> > In order to save people's time, add some compile-time hardening
> > that will at least weed out the "stupidly out of range" values.
> > This will *not* catch anything that isn't a compile-time constant.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 181fef12e8e8..a5ec4c7d3966 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -895,7 +895,7 @@ struct kvm_vcpu_arch {
> >   * Don't bother with VNCR-based accesses in the nVHE code, it has no
> >   * business dealing with NV.
> >   */
> > -static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> > +static inline u64 *___ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> 
> When in doubt, add more underscores!

That's the one true way.

> 
> >  {
> >  #if !defined (__KVM_NVHE_HYPERVISOR__)
> >  	if (unlikely(cpus_have_final_cap(ARM64_HAS_NESTED_VIRT) &&
> > @@ -905,6 +905,13 @@ static inline u64 *__ctxt_sys_reg(const struct kvm_cpu_context *ctxt, int r)
> >  	return (u64 *)&ctxt->sys_regs[r];
> >  }
> >  
> > +#define __ctxt_sys_reg(c,r)						\
> > +	({								\
> > +	    	BUILD_BUG_ON(__builtin_constant_p(r) &&			\
> > +			     (r) >= NR_SYS_REGS);			\
> > +		___ctxt_sys_reg(c, r);					\
> > +	})
> 
> I'm assuming the extra macro layer is to try make __builtin_constant_p() as
> effective as possible? Otherwise maybe it relies on the compiler inling the
> ___ctxt_sys_reg() function?

It's not about efficiency. It's about making it *work*. Otherwise,
lack of inlining will screw you over, and you may not check anything.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.