| Message ID | 20240529121251.1993135-6-ptosi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for hypervisor kCFI | expand |
On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote: > Use a name that expresses the fact that the routine might not exit > through the guest but will always (directly or indirectly) end up > executing hyp_panic(). > > Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic(). > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > --- > arch/arm64/kvm/hyp/entry.S | 6 +++--- > arch/arm64/kvm/hyp/hyp-entry.S | 2 +- > arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++-- > arch/arm64/kvm/hyp/nvhe/host.S | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) Hmm, I'm not sure about this. When is __guest_exit_panic() called outside of guest context? Will
On Mon, Jun 03, 2024 at 03:34:24PM +0100, Will Deacon wrote: > On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote: > > Use a name that expresses the fact that the routine might not exit > > through the guest but will always (directly or indirectly) end up > > executing hyp_panic(). > > > > Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic(). > > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > --- > > arch/arm64/kvm/hyp/entry.S | 6 +++--- > > arch/arm64/kvm/hyp/hyp-entry.S | 2 +- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++-- > > arch/arm64/kvm/hyp/nvhe/host.S | 4 ++-- > > 4 files changed, 8 insertions(+), 8 deletions(-) > > Hmm, I'm not sure about this. When is __guest_exit_panic() called outside > of guest context? AFAICT, it is also called from - the early __kvm_hyp_host_vector, installed by cpu_hyp_init_context() - the flavors of __kvm_hyp_vector, installed by cpu_hyp_init_features() which start handling exceptions long before the first guest can even be spawned. Hence __guest_exit_panic() needing to validate the context on entry. I don't get why those handlers didn't branch directly to hyp_panic() (perhaps to have a more robust flow?) but, as mentioned in [1], it is convenient for kCFI to be able to intercept all panic paths for sync exception from a single place. [1]: https://lore.kernel.org/kvm/qob5gnca2nte4ggkrnn4uil5mfbkz3p55lmk3egpxstnumixfr@lq7xomrhf6za/
On Tue, Jun 04, 2024 at 04:51:58PM +0100, Pierre-Clément Tosi wrote: > On Mon, Jun 03, 2024 at 03:34:24PM +0100, Will Deacon wrote: > > On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote: > > > Use a name that expresses the fact that the routine might not exit > > > through the guest but will always (directly or indirectly) end up > > > executing hyp_panic(). > > > > > > Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic(). > > > > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > > --- > > > arch/arm64/kvm/hyp/entry.S | 6 +++--- > > > arch/arm64/kvm/hyp/hyp-entry.S | 2 +- > > > arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++-- > > > arch/arm64/kvm/hyp/nvhe/host.S | 4 ++-- > > > 4 files changed, 8 insertions(+), 8 deletions(-) > > > > Hmm, I'm not sure about this. When is __guest_exit_panic() called outside > > of guest context? > > AFAICT, it is also called from > > - the early __kvm_hyp_host_vector, installed by cpu_hyp_init_context() Well, we've just agreed to remove that one :) > - the flavors of __kvm_hyp_vector, installed by cpu_hyp_init_features() cpu_hyp_init_features() doesn't actually plumb the vector into VBAR, though, so I still think that __guest_exit_panic() is only reachable in guest context. > which start handling exceptions long before the first guest can even be spawned. I don't see how :/ Will
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S index 4433a234aa9b..343851c17373 100644 --- a/arch/arm64/kvm/hyp/entry.S +++ b/arch/arm64/kvm/hyp/entry.S @@ -83,7 +83,7 @@ alternative_else_nop_endif eret sb -SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL) +SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL) // x2-x29,lr: vcpu regs // vcpu x0-x1 on the stack @@ -91,7 +91,7 @@ SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL) ldr x0, [x0, #CPU_ELR_EL2] msr elr_el2, x0 -SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) +SYM_INNER_LABEL(__hyp_panic, SYM_L_GLOBAL) // x2-x29,lr: vcpu regs // vcpu x0-x1 on the stack @@ -109,7 +109,7 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL) // accurate if the guest had been completely restored. adr_this_cpu x0, kvm_hyp_ctxt, x1 adr_l x1, hyp_panic - str x1, [x0, #CPU_XREG_OFFSET(30)] + str x1, [x0, #CPU_LR_OFFSET] get_vcpu_ptr x1, x0 diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S index 03f97d71984c..7e65ef738ec9 100644 --- a/arch/arm64/kvm/hyp/hyp-entry.S +++ b/arch/arm64/kvm/hyp/hyp-entry.S @@ -122,7 +122,7 @@ el2_error: eret sb -.macro invalid_vector label, target = __guest_exit_panic +.macro invalid_vector label, target = __hyp_panic .align 2 SYM_CODE_START_LOCAL(\label) b \target diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index ed9a63f1f7bf..d9931abf14c2 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -689,7 +689,7 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) static inline void __kvm_unexpected_el2_exception(void) { - extern char __guest_exit_restore_elr_and_panic[]; + extern char __hyp_restore_elr_and_panic[]; unsigned long addr, fixup; struct kvm_exception_table_entry *entry, *end; unsigned long elr_el2 = read_sysreg(elr_el2); @@ -712,7 +712,7 @@ static inline void __kvm_unexpected_el2_exception(void) /* Trigger a panic after restoring the hyp context. */ this_cpu_ptr(&kvm_hyp_ctxt)->sys_regs[ELR_EL2] = elr_el2; - write_sysreg(__guest_exit_restore_elr_and_panic, elr_el2); + write_sysreg(__hyp_restore_elr_and_panic, elr_el2); } #endif /* __ARM64_KVM_HYP_SWITCH_H__ */ diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S index bc0a73d9fcd0..a7db40a51e4a 100644 --- a/arch/arm64/kvm/hyp/nvhe/host.S +++ b/arch/arm64/kvm/hyp/nvhe/host.S @@ -214,7 +214,7 @@ SYM_FUNC_END(__host_hvc) .endm .macro host_el2_sync_vect - __host_el2_vect __guest_exit_panic + __host_el2_vect __hyp_panic .endm .macro invalid_host_el1_vect @@ -227,7 +227,7 @@ SYM_FUNC_END(__host_hvc) .endm .macro invalid_host_el2_vect - __host_el2_vect __guest_exit_panic + __host_el2_vect __hyp_panic .endm /*
Use a name that expresses the fact that the routine might not exit through the guest but will always (directly or indirectly) end up executing hyp_panic(). Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic(). Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> --- arch/arm64/kvm/hyp/entry.S | 6 +++--- arch/arm64/kvm/hyp/hyp-entry.S | 2 +- arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++-- arch/arm64/kvm/hyp/nvhe/host.S | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)