Re: [PATCH] KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs

From: Miguel Luis <miguel.luis@oracle.com>
To: Marc Zyngier <maz@kernel.org>
Cc: "kvmarm@lists.linux.dev" <kvmarm@lists.linux.dev>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.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>
Subject: Re: [PATCH] KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs
Date: Mon, 16 Oct 2023 15:01:44 +0000	[thread overview]
Message-ID: <69203F5C-1D5C-4E21-AA98-1DA2433A25A2@oracle.com> (raw)
In-Reply-To: <1DFF1105-A2C0-4249-9F71-3C9636C8FBCE@kernel.org>

Hi Marc,

> On 16 Oct 2023, at 14:28, Marc Zyngier <maz@kernel.org> wrote:
> 
> 
> 
> On 16 October 2023 15:15:02 BST, Miguel Luis <miguel.luis@oracle.com> wrote:
>> Hi Marc,
>> 
>>> On 13 Oct 2023, at 22:33, Marc Zyngier <maz@kernel.org> wrote:
>>> 
>>> DBGVCR32_EL2, DACR32_EL2, IFSR32_EL2 and FPEXC32_EL2 are required to
>>> UNDEF when AArch32 isn't implemented, which is definitely the case when
>>> running NV.
>>> 
>>> Given that this is the only case where these registers can trap,
>>> unconditionally inject an UNDEF exception.
>>> 
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>> arch/arm64/kvm/sys_regs.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 0afd6136e275..0071ccccaf00 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -1961,7 +1961,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>> // DBGDTR[TR]X_EL0 share the same encoding
>>> { SYS_DESC(SYS_DBGDTRTX_EL0), trap_raz_wi },
>>> 
>>> - { SYS_DESC(SYS_DBGVCR32_EL2), NULL, reset_val, DBGVCR32_EL2, 0 },
>>> + { SYS_DESC(SYS_DBGVCR32_EL2), trap_undef, reset_val, DBGVCR32_EL2, 0 },
>>> 
>>> { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
>>> 
>>> @@ -2380,18 +2380,18 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>> EL2_REG(VTTBR_EL2, access_rw, reset_val, 0),
>>> EL2_REG(VTCR_EL2, access_rw, reset_val, 0),
>>> 
>>> - { SYS_DESC(SYS_DACR32_EL2), NULL, reset_unknown, DACR32_EL2 },
>>> + { SYS_DESC(SYS_DACR32_EL2), trap_undef, reset_unknown, DACR32_EL2 },
>>> EL2_REG(HDFGRTR_EL2, access_rw, reset_val, 0),
>>> EL2_REG(HDFGWTR_EL2, access_rw, reset_val, 0),
>>> EL2_REG(SPSR_EL2, access_rw, reset_val, 0),
>>> EL2_REG(ELR_EL2, access_rw, reset_val, 0),
>>> { SYS_DESC(SYS_SP_EL1), access_sp_el1},
>>> 
>>> - { SYS_DESC(SYS_IFSR32_EL2), NULL, reset_unknown, IFSR32_EL2 },
>>> + { SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 },
>>> EL2_REG(AFSR0_EL2, access_rw, reset_val, 0),
>>> EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
>>> EL2_REG(ESR_EL2, access_rw, reset_val, 0),
>>> - { SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x700 },
>>> + { SYS_DESC(SYS_FPEXC32_EL2), trap_undef, reset_val, FPEXC32_EL2, 0x700 },
>>> 
>> 
>> Should SDER32_EL2 be considered to this same list?
>> 
> 
> This wouldn't make much sense.
> 
> This register is only available when running in secure mode, and KVM is firmly non-secure.
> 

Thanks for clarifying.

Miguel

> Thanks,
> 
>        M.
> 
> Jazz is not dead, it just smells funny