Re: [RFC PATCH] KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP
From: Michael Roth
Date: Tue Jul 18 2023 - 15:39:55 EST
On Mon, Jul 17, 2023 at 06:58:54PM -0700, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
>
> TDX KVM will use KVM_MEM_ENC_OP. Make struct sev_cmd common both for
> vendor backend, SEV and TDX, with rename. Make the struct common uABI for
> KVM_MEM_ENC_OP. TDX backend wants to return 64 bit error code instead of
> 32 bit. To keep ABI for SEV backend, use union to accommodate 64 bit
> member.
>
> Some data structures for sub-commands could be common. The current
> candidate would be KVM_SEV{,_ES}_INIT, KVM_SEV_LAUNCH_FINISH,
> KVM_SEV_LAUNCH_UPDATE_VMSA, KVM_SEV_DBG_DECRYPT, and KVM_SEV_DBG_ENCRYPT.
>
> Only compile tested for SEV code.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 2 +-
> arch/x86/include/uapi/asm/kvm.h | 22 +++++++++++
> arch/x86/kvm/svm/sev.c | 68 ++++++++++++++++++---------------
> arch/x86/kvm/svm/svm.h | 2 +-
> arch/x86/kvm/x86.c | 16 +++++++-
> 5 files changed, 76 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 28bd38303d70..f14c8df707ac 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1706,7 +1706,7 @@ struct kvm_x86_ops {
> void (*enable_smi_window)(struct kvm_vcpu *vcpu);
> #endif
>
> - int (*mem_enc_ioctl)(struct kvm *kvm, void __user *argp);
> + int (*mem_enc_ioctl)(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
> int (*mem_enc_register_region)(struct kvm *kvm, struct kvm_enc_region *argp);
> int (*mem_enc_unregister_region)(struct kvm *kvm, struct kvm_enc_region *argp);
> int (*vm_copy_enc_context_from)(struct kvm *kvm, unsigned int source_fd);
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 1a6a1f987949..c458c38bb0cb 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -562,4 +562,26 @@ struct kvm_pmu_event_filter {
> /* x86-specific KVM_EXIT_HYPERCALL flags. */
> #define KVM_EXIT_HYPERCALL_LONG_MODE BIT(0)
>
> +struct kvm_mem_enc_cmd {
> + /* sub-command id of KVM_MEM_ENC_OP. */
> + __u32 id;
> + /* Auxiliary flags for sub-command. */
> + __u32 flags;
struct kvm_sev_cmd doesn't have this flags field, so this would break for
older userspaces that try to pass it in instead of the struct kvm_mem_enc_cmd
proposed by this patch. Maybe move it to the end of the struct? Or
make it part of a TDX-specific union field.
But then you might also run into issues if you copy_to_user() with
sizeof(struct kvm_mem_enc_cmd) instead of sizeof(struct kvm_sev_cmd),
since the former might copy an additional 4 bytes more than what userspace
allocated.
So maybe only common bits should be copy_to_user()'d by common KVM code,
and the platform-specific fields in the union should be separately copied
by platform code?
E.g.
struct kvm_mem_enc_sev_cmd {
__u32 error;
__u32 sev_fd;
}
struct kvm_mem_enc_tdx_cmd {
__u64 error;
__u32 flags;
}
struct kvm_mem_enc_cmd {
__u32 id;
__u64 data;
union {
struct kvm_mem_enc_sev_cmd sev_cmd;
struct kvm_mem_enc_tdx_cmd tdx_cmd;
}
};
But then we'd need to copy_from_user() for common header, then for
platform-specific sub-command metadata like sev_fd, then for the
sub-command-specific parameters themselves.
Make me wonder if this warrants a KVM_MEM_ENC_OP2 (or whatever) that
uses the new structure from the start so that legacy constaints aren't
an issue.
-Mike
> + /* Data for sub-command. Typically __user pointer to actual parameter. */
> + __u64 data;
> + /* Supplemental error code in error case. */
> + union {
> + struct {
> + __u32 error;
> + /*
> + * KVM_SEV_LAUNCH_START and KVM_SEV_RECEIVE_START
> + * require extra data. Not included in struct
> + * kvm_sev_launch_start or struct kvm_sev_receive_start.
> + */
> + __u32 sev_fd;
> + };
> + __u64 error64;
> + };
> +};
> +
> #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 07756b7348ae..94e13bb49c86 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1835,30 +1835,39 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
> return ret;
> }
>
> -int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> +int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd)
> {
> - struct kvm_sev_cmd sev_cmd;
> + struct kvm_sev_cmd *sev_cmd = (struct kvm_sev_cmd *)cmd;
> int r;
>
> + /* TODO: replace struct kvm_sev_cmd with kvm_mem_enc_cmd. */
> + BUILD_BUG_ON(sizeof(*sev_cmd) != sizeof(*cmd));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, id) !=
> + offsetof(struct kvm_mem_enc_cmd, id));
> + BUILD_BUG_ON(sizeof(sev_cmd->id) != sizeof(cmd->id));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, data) !=
> + offsetof(struct kvm_mem_enc_cmd, data));
> + BUILD_BUG_ON(sizeof(sev_cmd->data) != sizeof(cmd->data));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, error) !=
> + offsetof(struct kvm_mem_enc_cmd, error));
> + BUILD_BUG_ON(sizeof(sev_cmd->error) != sizeof(cmd->error));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, sev_fd) !=
> + offsetof(struct kvm_mem_enc_cmd, sev_fd));
> + BUILD_BUG_ON(sizeof(sev_cmd->sev_fd) != sizeof(cmd->sev_fd));
> +
> if (!sev_enabled)
> return -ENOTTY;
>
> - if (!argp)
> - return 0;
> -
> - if (copy_from_user(&sev_cmd, argp, sizeof(struct kvm_sev_cmd)))
> - return -EFAULT;
> -
> mutex_lock(&kvm->lock);
>
> /* Only the enc_context_owner handles some memory enc operations. */
> if (is_mirroring_enc_context(kvm) &&
> - !is_cmd_allowed_from_mirror(sev_cmd.id)) {
> + !is_cmd_allowed_from_mirror(sev_cmd->id)) {
> r = -EINVAL;
> goto out;
> }
>
> - switch (sev_cmd.id) {
> + switch (sev_cmd->id) {
> case KVM_SEV_ES_INIT:
> if (!sev_es_enabled) {
> r = -ENOTTY;
> @@ -1866,67 +1875,64 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> }
> fallthrough;
> case KVM_SEV_INIT:
> - r = sev_guest_init(kvm, &sev_cmd);
> + r = sev_guest_init(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_START:
> - r = sev_launch_start(kvm, &sev_cmd);
> + r = sev_launch_start(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_UPDATE_DATA:
> - r = sev_launch_update_data(kvm, &sev_cmd);
> + r = sev_launch_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_UPDATE_VMSA:
> - r = sev_launch_update_vmsa(kvm, &sev_cmd);
> + r = sev_launch_update_vmsa(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_MEASURE:
> - r = sev_launch_measure(kvm, &sev_cmd);
> + r = sev_launch_measure(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_FINISH:
> - r = sev_launch_finish(kvm, &sev_cmd);
> + r = sev_launch_finish(kvm, sev_cmd);
> break;
> case KVM_SEV_GUEST_STATUS:
> - r = sev_guest_status(kvm, &sev_cmd);
> + r = sev_guest_status(kvm, sev_cmd);
> break;
> case KVM_SEV_DBG_DECRYPT:
> - r = sev_dbg_crypt(kvm, &sev_cmd, true);
> + r = sev_dbg_crypt(kvm, sev_cmd, true);
> break;
> case KVM_SEV_DBG_ENCRYPT:
> - r = sev_dbg_crypt(kvm, &sev_cmd, false);
> + r = sev_dbg_crypt(kvm, sev_cmd, false);
> break;
> case KVM_SEV_LAUNCH_SECRET:
> - r = sev_launch_secret(kvm, &sev_cmd);
> + r = sev_launch_secret(kvm, sev_cmd);
> break;
> case KVM_SEV_GET_ATTESTATION_REPORT:
> - r = sev_get_attestation_report(kvm, &sev_cmd);
> + r = sev_get_attestation_report(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_START:
> - r = sev_send_start(kvm, &sev_cmd);
> + r = sev_send_start(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_UPDATE_DATA:
> - r = sev_send_update_data(kvm, &sev_cmd);
> + r = sev_send_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_FINISH:
> - r = sev_send_finish(kvm, &sev_cmd);
> + r = sev_send_finish(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_CANCEL:
> - r = sev_send_cancel(kvm, &sev_cmd);
> + r = sev_send_cancel(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_START:
> - r = sev_receive_start(kvm, &sev_cmd);
> + r = sev_receive_start(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_UPDATE_DATA:
> - r = sev_receive_update_data(kvm, &sev_cmd);
> + r = sev_receive_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_FINISH:
> - r = sev_receive_finish(kvm, &sev_cmd);
> + r = sev_receive_finish(kvm, sev_cmd);
> break;
> default:
> r = -EINVAL;
> goto out;
> }
>
> - if (copy_to_user(argp, &sev_cmd, sizeof(struct kvm_sev_cmd)))
> - r = -EFAULT;
> -
> out:
> mutex_unlock(&kvm->lock);
> return r;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 18af7e712a5a..74ecab20c24b 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -716,7 +716,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
> extern unsigned int max_sev_asid;
>
> void sev_vm_destroy(struct kvm *kvm);
> -int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp);
> +int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
> int sev_mem_enc_register_region(struct kvm *kvm,
> struct kvm_enc_region *range);
> int sev_mem_enc_unregister_region(struct kvm *kvm,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a6b9bea62fb8..14cfbc3266dd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7018,11 +7018,25 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> goto out;
> }
> case KVM_MEMORY_ENCRYPT_OP: {
> + struct kvm_mem_enc_cmd cmd;
> +
> r = -ENOTTY;
> if (!kvm_x86_ops.mem_enc_ioctl)
> goto out;
>
> - r = static_call(kvm_x86_mem_enc_ioctl)(kvm, argp);
> + if (!argp) {
> + r = 0;
> + goto out;
> + }
> +
> + if (copy_from_user(&cmd, argp, sizeof(cmd))) {
> + r = -EFAULT;
> + goto out;
> + }
> + r = static_call(kvm_x86_mem_enc_ioctl)(kvm, &cmd);
> + if (copy_to_user(argp, &cmd, sizeof(cmd)))
> + r = -EFAULT;
> +
> break;
> }
> case KVM_MEMORY_ENCRYPT_REG_REGION: {
>
> base-commit: 831fe284d8275987596b7d640518dddba5735f61
> --
> 2.25.1
>