On Wed, Aug 16, 2023 at 8:58 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> That branch builds (and looks) just fine on gcc-12 and clang-14. Maybe you have
> stale objects in your build directory? Or maybe PEBKAC?
Hmm, so it does- PEBKAC indeed...
> I was thinking that we couldn't have two anonymous unions, and so userspace (and
> KVM) would need to do something like
>
> run->exit2.memory_fault.gpa
>
> instead of
>
> run->memory_fault.gpa
>
> but the names just need to be unique, e.g. the below compiles just fine. So unless
> someone has a better idea, using a separate union for exits that might be clobbered
> seems like the way to go.
Agreed. By the way, what was the reason why you wanted to avoid the
exit reason canary being ABI?
On Wed, Jun 14, 2023 at 10:35 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> And rather than use kvm_run.exit_reason as the canary, we should carve out a
> kernel-only, i.e. non-ABI, field from the union. That would allow setting the
> canary in common KVM code, which can't be done for kvm_run.exit_reason because
> some architectures, e.g. s390 (and x86 IIRC), consume the exit_reason early on
> in KVM_RUN.
>
> E.g. something like this (the #ifdefs are heinous, it might be better to let
> userspace see the exit_canary, but make it abundantly clear that it's not ABI).
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 143abb334f56..233702124e0a 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -511,16 +511,43 @@ struct kvm_run {
> + /*
> + * This second KVM_EXIT_* union holds structures for exits that may be
> + * triggered after KVM has already initiated a different exit, and/or
> + * may be filled speculatively by KVM. E.g. because of limitations in
> + * KVM's uAPI, a memory fault can be encountered after an MMIO exit is
> + * initiated and kvm_run.mmio is filled. Isolating these structures
> + * from the primary KVM_EXIT_* union ensures that KVM won't clobber
> + * information for the original exit.
> + */
> + union {
> /* KVM_EXIT_MEMORY_FAULT */
> blahblahblah
> +#endif
> };
>
> +#ifdef __KERNEL__
> + /*
> + * Non-ABI, kernel-only field that KVM uses to detect bugs related to
> + * filling exit_reason and the exit unions, e.g. to guard against
> + * clobbering a previous exit.
> + */
> + __u64 exit_canary;
> +#endif
> +
We can't set exit_reason in the kvm_handle_guest_uaccess_fault()
helper if we're to handle case A (the setup vcpu exit -> fail guest
memory access -> return to userspace) correctly. But then userspace
needs some other way to check whether an efault is annotated, and
might as well check the canary, so something like
> + __u32 speculative_exit_reason;
> + union {
> + /* KVM_SPEC_EXIT_MEMORY_FAULT */
> + struct {
> + __u64 flags;
> + __u64 gpa;
> + __u64 len;
> + } memory_fault;
> + /* Fix the size of the union. */
> + char speculative_padding[256];
> + };
With the condition for an annotated efault being "if kvm_run returns
-EFAULT and speculative_exit_reason is..."
