| Message ID | 147190849706.9523.17127624683768628621.stgit@brijesh-build-machine (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/08/2016 01:28, Brijesh Singh wrote: > The ioctl will be used by qemu to issue the Secure Encrypted > Virtualization (SEV) guest commands to transition a guest into > into SEV-enabled mode. > > a typical usage: > > struct kvm_sev_launch_start start; > struct kvm_sev_issue_cmd data; > > data.cmd = KVM_SEV_LAUNCH_START; > data.opaque = &start; > > ret = ioctl(fd, KVM_SEV_ISSUE_CMD, &data); > > On SEV command failure, data.ret_code will contain the firmware error code. Please modify the ioctl to require the file descriptor for the PSP. A program without access to /dev/psp should not be able to use SEV. > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/kvm_host.h | 3 + > arch/x86/kvm/x86.c | 13 ++++ > include/uapi/linux/kvm.h | 125 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 141 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 9b885fc..a94e37d 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1040,6 +1040,9 @@ struct kvm_x86_ops { > void (*cancel_hv_timer)(struct kvm_vcpu *vcpu); > > void (*setup_mce)(struct kvm_vcpu *vcpu); > + > + int (*sev_issue_cmd)(struct kvm *kvm, > + struct kvm_sev_issue_cmd __user *argp); > }; > > struct kvm_arch_async_pf { > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d6f2f4b..0c0adad 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3820,6 +3820,15 @@ split_irqchip_unlock: > return r; > } > > +static int kvm_vm_ioctl_sev_issue_cmd(struct kvm *kvm, > + struct kvm_sev_issue_cmd __user *argp) > +{ > + if (kvm_x86_ops->sev_issue_cmd) > + return kvm_x86_ops->sev_issue_cmd(kvm, argp); > + > + return -ENOTTY; > +} Please make a more generic vm_ioctl callback. > long kvm_arch_vm_ioctl(struct file *filp, > unsigned int ioctl, unsigned long arg) > { > @@ -4085,6 +4094,10 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = kvm_vm_ioctl_enable_cap(kvm, &cap); > break; > } > + case KVM_SEV_ISSUE_CMD: { > + r = kvm_vm_ioctl_sev_issue_cmd(kvm, argp); > + break; > + } > default: > r = kvm_vm_ioctl_assigned_device(kvm, ioctl, arg); > } > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 300ef25..72c18c3 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1274,6 +1274,131 @@ struct kvm_s390_ucas_mapping { > /* Available with KVM_CAP_X86_SMM */ > #define KVM_SMI _IO(KVMIO, 0xb7) > > +/* Secure Encrypted Virtualization mode */ > +enum sev_cmd { > + KVM_SEV_LAUNCH_START = 0, > + KVM_SEV_LAUNCH_UPDATE, > + KVM_SEV_LAUNCH_FINISH, > + KVM_SEV_GUEST_STATUS, > + KVM_SEV_DBG_DECRYPT, > + KVM_SEV_DBG_ENCRYPT, > + KVM_SEV_RECEIVE_START, > + KVM_SEV_RECEIVE_UPDATE, > + KVM_SEV_RECEIVE_FINISH, > + KVM_SEV_SEND_START, > + KVM_SEV_SEND_UPDATE, > + KVM_SEV_SEND_FINISH, > + KVM_SEV_API_VERSION, > + KVM_SEV_NR_MAX, > +}; > + > +struct kvm_sev_issue_cmd { > + __u32 cmd; > + __u64 opaque; > + __u32 ret_code; > +}; > + > +struct kvm_sev_launch_start { > + __u32 handle; > + __u32 flags; > + __u32 policy; > + __u8 nonce[16]; > + __u8 dh_pub_qx[32]; > + __u8 dh_pub_qy[32]; > +}; > + > +struct kvm_sev_launch_update { > + __u64 address; > + __u32 length; > +}; > + > +struct kvm_sev_launch_finish { > + __u32 vcpu_count; > + __u32 vcpu_length; > + __u64 vcpu_mask_addr; > + __u32 vcpu_mask_length; > + __u8 measurement[32]; > +}; > + > +struct kvm_sev_guest_status { > + __u32 policy; > + __u32 state; > +}; > + > +struct kvm_sev_dbg_decrypt { > + __u64 src_addr; > + __u64 dst_addr; > + __u32 length; > +}; > + > +struct kvm_sev_dbg_encrypt { > + __u64 src_addr; > + __u64 dst_addr; > + __u32 length; > +}; > + > +struct kvm_sev_receive_start { > + __u32 handle; > + __u32 flags; > + __u32 policy; > + __u8 policy_meas[32]; > + __u8 wrapped_tek[24]; > + __u8 wrapped_tik[24]; > + __u8 ten[16]; > + __u8 dh_pub_qx[32]; > + __u8 dh_pub_qy[32]; > + __u8 nonce[16]; > +}; > + > +struct kvm_sev_receive_update { > + __u8 iv[16]; > + __u64 address; > + __u32 length; > +}; > + > +struct kvm_sev_receive_finish { > + __u8 measurement[32]; > +}; > + > +struct kvm_sev_send_start { > + __u8 nonce[16]; > + __u32 policy; > + __u8 policy_meas[32]; > + __u8 wrapped_tek[24]; > + __u8 wrapped_tik[24]; > + __u8 ten[16]; > + __u8 iv[16]; > + __u32 flags; > + __u8 api_major; > + __u8 api_minor; > + __u32 serial; > + __u8 dh_pub_qx[32]; > + __u8 dh_pub_qy[32]; > + __u8 pek_sig_r[32]; > + __u8 pek_sig_s[32]; > + __u8 cek_sig_r[32]; > + __u8 cek_sig_s[32]; > + __u8 cek_pub_qx[32]; > + __u8 cek_pub_qy[32]; > + __u8 ask_sig_r[32]; > + __u8 ask_sig_s[32]; > + __u32 ncerts; > + __u32 cert_length; > + __u64 certs_addr; > +}; > + > +struct kvm_sev_send_update { > + __u32 length; > + __u64 src_addr; > + __u64 dst_addr; > +}; > + > +struct kvm_sev_send_finish { > + __u8 measurement[32]; > +}; > + > +#define KVM_SEV_ISSUE_CMD _IOWR(KVMIO, 0xb8, struct kvm_sev_issue_cmd) > + > #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) > #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) > #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2) > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paolo, On 10/13/2016 05:45 AM, Paolo Bonzini wrote: > > > On 23/08/2016 01:28, Brijesh Singh wrote: >> The ioctl will be used by qemu to issue the Secure Encrypted >> Virtualization (SEV) guest commands to transition a guest into >> into SEV-enabled mode. >> >> a typical usage: >> >> struct kvm_sev_launch_start start; >> struct kvm_sev_issue_cmd data; >> >> data.cmd = KVM_SEV_LAUNCH_START; >> data.opaque = &start; >> >> ret = ioctl(fd, KVM_SEV_ISSUE_CMD, &data); >> >> On SEV command failure, data.ret_code will contain the firmware error code. > > Please modify the ioctl to require the file descriptor for the PSP. A > program without access to /dev/psp should not be able to use SEV. > I am not sure if I fully understand this feedback. Let me summaries what we have right now. At highest level SEV key management commands are divided into two sections: - platform management : commands used during platform provisioning. PSP drv provides ioctl's for these commands. Qemu will not use these ioctl's, i believe these ioctl will be used by other tools. - guest management: command used during guest life cycle. PSP drv exports various function and KVM drv calls these function when it receives the SEV_ISSUE_CMD ioctl from qemu. If I understanding correctly then you are recommending that instead of exporting various functions from PSP drv we should expose one function for the all the guest command handling (something like this). int psp_issue_cmd_external_user(struct file *filep, int cmd, unsigned long addr, int *psp_ret) { /* here we check to ensure that file->f_ops is a valid * psp instance. */ if (filep->f_ops != &psp_fops) return -EINVAL; /* handle the command */ return psp_issue_cmd (cmd, addr, timeout, psp_ret); } In KVM driver use something like this to invoke the PSP command handler. int kvm_sev_psp_cmd (struct kvm_sev_issue_cmd *input, unsigned long data) { int ret; struct fd f; f = fdget(input->psp_fd); if (!f.file) return -EBADF; .... psp_issue_cmd_external_user(f.file, input->cmd, data, &input->psp_ret); .... } Please let me know if I understood this correctly. >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/include/asm/kvm_host.h | 3 + >> arch/x86/kvm/x86.c | 13 ++++ >> include/uapi/linux/kvm.h | 125 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 141 insertions(+) >> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I am not sure if I fully understand this feedback. Let me summaries what > we have right now. > > At highest level SEV key management commands are divided into two sections: > > - platform management : commands used during platform provisioning. PSP > drv provides ioctl's for these commands. Qemu will not use these > ioctl's, i believe these ioctl will be used by other tools. > > - guest management: command used during guest life cycle. PSP drv > exports various function and KVM drv calls these function when it > receives the SEV_ISSUE_CMD ioctl from qemu. > > If I understanding correctly then you are recommending that instead of > exporting various functions from PSP drv we should expose one function > for the all the guest command handling (something like this). My understanding is that a user could exhaust the ASIDs for encrypted VMs if it was allowed to start an arbitrary number of KVM guests. So we would need some kind of control. Is this correct? If so, does /dev/psp provide any functionality that you believe is dangerous for the KVM userspace (which runs in a very confined environment)? Is this functionality blocked through capability checks? Thanks, Paolo > int psp_issue_cmd_external_user(struct file *filep, > int cmd, unsigned long addr, > int *psp_ret) > { > /* here we check to ensure that file->f_ops is a valid > * psp instance. > */ > if (filep->f_ops != &psp_fops) > return -EINVAL; > > /* handle the command */ > return psp_issue_cmd (cmd, addr, timeout, psp_ret); > } > > In KVM driver use something like this to invoke the PSP command handler. > > int kvm_sev_psp_cmd (struct kvm_sev_issue_cmd *input, > unsigned long data) > { > int ret; > struct fd f; > > f = fdget(input->psp_fd); > if (!f.file) > return -EBADF; > .... > > psp_issue_cmd_external_user(f.file, input->cmd, > data, &input->psp_ret); > .... > } > > Please let me know if I understood this correctly. > > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> --- > >> arch/x86/include/asm/kvm_host.h | 3 + > >> arch/x86/kvm/x86.c | 13 ++++ > >> include/uapi/linux/kvm.h | 125 > >> +++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 141 insertions(+) > >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paolo, On 10/17/2016 03:14 PM, Paolo Bonzini wrote: >> I am not sure if I fully understand this feedback. Let me summaries what >> we have right now. >> >> At highest level SEV key management commands are divided into two sections: >> >> - platform management : commands used during platform provisioning. PSP >> drv provides ioctl's for these commands. Qemu will not use these >> ioctl's, i believe these ioctl will be used by other tools. >> >> - guest management: command used during guest life cycle. PSP drv >> exports various function and KVM drv calls these function when it >> receives the SEV_ISSUE_CMD ioctl from qemu. >> >> If I understanding correctly then you are recommending that instead of >> exporting various functions from PSP drv we should expose one function >> for the all the guest command handling (something like this). > > My understanding is that a user could exhaust the ASIDs for encrypted > VMs if it was allowed to start an arbitrary number of KVM guests. So > we would need some kind of control. Is this correct? > Yes, there is limited number of ASIDs for encrypted VMs. Do we need to pass the psp_fd into SEV_ISSUE_CMD ioctl or can we handle it from Qemu itself ? e.g when user asks to transition a guest into SEV-enabled mode then before calling LAUNCH_START Qemu tries to open /dev/psp device. If open() returns success then we know user has permission to communicate with PSP firmware. Please let me know if I am missing something. > If so, does /dev/psp provide any functionality that you believe is > dangerous for the KVM userspace (which runs in a very confined > environment)? Is this functionality blocked through capability > checks? > I do not see /dev/psp providing anything which would be dangerous to KVM userspace. It should be safe to access /dev/psp into KVM userspace. > Thanks, > > Paolo > > >> int psp_issue_cmd_external_user(struct file *filep, >> int cmd, unsigned long addr, >> int *psp_ret) >> { >> /* here we check to ensure that file->f_ops is a valid >> * psp instance. >> */ >> if (filep->f_ops != &psp_fops) >> return -EINVAL; >> >> /* handle the command */ >> return psp_issue_cmd (cmd, addr, timeout, psp_ret); >> } >> >> In KVM driver use something like this to invoke the PSP command handler. >> >> int kvm_sev_psp_cmd (struct kvm_sev_issue_cmd *input, >> unsigned long data) >> { >> int ret; >> struct fd f; >> >> f = fdget(input->psp_fd); >> if (!f.file) >> return -EBADF; >> .... >> >> psp_issue_cmd_external_user(f.file, input->cmd, >> data, &input->psp_ret); >> .... >> } >> >> Please let me know if I understood this correctly. >> >>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>>> --- >>>> arch/x86/include/asm/kvm_host.h | 3 + >>>> arch/x86/kvm/x86.c | 13 ++++ >>>> include/uapi/linux/kvm.h | 125 >>>> +++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 141 insertions(+) >>>> >> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > If I understanding correctly then you are recommending that instead of > > exporting various functions from PSP drv we should expose one function > > for the all the guest command handling (something like this). > > > > My understanding is that a user could exhaust the ASIDs for encrypted > > VMs if it was allowed to start an arbitrary number of KVM guests. So > > we would need some kind of control. Is this correct? > > Yes, there is limited number of ASIDs for encrypted VMs. Do we need to > pass the psp_fd into SEV_ISSUE_CMD ioctl or can we handle it from Qemu > itself ? e.g when user asks to transition a guest into SEV-enabled mode > then before calling LAUNCH_START Qemu tries to open /dev/psp device. If > open() returns success then we know user has permission to communicate > with PSP firmware. No, this is a stateful mechanism and it's hard to implement. Passing a /dev/psp file descriptor is the simplest way to "prove" that you have access to the device. Thanks, Paolo > > If so, does /dev/psp provide any functionality that you believe is > > dangerous for the KVM userspace (which runs in a very confined > > environment)? Is this functionality blocked through capability > > checks? > > I do not see /dev/psp providing anything which would be dangerous to KVM > userspace. It should be safe to access /dev/psp into KVM userspace. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 9b885fc..a94e37d 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1040,6 +1040,9 @@ struct kvm_x86_ops { void (*cancel_hv_timer)(struct kvm_vcpu *vcpu); void (*setup_mce)(struct kvm_vcpu *vcpu); + + int (*sev_issue_cmd)(struct kvm *kvm, + struct kvm_sev_issue_cmd __user *argp); }; struct kvm_arch_async_pf { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index d6f2f4b..0c0adad 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3820,6 +3820,15 @@ split_irqchip_unlock: return r; } +static int kvm_vm_ioctl_sev_issue_cmd(struct kvm *kvm, + struct kvm_sev_issue_cmd __user *argp) +{ + if (kvm_x86_ops->sev_issue_cmd) + return kvm_x86_ops->sev_issue_cmd(kvm, argp); + + return -ENOTTY; +} + long kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg) { @@ -4085,6 +4094,10 @@ long kvm_arch_vm_ioctl(struct file *filp, r = kvm_vm_ioctl_enable_cap(kvm, &cap); break; } + case KVM_SEV_ISSUE_CMD: { + r = kvm_vm_ioctl_sev_issue_cmd(kvm, argp); + break; + } default: r = kvm_vm_ioctl_assigned_device(kvm, ioctl, arg); } diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 300ef25..72c18c3 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1274,6 +1274,131 @@ struct kvm_s390_ucas_mapping { /* Available with KVM_CAP_X86_SMM */ #define KVM_SMI _IO(KVMIO, 0xb7) +/* Secure Encrypted Virtualization mode */ +enum sev_cmd { + KVM_SEV_LAUNCH_START = 0, + KVM_SEV_LAUNCH_UPDATE, + KVM_SEV_LAUNCH_FINISH, + KVM_SEV_GUEST_STATUS, + KVM_SEV_DBG_DECRYPT, + KVM_SEV_DBG_ENCRYPT, + KVM_SEV_RECEIVE_START, + KVM_SEV_RECEIVE_UPDATE, + KVM_SEV_RECEIVE_FINISH, + KVM_SEV_SEND_START, + KVM_SEV_SEND_UPDATE, + KVM_SEV_SEND_FINISH, + KVM_SEV_API_VERSION, + KVM_SEV_NR_MAX, +}; + +struct kvm_sev_issue_cmd { + __u32 cmd; + __u64 opaque; + __u32 ret_code; +}; + +struct kvm_sev_launch_start { + __u32 handle; + __u32 flags; + __u32 policy; + __u8 nonce[16]; + __u8 dh_pub_qx[32]; + __u8 dh_pub_qy[32]; +}; + +struct kvm_sev_launch_update { + __u64 address; + __u32 length; +}; + +struct kvm_sev_launch_finish { + __u32 vcpu_count; + __u32 vcpu_length; + __u64 vcpu_mask_addr; + __u32 vcpu_mask_length; + __u8 measurement[32]; +}; + +struct kvm_sev_guest_status { + __u32 policy; + __u32 state; +}; + +struct kvm_sev_dbg_decrypt { + __u64 src_addr; + __u64 dst_addr; + __u32 length; +}; + +struct kvm_sev_dbg_encrypt { + __u64 src_addr; + __u64 dst_addr; + __u32 length; +}; + +struct kvm_sev_receive_start { + __u32 handle; + __u32 flags; + __u32 policy; + __u8 policy_meas[32]; + __u8 wrapped_tek[24]; + __u8 wrapped_tik[24]; + __u8 ten[16]; + __u8 dh_pub_qx[32]; + __u8 dh_pub_qy[32]; + __u8 nonce[16]; +}; + +struct kvm_sev_receive_update { + __u8 iv[16]; + __u64 address; + __u32 length; +}; + +struct kvm_sev_receive_finish { + __u8 measurement[32]; +}; + +struct kvm_sev_send_start { + __u8 nonce[16]; + __u32 policy; + __u8 policy_meas[32]; + __u8 wrapped_tek[24]; + __u8 wrapped_tik[24]; + __u8 ten[16]; + __u8 iv[16]; + __u32 flags; + __u8 api_major; + __u8 api_minor; + __u32 serial; + __u8 dh_pub_qx[32]; + __u8 dh_pub_qy[32]; + __u8 pek_sig_r[32]; + __u8 pek_sig_s[32]; + __u8 cek_sig_r[32]; + __u8 cek_sig_s[32]; + __u8 cek_pub_qx[32]; + __u8 cek_pub_qy[32]; + __u8 ask_sig_r[32]; + __u8 ask_sig_s[32]; + __u32 ncerts; + __u32 cert_length; + __u64 certs_addr; +}; + +struct kvm_sev_send_update { + __u32 length; + __u64 src_addr; + __u64 dst_addr; +}; + +struct kvm_sev_send_finish { + __u8 measurement[32]; +}; + +#define KVM_SEV_ISSUE_CMD _IOWR(KVMIO, 0xb8, struct kvm_sev_issue_cmd) + #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
The ioctl will be used by qemu to issue the Secure Encrypted Virtualization (SEV) guest commands to transition a guest into into SEV-enabled mode. a typical usage: struct kvm_sev_launch_start start; struct kvm_sev_issue_cmd data; data.cmd = KVM_SEV_LAUNCH_START; data.opaque = &start; ret = ioctl(fd, KVM_SEV_ISSUE_CMD, &data); On SEV command failure, data.ret_code will contain the firmware error code. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/kvm_host.h | 3 + arch/x86/kvm/x86.c | 13 ++++ include/uapi/linux/kvm.h | 125 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html