| Message ID | 20231013223311.3950585-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Do not let a L1 hypervisor access the *32_EL2 sysregs | expand |
On Fri, Oct 13, 2023 at 11:33:11PM +0100, Marc Zyngier 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> If you intend to send this as a fix for 6.6: Reviewed-by: Oliver Upton <oliver.upton@linux.dev> Otherwise it is on the stack of patches I'll pick up for 6.7
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? Thanks, Miguel > EL2_REG(FAR_EL2, access_rw, reset_val, 0), > EL2_REG(HPFAR_EL2, access_rw, reset_val, 0), > -- > 2.34.1 > >
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, M. Jazz is not dead, it just smells funny
Hi Marc, On 10/14/23 00:33, Marc Zyngier 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> Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric > --- > 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 }, > > EL2_REG(FAR_EL2, access_rw, reset_val, 0), > EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
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
On Sat, 14 Oct 2023 04:32:46 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Fri, Oct 13, 2023 at 11:33:11PM +0100, Marc Zyngier 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> > > If you intend to send this as a fix for 6.6: > > Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > > Otherwise it is on the stack of patches I'll pick up for 6.7 Thanks. I've queued it as a fix for now, together with Miguel's NV fix series. M.
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 }, EL2_REG(FAR_EL2, access_rw, reset_val, 0), EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
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(-)