* [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction [not found] <20241018194757.3685856-1-oliver.upton@linux.dev> @ 2024-10-18 19:47 ` Oliver Upton 2024-10-19 9:10 ` Marc Zyngier 0 siblings, 1 reply; 3+ messages in thread From: Oliver Upton @ 2024-10-18 19:47 UTC (permalink / raw) To: kvmarm Cc: Marc Zyngier, Joey Gouly, Suzuki K Poulose, Zenghui Yu, Oliver Upton, stable, Alexander Potapenko Returning an abort to the guest for an unsupported MMIO access is a documented feature of the KVM UAPI. Nevertheless, it's clear that this plumbing has seen limited testing, since userspace can trivially cause a WARN in the MMIO return: WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 Call trace: kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133 kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487 __do_sys_ioctl fs/ioctl.c:51 [inline] __se_sys_ioctl fs/ioctl.c:893 [inline] __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893 __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline] invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49 el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132 do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151 el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712 el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598 The splat is complaining that KVM is advancing PC while an exception is pending, i.e. that KVM is retiring the MMIO instruction despite a pending external abort. Womp womp. Fix the glaring UAPI bug by skipping over all the MMIO emulation in case there is a pending synchronous exception. Note that while userspace is capable of pending an asynchronous exception (SError, IRQ, or FIQ), it is still safe to retire the MMIO instruction in this case as (1) they are by definition asynchronous, and (2) KVM relies on hardware support for pending/delivering these exceptions instead of the software state machine for advancing PC. Cc: stable@vger.kernel.org Fixes: da345174ceca ("KVM: arm/arm64: Allow user injection of external data aborts") Reported-by: Alexander Potapenko <glider@google.com> Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- arch/arm64/include/asm/kvm_emulate.h | 25 +++++++++++++++++++++++++ arch/arm64/kvm/mmio.c | 7 +++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index a601a9305b10..1b229099f684 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -544,6 +544,31 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu) vcpu_set_flag((v), e); \ } while (0) +static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu) +{ + if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION)) + return false; + + if (vcpu_el1_is_32bit(vcpu)) { + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { + case unpack_vcpu_flag(EXCEPT_AA32_UND): + case unpack_vcpu_flag(EXCEPT_AA32_IABT): + case unpack_vcpu_flag(EXCEPT_AA32_DABT): + return true; + default: + return false; + } + } else { + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { + case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC): + case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC): + return true; + default: + return false; + } + } +} + #define __build_check_all_or_none(r, bits) \ BUILD_BUG_ON(((r) & (bits)) && ((r) & (bits)) != (bits)) diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c index cd6b7b83e2c3..0155ba665717 100644 --- a/arch/arm64/kvm/mmio.c +++ b/arch/arm64/kvm/mmio.c @@ -84,8 +84,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu) unsigned int len; int mask; - /* Detect an already handled MMIO return */ - if (unlikely(!vcpu->mmio_needed)) + /* + * Detect if the MMIO return was already handled or if userspace aborted + * the MMIO access. + */ + if (unlikely(!vcpu->mmio_needed || kvm_pending_sync_exception(vcpu))) return 1; vcpu->mmio_needed = 0; -- 2.47.0.rc1.288.g06298d1525-goog ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction 2024-10-18 19:47 ` [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction Oliver Upton @ 2024-10-19 9:10 ` Marc Zyngier 2024-10-19 18:13 ` Oliver Upton 0 siblings, 1 reply; 3+ messages in thread From: Marc Zyngier @ 2024-10-19 9:10 UTC (permalink / raw) To: Oliver Upton Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, stable, Alexander Potapenko On Fri, 18 Oct 2024 20:47:56 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > Returning an abort to the guest for an unsupported MMIO access is a > documented feature of the KVM UAPI. Nevertheless, it's clear that this > plumbing has seen limited testing, since userspace can trivially cause a > WARN in the MMIO return: > > WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 > Call trace: > kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 > kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133 > kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487 > __do_sys_ioctl fs/ioctl.c:51 [inline] > __se_sys_ioctl fs/ioctl.c:893 [inline] > __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893 > __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline] > invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49 > el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132 > do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151 > el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712 > el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730 > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598 > > The splat is complaining that KVM is advancing PC while an exception is > pending, i.e. that KVM is retiring the MMIO instruction despite a > pending external abort. Womp womp. nit: *synchronous* external abort. > > Fix the glaring UAPI bug by skipping over all the MMIO emulation in > case there is a pending synchronous exception. Note that while userspace > is capable of pending an asynchronous exception (SError, IRQ, or FIQ), > it is still safe to retire the MMIO instruction in this case as (1) they > are by definition asynchronous, and (2) KVM relies on hardware support > for pending/delivering these exceptions instead of the software state > machine for advancing PC. > > Cc: stable@vger.kernel.org > Fixes: da345174ceca ("KVM: arm/arm64: Allow user injection of external data aborts") > Reported-by: Alexander Potapenko <glider@google.com> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/arm64/include/asm/kvm_emulate.h | 25 +++++++++++++++++++++++++ > arch/arm64/kvm/mmio.c | 7 +++++-- > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index a601a9305b10..1b229099f684 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -544,6 +544,31 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu) > vcpu_set_flag((v), e); \ > } while (0) > > +static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu) > +{ > + if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION)) > + return false; > + > + if (vcpu_el1_is_32bit(vcpu)) { > + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { > + case unpack_vcpu_flag(EXCEPT_AA32_UND): > + case unpack_vcpu_flag(EXCEPT_AA32_IABT): > + case unpack_vcpu_flag(EXCEPT_AA32_DABT): > + return true; > + default: > + return false; > + } > + } else { > + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { > + case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC): > + case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC): > + return true; > + default: > + return false; > + } > + } > +} > + Is there any advantage in adding this to an otherwise unsuspecting include file, given that this is only used in a single spot? > #define __build_check_all_or_none(r, bits) \ > BUILD_BUG_ON(((r) & (bits)) && ((r) & (bits)) != (bits)) > > diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c > index cd6b7b83e2c3..0155ba665717 100644 > --- a/arch/arm64/kvm/mmio.c > +++ b/arch/arm64/kvm/mmio.c > @@ -84,8 +84,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu) > unsigned int len; > int mask; > > - /* Detect an already handled MMIO return */ > - if (unlikely(!vcpu->mmio_needed)) > + /* > + * Detect if the MMIO return was already handled or if userspace aborted > + * the MMIO access. > + */ > + if (unlikely(!vcpu->mmio_needed || kvm_pending_sync_exception(vcpu))) > return 1; > > vcpu->mmio_needed = 0; Otherwise looks good to me! Thanks, M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction 2024-10-19 9:10 ` Marc Zyngier @ 2024-10-19 18:13 ` Oliver Upton 0 siblings, 0 replies; 3+ messages in thread From: Oliver Upton @ 2024-10-19 18:13 UTC (permalink / raw) To: Marc Zyngier Cc: kvmarm, Joey Gouly, Suzuki K Poulose, Zenghui Yu, stable, Alexander Potapenko On Sat, Oct 19, 2024 at 10:10:04AM +0100, Marc Zyngier wrote: > On Fri, 18 Oct 2024 20:47:56 +0100, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > Returning an abort to the guest for an unsupported MMIO access is a > > documented feature of the KVM UAPI. Nevertheless, it's clear that this > > plumbing has seen limited testing, since userspace can trivially cause a > > WARN in the MMIO return: > > > > WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 > > Call trace: > > kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 > > kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133 > > kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487 > > __do_sys_ioctl fs/ioctl.c:51 [inline] > > __se_sys_ioctl fs/ioctl.c:893 [inline] > > __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893 > > __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline] > > invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49 > > el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132 > > do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151 > > el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712 > > el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730 > > el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598 > > > > The splat is complaining that KVM is advancing PC while an exception is > > pending, i.e. that KVM is retiring the MMIO instruction despite a > > pending external abort. Womp womp. > > nit: *synchronous* external abort. Doh! > > +static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu) > > +{ > > + if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION)) > > + return false; > > + > > + if (vcpu_el1_is_32bit(vcpu)) { > > + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { > > + case unpack_vcpu_flag(EXCEPT_AA32_UND): > > + case unpack_vcpu_flag(EXCEPT_AA32_IABT): > > + case unpack_vcpu_flag(EXCEPT_AA32_DABT): > > + return true; > > + default: > > + return false; > > + } > > + } else { > > + switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) { > > + case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC): > > + case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC): > > + return true; > > + default: > > + return false; > > + } > > + } > > +} > > + > > Is there any advantage in adding this to an otherwise unsuspecting > include file, given that this is only used in a single spot? v0 of this was a bit more involved, which is why I had this in a header. I'll move it. > Otherwise looks good to me! Thanks! -- Best, Oliver ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-19 18:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241018194757.3685856-1-oliver.upton@linux.dev>
2024-10-18 19:47 ` [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction Oliver Upton
2024-10-19 9:10 ` Marc Zyngier
2024-10-19 18:13 ` Oliver Upton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox