| Message ID | 20210602140416.23573-23-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Wed, Jun 02, 2021 at 09:04:16AM -0500, Brijesh Singh wrote: > SEV-SNP specification provides the guest a mechanism to communicate with > the PSP without risk from a malicious hypervisor who wishes to read, alter, > drop or replay the messages sent. The driver uses snp_issue_guest_request() > to issue GHCB SNP_GUEST_REQUEST NAE event. This command constructs a > trusted channel between the guest and the PSP firmware. > > The userspace can use the following ioctls provided by the driver: > > 1. Request an attestation report that can be used to assume the identity > and security configuration of the guest. > 2. Ask the firmware to provide a key derived from a root key. > > See SEV-SNP spec section Guest Messages for more details. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/virt/Kconfig | 3 + > drivers/virt/Makefile | 1 + > drivers/virt/sevguest/Kconfig | 10 + > drivers/virt/sevguest/Makefile | 4 + > drivers/virt/sevguest/snp.c | 448 +++++++++++++++++++++++++++++++++ > drivers/virt/sevguest/snp.h | 63 +++++ > include/uapi/linux/sev-guest.h | 56 +++++ > 7 files changed, 585 insertions(+) > create mode 100644 drivers/virt/sevguest/Kconfig > create mode 100644 drivers/virt/sevguest/Makefile > create mode 100644 drivers/virt/sevguest/snp.c > create mode 100644 drivers/virt/sevguest/snp.h > create mode 100644 include/uapi/linux/sev-guest.h Seeing how there are a bunch of such driver things for SEV stuff, I'd say to put it under: drivers/virt/coco/ where we can collect all those confidential computing supporting drivers. > > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig > index 8061e8ef449f..4de714c5ee9a 100644 > --- a/drivers/virt/Kconfig > +++ b/drivers/virt/Kconfig > @@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig" > source "drivers/virt/nitro_enclaves/Kconfig" > > source "drivers/virt/acrn/Kconfig" > + > +source "drivers/virt/sevguest/Kconfig" > + > endif > diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile > index 3e272ea60cd9..b2d1a8131c90 100644 > --- a/drivers/virt/Makefile > +++ b/drivers/virt/Makefile > @@ -8,3 +8,4 @@ obj-y += vboxguest/ > > obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ > obj-$(CONFIG_ACRN_HSM) += acrn/ > +obj-$(CONFIG_SEV_GUEST) += sevguest/ > diff --git a/drivers/virt/sevguest/Kconfig b/drivers/virt/sevguest/Kconfig > new file mode 100644 > index 000000000000..e88a85527bf6 > --- /dev/null > +++ b/drivers/virt/sevguest/Kconfig > @@ -0,0 +1,10 @@ > +config SEV_GUEST > + tristate "AMD SEV Guest driver" > + default y > + depends on AMD_MEM_ENCRYPT > + help > + Provides AMD SNP guest request driver. The driver can be used by the s/Provides AMD SNP guest request driver. // > + guest to communicate with the hypervisor to request the attestation report to communicate with the PSP, I thought, not the hypervisor? > + and more. > + > + If you choose 'M' here, this module will be called sevguest. > diff --git a/drivers/virt/sevguest/Makefile b/drivers/virt/sevguest/Makefile > new file mode 100644 > index 000000000000..1505df437682 > --- /dev/null > +++ b/drivers/virt/sevguest/Makefile > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +sevguest-y := snp.o What's that for? Why isn't the filename simply called: drivers/virt/coco/sevguest.c ? Or is more coming? And below there's .name = "snp-guest", so you need to get the naming in order here. > +obj-$(CONFIG_SEV_GUEST) += sevguest.o > diff --git a/drivers/virt/sevguest/snp.c b/drivers/virt/sevguest/snp.c > new file mode 100644 > index 000000000000..00d8e8fddf2c > --- /dev/null > +++ b/drivers/virt/sevguest/snp.c > @@ -0,0 +1,448 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * AMD Secure Encrypted Virtualization Nested Paging (SEV-SNP) guest request interface > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/types.h> > +#include <linux/mutex.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/miscdevice.h> > +#include <linux/set_memory.h> > +#include <linux/fs.h> > +#include <crypto/aead.h> > +#include <linux/scatterlist.h> > +#include <linux/sev-guest.h> > +#include <uapi/linux/sev-guest.h> > + > +#include "snp.h" > + > +#define DEVICE_NAME "sev-guest" > +#define AAD_LEN 48 > +#define MSG_HDR_VER 1 > + > +struct snp_guest_crypto { > + struct crypto_aead *tfm; > + uint8_t *iv, *authtag; > + int iv_len, a_len; > +}; > + > +struct snp_guest_dev { > + struct device *dev; > + struct miscdevice misc; > + > + struct snp_guest_crypto *crypto; > + struct snp_guest_msg *request, *response; > +}; > + > +static DEFINE_MUTEX(snp_cmd_mutex); > + > +static inline struct snp_guest_dev *to_snp_dev(struct file *file) > +{ > + struct miscdevice *dev = file->private_data; > + > + return container_of(dev, struct snp_guest_dev, misc); > +} > + > +static struct snp_guest_crypto *init_crypto(struct snp_guest_dev *snp_dev, uint8_t *key, > + size_t keylen) > +{ > + struct snp_guest_crypto *crypto; > + > + crypto = kzalloc(sizeof(*crypto), GFP_KERNEL_ACCOUNT); > + if (!crypto) > + return NULL; > + > + crypto->tfm = crypto_alloc_aead("gcm(aes)", 0, 0); I know that it is hard to unselect CONFIG_CRYPTO_AEAD2 which provides this but you better depend on it in the Makefile so that some random config still builds. > + if (IS_ERR(crypto->tfm)) > + goto e_free; > + > + if (crypto_aead_setkey(crypto->tfm, key, keylen)) > + goto e_free_crypto; > + > + crypto->iv_len = crypto_aead_ivsize(crypto->tfm); > + if (crypto->iv_len < 12) { > + dev_err(snp_dev->dev, "IV length is less than 12.\n"); > + goto e_free_crypto; > + } > + > + crypto->iv = kmalloc(crypto->iv_len, GFP_KERNEL_ACCOUNT); > + if (!crypto->iv) > + goto e_free_crypto; > + > + if (crypto_aead_authsize(crypto->tfm) > MAX_AUTHTAG_LEN) { > + if (crypto_aead_setauthsize(crypto->tfm, MAX_AUTHTAG_LEN)) { > + dev_err(snp_dev->dev, "failed to set authsize to %d\n", MAX_AUTHTAG_LEN); > + goto e_free_crypto; > + } > + } > + > + crypto->a_len = crypto_aead_authsize(crypto->tfm); > + crypto->authtag = kmalloc(crypto->a_len, GFP_KERNEL_ACCOUNT); > + if (!crypto->authtag) > + goto e_free_crypto; > + > + return crypto; > + > +e_free_crypto: > + crypto_free_aead(crypto->tfm); > +e_free: > + kfree(crypto->iv); > + kfree(crypto->authtag); > + kfree(crypto); > + > + return NULL; > +} ... > +static int handle_guest_request(struct snp_guest_dev *snp_dev, int msg_type, > + struct snp_user_guest_request *input, void *req_buf, > + size_t req_len, void __user *resp_buf, size_t resp_len) > +{ > + struct snp_guest_crypto *crypto = snp_dev->crypto; > + struct page *page; > + size_t msg_len; > + int ret; > + > + /* Allocate the buffer to hold response */ > + resp_len += crypto->a_len; > + page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(resp_len)); > + if (!page) > + return -ENOMEM; > + > + ret = __handle_guest_request(snp_dev, msg_type, input, req_buf, req_len, > + page_address(page), resp_len, &msg_len); Align arguments on the opening brace. Check the whole patch too for other similar cases. > + if (ret) > + goto e_free; > + > + if (copy_to_user(resp_buf, page_address(page), msg_len)) > + ret = -EFAULT; > + > +e_free: > + __free_pages(page, get_order(resp_len)); > + > + return ret; > +} > + > +static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *input) > +{ > + struct snp_user_report __user *report = (struct snp_user_report *)input->data; > + struct snp_user_report_req req; > + > + if (copy_from_user(&req, &report->req, sizeof(req))) What guarantees that that __user report thing is valid and is not going to trick the kernel into doing a NULL pointer access in the ->req access here? IOW, you need to verify all your user data being passed through before using it. > + return -EFAULT; > + > + return handle_guest_request(snp_dev, SNP_MSG_REPORT_REQ, input, &req.user_data, > + sizeof(req.user_data), report->response, sizeof(report->response)); > +} > + > +static int derive_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *input) > +{ > + struct snp_user_derive_key __user *key = (struct snp_user_derive_key *)input->data; > + struct snp_user_derive_key_req req; > + > + if (copy_from_user(&req, &key->req, sizeof(req))) > + return -EFAULT; > + > + return handle_guest_request(snp_dev, SNP_MSG_KEY_REQ, input, &req, sizeof(req), > + key->response, sizeof(key->response)); > +} > + > +static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) > +{ > + struct snp_guest_dev *snp_dev = to_snp_dev(file); > + struct snp_user_guest_request input; > + void __user *argp = (void __user *)arg; > + int ret = -ENOTTY; > + > + if (copy_from_user(&input, argp, sizeof(input))) > + return -EFAULT; > + > + mutex_lock(&snp_cmd_mutex); > + switch (ioctl) { > + case SNP_GET_REPORT: { > + ret = get_report(snp_dev, &input); > + break; > + } > + case SNP_DERIVE_KEY: { > + ret = derive_key(snp_dev, &input); > + break; > + } > + default: > + break; > + } If only two ioctls, you don't need the switch-case thing. > + > + mutex_unlock(&snp_cmd_mutex); > + > + if (copy_to_user(argp, &input, sizeof(input))) > + return -EFAULT; > + > + return ret; > +} ... > diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h > new file mode 100644 > index 000000000000..0a8454631605 > --- /dev/null > +++ b/include/uapi/linux/sev-guest.h > @@ -0,0 +1,56 @@ > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* > + * Userspace interface for AMD SEV and SEV-SNP guest driver. > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + * > + * SEV-SNP API specification is available at: https://developer.amd.com/sev/ > + */ > + > +#ifndef __UAPI_LINUX_SEV_GUEST_H_ > +#define __UAPI_LINUX_SEV_GUEST_H_ > + > +#include <linux/types.h> > + > +struct snp_user_report_req { > + __u8 user_data[64]; > +}; > + > +struct snp_user_report { > + struct snp_user_report_req req; > + > + /* see SEV-SNP spec for the response format */ > + __u8 response[4000]; > +}; > + > +struct snp_user_derive_key_req { > + __u8 root_key_select; > + __u64 guest_field_select; > + __u32 vmpl; > + __u32 guest_svn; > + __u64 tcb_version; > +}; > + > +struct snp_user_derive_key { > + struct snp_user_derive_key_req req; > + > + /* see SEV-SNP spec for the response format */ > + __u8 response[64]; > +}; > + > +struct snp_user_guest_request { > + /* Message version number (must be non-zero) */ > + __u8 msg_version; > + __u64 data; > + > + /* firmware error code on failure (see psp-sev.h) */ > + __u32 fw_err; > +}; All those struct names have a "snp_user" prefix. It seems to me that that "user" is superfluous. > + > +#define SNP_GUEST_REQ_IOC_TYPE 'S' > +#define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, struct snp_user_guest_request) > +#define SNP_DERIVE_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request) Where are those ioctls documented so that userspace can know how to use them? Thx.
On 6/30/2021 8:35 AM, Borislav Petkov wrote: > > Seeing how there are a bunch of such driver things for SEV stuff, I'd > say to put it under: > > drivers/virt/coco/ > > where we can collect all those confidential computing supporting > drivers. > Sounds good to me. >> >> + depends on AMD_MEM_ENCRYPT >> + help >> + Provides AMD SNP guest request driver. The driver can be used by the > > s/Provides AMD SNP guest request driver. // > >> + guest to communicate with the hypervisor to request the attestation report > > to communicate with the PSP, I thought, not the hypervisor? Yes, the guest communicates directly with the PSP through the hypervisor. I will fix the wording. > >> + and more. >> + >> + If you choose 'M' here, this module will be called sevguest. >> diff --git a/drivers/virt/sevguest/Makefile b/drivers/virt/sevguest/Makefile >> new file mode 100644 >> index 000000000000..1505df437682 >> --- /dev/null >> +++ b/drivers/virt/sevguest/Makefile >> @@ -0,0 +1,4 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +sevguest-y := snp.o > > What's that for? > > Why isn't the filename simply called: > > drivers/virt/coco/sevguest.c > > ? > > Or is more coming? > > And below there's > > .name = "snp-guest", > > so you need to get the naming in order here. > As you have noticed that Dov is submitting the SEV specific driver. I was thinking that it will be nice if we have one driver that covers both the SEV and SEV-SNP. That driver can be called "sevguest". The kernel will install the appropriate platform device. The sevguest driver can probe for both the "sev-guest" and "snp-guest" and delegate the ioctl handling accordingly. In the kernel the directory structure may look like this: virt/coco/sevguest sevguest.c // common code snp.c // SNP specific ioctl implementation sev.c // SEV specific ioctl or sysfs implementation Thoughts ? >> + struct snp_guest_crypto *crypto; >> + >> + crypto = kzalloc(sizeof(*crypto), GFP_KERNEL_ACCOUNT); >> + if (!crypto) >> + return NULL; >> + >> + crypto->tfm = crypto_alloc_aead("gcm(aes)", 0, 0); > > I know that it is hard to unselect CONFIG_CRYPTO_AEAD2 which provides > this but you better depend on it in the Makefile so that some random > config still builds. > Noted. >> + if (IS_ERR(crypto->tfm)) >> + goto e_free; >> + >> + if (crypto_aead_setkey(crypto->tfm, key, keylen)) >> + >> + ret = __handle_guest_request(snp_dev, msg_type, input, req_buf, req_len, >> + page_address(page), resp_len, &msg_len); > > Align arguments on the opening brace. > > Check the whole patch too for other similar cases. Noted. > >> + struct snp_user_report __user *report = (struct snp_user_report *)input->data; >> + struct snp_user_report_req req; >> + >> + if (copy_from_user(&req, &report->req, sizeof(req))) > > What guarantees that that __user report thing is valid and is not going > to trick the kernel into doing a NULL pointer access in the ->req access > here? > > IOW, you need to verify all your user data being passed through before > using it. Let me work to go through it and make sure that we don't get into NULL deference situtation. > >> + case SNP_GET_REPORT: { >> + ret = get_report(snp_dev, &input); >> + break; >> + } >> + case SNP_DERIVE_KEY: { >> + ret = derive_key(snp_dev, &input); >> + break; >> + } >> + default: >> + break; >> + } > > If only two ioctls, you don't need the switch-case thing. > I am working to add support for "extended guest request" that will make it 3 ioctl. >> + >> +struct snp_user_guest_request { >> + /* Message version number (must be non-zero) */ >> + __u8 msg_version; >> + __u64 data; >> + >> + /* firmware error code on failure (see psp-sev.h) */ >> + __u32 fw_err; >> +}; > > All those struct names have a "snp_user" prefix. It seems to me that > that "user" is superfluous. > I followed the naming convension you recommended during the initial SEV driver developement. IIRC, the main reason for us having to add "user" in it because we wanted to distinguious that this structure is not exactly same as the what is defined in the SEV-SNP firmware spec. >> + >> +#define SNP_GUEST_REQ_IOC_TYPE 'S' >> +#define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, struct snp_user_guest_request) >> +#define SNP_DERIVE_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request) > > Where are those ioctls documented so that userspace can know how to use > them? Good question, I am not able to find a generic place to document it. Should we create a documentation "Documentation/virt/coco/sevguest-api.rst" for it ? I am open to other suggestions. -Brijesh
On Wed, Jun 30, 2021 at 11:26:46AM -0500, Brijesh Singh wrote: > As you have noticed that Dov is submitting the SEV specific driver. Well, reportedly that driver is generic-ish as it only handles the EFI-provided sekrits and is not SEV-specific - the SEV use is only exemplary. > I was thinking that it will be nice if we have one driver that covers > both the SEV and SEV-SNP. That driver can be called "sevguest". The > kernel will install the appropriate platform device. The sevguest > driver can probe for both the "sev-guest" and "snp-guest" and delegate > the ioctl handling accordingly. > > In the kernel the directory structure may look like this: > > virt/coco/sevguest > sevguest.c // common code > snp.c // SNP specific ioctl implementation > sev.c // SEV specific ioctl or sysfs implementation > > Thoughts ? Sure, but I'd call it sevguest.c and will have it deal with both SEV and SNP ioctls depending on what has been detected in the hardware. Or is there some special reason for having snp.c and sev.c separate? > I followed the naming convension you recommended during the initial SEV driver > developement. IIRC, the main reason for us having to add "user" in it because > we wanted to distinguious that this structure is not exactly same as the what > is defined in the SEV-SNP firmware spec. I most definitely have forgotten about this. Can you point me to the details of that discussion and why there's a need to distinguish? > Good question, I am not able to find a generic place to document it. Should we > create a documentation "Documentation/virt/coco/sevguest-api.rst" for it ? I am > open to other suggestions. Well, grepping the tree for "ioctl" I see: Documentation/driver-api/ioctl.rst Documentation/process/botching-up-ioctls.rst Documentation/userspace-api/ioctl/cdrom.rst Documentation/userspace-api/ioctl/hdio.rst Documentation/userspace-api/ioctl/index.rst Documentation/userspace-api/ioctl/ioctl-decoding.rst Documentation/userspace-api/ioctl/ioctl-number.rst Documentation/userspace-api/media/cec/cec-func-ioctl.rst Documentation/userspace-api/media/mediactl/media-func-ioctl.rst Documentation/userspace-api/media/mediactl/request-func-ioctl.rst Documentation/userspace-api/media/v4l/func-ioctl.rst and there's some good info as to what to do. In any case, Documentation/virt/coco/sevguest-api.rst doesn't sound too bad either, actually, as it collects everything under virt/ Thx.
On 7/1/2021 1:03 PM, Borislav Petkov wrote: > > Sure, but I'd call it sevguest.c and will have it deal with both SEV and > SNP ioctls depending on what has been detected in the hardware. Or is > there some special reason for having snp.c and sev.c separate? > I don't have any strong reason. I am okay to begin putting all the SNP stuff in the sevguest.c. >> I followed the naming convension you recommended during the initial SEV driver >> developement. IIRC, the main reason for us having to add "user" in it because >> we wanted to distinguious that this structure is not exactly same as the what >> is defined in the SEV-SNP firmware spec. > > I most definitely have forgotten about this. Can you point me to the > details of that discussion and why there's a need to distinguish? > >> Good question, I am not able to find a generic place to document it. Should we >> create a documentation "Documentation/virt/coco/sevguest-api.rst" for it ? I am >> open to other suggestions. > The spec definition is present in include/linux/psp-sev.h but sometime we don't expose the spec defs as-is to userspace. Several SEV/SEV-SNP does not need to be exposed to the userspace, those which need to be expose we provide a bit modified Linux uapi for it, and for SEV drivers we choose "_user" prefix. e.g a spec definition for the PEK import in include/linux/psp-sev.h is: struct sev_data_pek_cert_import { u64 pdh_cert_address; /* system physical address */ u32 pdh_cert_len; u32 reserved; ... }; But its corresponding userspace structure def in include/uapi/linux/psp-sev.h is: struct sev_user_data_pek_cert_import { __u64 pek_cert_uaddr; /* userspace address */ __u32 pek_cert_len; ... }; The ioctl handling takes care of mapping from uaddr to pa and other things as required. So, I took similar approach for the SEV-SNP guest ioctl. In this particular case the guest request structure defined in the spec contains multiple field but many of those fields are managed internally by the kernel (e.g seqno, IV, etc etc). -Brijesh -Brijesh
On Thu, Jul 01, 2021 at 04:32:25PM -0500, Brijesh Singh wrote: > The spec definition is present in include/linux/psp-sev.h but sometime > we don't expose the spec defs as-is to userspace. Why? Having such undocumented and maybe unwarranted differences - I still don't see a clear reason why - is calling for additional and unnecessary confusion. > Several SEV/SEV-SNP does not need to be exposed to the userspace, > those which need to be expose we provide a bit modified Linux uapi for > it, and for SEV drivers we choose "_user" prefix. Is that documented somewhere? Because "user" doesn't tell me it is a modified structure which is different from the spec. > e.g > a spec definition for the PEK import in include/linux/psp-sev.h is: > struct sev_data_pek_cert_import { > u64 pdh_cert_address; /* system physical address */ > u32 pdh_cert_len; > u32 reserved; > ... > }; > > But its corresponding userspace structure def in include/uapi/linux/psp-sev.h is: > struct sev_user_data_pek_cert_import { > __u64 pek_cert_uaddr; /* userspace address */ > __u32 pek_cert_len; > ... > }; And the difference is a single "u32 reserved"? Dunno, from where I'm standing this looks like unnecessary confusion to me. > The ioctl handling takes care of mapping from uaddr to pa and other > things as required. So, I took similar approach for the SEV-SNP guest > ioctl. In this particular case the guest request structure defined in > the spec contains multiple field but many of those fields are managed > internally by the kernel (e.g seqno, IV, etc etc). Ok, multiple fields sounds like you wanna save on the data that is shovelled between kernel and user space and then some of the fields don't mean a thing for the user API. Ok. But again, where is this documented and stated clear so that people are aware? Or are you assuming that since the user counterparts are in include/uapi/linux/psp-sev.h ^^^^ and it being an uapi header, then that should state that?
On 7/3/21 11:19 AM, Borislav Petkov wrote: > On Thu, Jul 01, 2021 at 04:32:25PM -0500, Brijesh Singh wrote: >> The spec definition is present in include/linux/psp-sev.h but sometime >> we don't expose the spec defs as-is to userspace. > Why? > > Having such undocumented and maybe unwarranted differences - I still > don't see a clear reason why - is calling for additional and unnecessary > confusion. Because some of fields don't make any sense for the userspace interfaces. > >> Several SEV/SEV-SNP does not need to be exposed to the userspace, >> those which need to be expose we provide a bit modified Linux uapi for >> it, and for SEV drivers we choose "_user" prefix. > Is that documented somewhere? > > Because "user" doesn't tell me it is a modified structure which is > different from the spec. We have good documentation for the SEV ioctl and structure provided through the KVM interface. Unfortunately the the documentation for the ioctl and structure provided through /dev/sev does not exist. We could look into adding this documentation outside this series. The structure provided through /dev/sev is identical to the structure documented in the spec with minor changes such as not exposing reserved fields or rename from paddr to uaddr etc. >> e.g >> a spec definition for the PEK import in include/linux/psp-sev.h is: >> struct sev_data_pek_cert_import { >> u64 pdh_cert_address; /* system physical address */ >> u32 pdh_cert_len; >> u32 reserved; >> ... >> }; >> >> But its corresponding userspace structure def in include/uapi/linux/psp-sev.h is: >> struct sev_user_data_pek_cert_import { >> __u64 pek_cert_uaddr; /* userspace address */ >> __u32 pek_cert_len; >> ... >> }; > And the difference is a single "u32 reserved"? Mostly yes. > > Dunno, from where I'm standing this looks like unnecessary confusion to > me. >> The ioctl handling takes care of mapping from uaddr to pa and other >> things as required. So, I took similar approach for the SEV-SNP guest >> ioctl. In this particular case the guest request structure defined in >> the spec contains multiple field but many of those fields are managed >> internally by the kernel (e.g seqno, IV, etc etc). > Ok, multiple fields sounds like you wanna save on the data that is > shovelled between kernel and user space and then some of the fields > don't mean a thing for the user API. Ok. > > But again, where is this documented and stated clear so that people are > aware? > > Or are you assuming that since the user counterparts are in > > include/uapi/linux/psp-sev.h > ^^^^ > > and it being an uapi header, then that should state that? > Yes, the assumption is user wanting to communicate to PSP through /dev/sev will need to include include psp-sev.h from uapi/linux/psp-sev.h. The header file itself document the fields definition, and then user need to refer to SEV SPEC for the further details. I could start documenting the SNP specific ioctl in Documentation/virt/coco/sevguest.rst and it can be later expanded to cover SEV and SEV-ES. -Brijesh
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig index 8061e8ef449f..4de714c5ee9a 100644 --- a/drivers/virt/Kconfig +++ b/drivers/virt/Kconfig @@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig" source "drivers/virt/nitro_enclaves/Kconfig" source "drivers/virt/acrn/Kconfig" + +source "drivers/virt/sevguest/Kconfig" + endif diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile index 3e272ea60cd9..b2d1a8131c90 100644 --- a/drivers/virt/Makefile +++ b/drivers/virt/Makefile @@ -8,3 +8,4 @@ obj-y += vboxguest/ obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ obj-$(CONFIG_ACRN_HSM) += acrn/ +obj-$(CONFIG_SEV_GUEST) += sevguest/ diff --git a/drivers/virt/sevguest/Kconfig b/drivers/virt/sevguest/Kconfig new file mode 100644 index 000000000000..e88a85527bf6 --- /dev/null +++ b/drivers/virt/sevguest/Kconfig @@ -0,0 +1,10 @@ +config SEV_GUEST + tristate "AMD SEV Guest driver" + default y + depends on AMD_MEM_ENCRYPT + help + Provides AMD SNP guest request driver. The driver can be used by the + guest to communicate with the hypervisor to request the attestation report + and more. + + If you choose 'M' here, this module will be called sevguest. diff --git a/drivers/virt/sevguest/Makefile b/drivers/virt/sevguest/Makefile new file mode 100644 index 000000000000..1505df437682 --- /dev/null +++ b/drivers/virt/sevguest/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0-only +sevguest-y := snp.o + +obj-$(CONFIG_SEV_GUEST) += sevguest.o diff --git a/drivers/virt/sevguest/snp.c b/drivers/virt/sevguest/snp.c new file mode 100644 index 000000000000..00d8e8fddf2c --- /dev/null +++ b/drivers/virt/sevguest/snp.c @@ -0,0 +1,448 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * AMD Secure Encrypted Virtualization Nested Paging (SEV-SNP) guest request interface + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/types.h> +#include <linux/mutex.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/miscdevice.h> +#include <linux/set_memory.h> +#include <linux/fs.h> +#include <crypto/aead.h> +#include <linux/scatterlist.h> +#include <linux/sev-guest.h> +#include <uapi/linux/sev-guest.h> + +#include "snp.h" + +#define DEVICE_NAME "sev-guest" +#define AAD_LEN 48 +#define MSG_HDR_VER 1 + +struct snp_guest_crypto { + struct crypto_aead *tfm; + uint8_t *iv, *authtag; + int iv_len, a_len; +}; + +struct snp_guest_dev { + struct device *dev; + struct miscdevice misc; + + struct snp_guest_crypto *crypto; + struct snp_guest_msg *request, *response; +}; + +static DEFINE_MUTEX(snp_cmd_mutex); + +static inline struct snp_guest_dev *to_snp_dev(struct file *file) +{ + struct miscdevice *dev = file->private_data; + + return container_of(dev, struct snp_guest_dev, misc); +} + +static struct snp_guest_crypto *init_crypto(struct snp_guest_dev *snp_dev, uint8_t *key, + size_t keylen) +{ + struct snp_guest_crypto *crypto; + + crypto = kzalloc(sizeof(*crypto), GFP_KERNEL_ACCOUNT); + if (!crypto) + return NULL; + + crypto->tfm = crypto_alloc_aead("gcm(aes)", 0, 0); + if (IS_ERR(crypto->tfm)) + goto e_free; + + if (crypto_aead_setkey(crypto->tfm, key, keylen)) + goto e_free_crypto; + + crypto->iv_len = crypto_aead_ivsize(crypto->tfm); + if (crypto->iv_len < 12) { + dev_err(snp_dev->dev, "IV length is less than 12.\n"); + goto e_free_crypto; + } + + crypto->iv = kmalloc(crypto->iv_len, GFP_KERNEL_ACCOUNT); + if (!crypto->iv) + goto e_free_crypto; + + if (crypto_aead_authsize(crypto->tfm) > MAX_AUTHTAG_LEN) { + if (crypto_aead_setauthsize(crypto->tfm, MAX_AUTHTAG_LEN)) { + dev_err(snp_dev->dev, "failed to set authsize to %d\n", MAX_AUTHTAG_LEN); + goto e_free_crypto; + } + } + + crypto->a_len = crypto_aead_authsize(crypto->tfm); + crypto->authtag = kmalloc(crypto->a_len, GFP_KERNEL_ACCOUNT); + if (!crypto->authtag) + goto e_free_crypto; + + return crypto; + +e_free_crypto: + crypto_free_aead(crypto->tfm); +e_free: + kfree(crypto->iv); + kfree(crypto->authtag); + kfree(crypto); + + return NULL; +} + +static void deinit_crypto(struct snp_guest_crypto *crypto) +{ + crypto_free_aead(crypto->tfm); + kfree(crypto->iv); + kfree(crypto->authtag); + kfree(crypto); +} + +static int enc_dec_message(struct snp_guest_crypto *crypto, struct snp_guest_msg *msg, + uint8_t *src_buf, uint8_t *dst_buf, size_t len, bool enc) +{ + struct snp_guest_msg_hdr *hdr = &msg->hdr; + struct scatterlist src[3], dst[3]; + DECLARE_CRYPTO_WAIT(wait); + struct aead_request *req; + int ret; + + req = aead_request_alloc(crypto->tfm, GFP_KERNEL); + if (!req) + return -ENOMEM; + + /* + * AEAD memory operations: + * +------ AAD -------+------- DATA -----+---- AUTHTAG----+ + * | msg header | plaintext | hdr->authtag | + * | bytes 30h - 5Fh | or | | + * | | cipher | | + * +------------------+------------------+----------------+ + */ + sg_init_table(src, 3); + sg_set_buf(&src[0], &hdr->algo, AAD_LEN); + sg_set_buf(&src[1], src_buf, hdr->msg_sz); + sg_set_buf(&src[2], hdr->authtag, crypto->a_len); + + sg_init_table(dst, 3); + sg_set_buf(&dst[0], &hdr->algo, AAD_LEN); + sg_set_buf(&dst[1], dst_buf, hdr->msg_sz); + sg_set_buf(&dst[2], hdr->authtag, crypto->a_len); + + aead_request_set_ad(req, AAD_LEN); + aead_request_set_tfm(req, crypto->tfm); + aead_request_set_callback(req, 0, crypto_req_done, &wait); + + aead_request_set_crypt(req, src, dst, len, crypto->iv); + ret = crypto_wait_req(enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req), &wait); + + aead_request_free(req); + return ret; +} + +static int encrypt_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg, + void *plaintext, size_t len) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_guest_msg_hdr *hdr = &msg->hdr; + + memset(crypto->iv, 0, crypto->iv_len); + memcpy(crypto->iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno)); + + return enc_dec_message(crypto, msg, plaintext, msg->payload, len, true); +} + +static int decrypt_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg, + void *plaintext, size_t len) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_guest_msg_hdr *hdr = &msg->hdr; + + /* Build IV with response buffer sequence number */ + memset(crypto->iv, 0, crypto->iv_len); + memcpy(crypto->iv, &hdr->msg_seqno, sizeof(hdr->msg_seqno)); + + return enc_dec_message(crypto, msg, msg->payload, plaintext, len, false); +} + +static int __handle_guest_request(struct snp_guest_dev *snp_dev, int msg_type, + struct snp_user_guest_request *input, uint8_t *req_buf, + size_t req_sz, uint8_t *resp_buf, size_t resp_sz, size_t *msg_sz) +{ + struct snp_guest_msg *response = snp_dev->response; + struct snp_guest_msg_hdr *resp_hdr = &response->hdr; + struct snp_guest_msg *request = snp_dev->request; + struct snp_guest_msg_hdr *req_hdr = &request->hdr; + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct snp_guest_request_data data; + int ret; + + memset(request, 0, sizeof(*request)); + + /* Populate the request header */ + req_hdr->algo = SNP_AEAD_AES_256_GCM; + req_hdr->hdr_version = MSG_HDR_VER; + req_hdr->hdr_sz = sizeof(*req_hdr); + req_hdr->msg_type = msg_type; + req_hdr->msg_version = input->msg_version; + req_hdr->msg_seqno = snp_msg_seqno(); + req_hdr->msg_vmpck = 0; + req_hdr->msg_sz = req_sz; + + dev_dbg(snp_dev->dev, "request [seqno %lld type %d version %d sz %d]\n", + req_hdr->msg_seqno, req_hdr->msg_type, req_hdr->msg_version, req_hdr->msg_sz); + + /* Encrypt the request message buffer */ + ret = encrypt_payload(snp_dev, request, req_buf, req_sz); + if (ret) + return ret; + + /* Call firmware to process the request */ + data.req_gpa = __pa(request); + data.resp_gpa = __pa(response); + ret = snp_issue_guest_request(GUEST_REQUEST, &data); + input->fw_err = ret; + if (ret) + return ret; + + dev_dbg(snp_dev->dev, "response [msg_seqno %lld msg_type %d msg_version %d msg_sz %d]\n", + resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version, resp_hdr->msg_sz); + + /* Verify that the sequence counter is incremented by 1 */ + if (unlikely(resp_hdr->msg_seqno != (req_hdr->msg_seqno + 1))) + return -EBADMSG; + + /* Verify response message type and version */ + if ((resp_hdr->msg_type != (req_hdr->msg_type + 1)) || + (resp_hdr->msg_version != req_hdr->msg_version)) + return -EBADMSG; + + /* + * If the message size is greather than our buffer length then return + * an error. + */ + if (unlikely((resp_hdr->msg_sz + crypto->a_len) > resp_sz)) + return -EBADMSG; + + /* Decrypt the payload */ + ret = decrypt_payload(snp_dev, response, resp_buf, resp_hdr->msg_sz + crypto->a_len); + if (ret) + return ret; + + *msg_sz = resp_hdr->msg_sz; + return 0; +} + +static int handle_guest_request(struct snp_guest_dev *snp_dev, int msg_type, + struct snp_user_guest_request *input, void *req_buf, + size_t req_len, void __user *resp_buf, size_t resp_len) +{ + struct snp_guest_crypto *crypto = snp_dev->crypto; + struct page *page; + size_t msg_len; + int ret; + + /* Allocate the buffer to hold response */ + resp_len += crypto->a_len; + page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(resp_len)); + if (!page) + return -ENOMEM; + + ret = __handle_guest_request(snp_dev, msg_type, input, req_buf, req_len, + page_address(page), resp_len, &msg_len); + if (ret) + goto e_free; + + if (copy_to_user(resp_buf, page_address(page), msg_len)) + ret = -EFAULT; + +e_free: + __free_pages(page, get_order(resp_len)); + + return ret; +} + +static int get_report(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *input) +{ + struct snp_user_report __user *report = (struct snp_user_report *)input->data; + struct snp_user_report_req req; + + if (copy_from_user(&req, &report->req, sizeof(req))) + return -EFAULT; + + return handle_guest_request(snp_dev, SNP_MSG_REPORT_REQ, input, &req.user_data, + sizeof(req.user_data), report->response, sizeof(report->response)); +} + +static int derive_key(struct snp_guest_dev *snp_dev, struct snp_user_guest_request *input) +{ + struct snp_user_derive_key __user *key = (struct snp_user_derive_key *)input->data; + struct snp_user_derive_key_req req; + + if (copy_from_user(&req, &key->req, sizeof(req))) + return -EFAULT; + + return handle_guest_request(snp_dev, SNP_MSG_KEY_REQ, input, &req, sizeof(req), + key->response, sizeof(key->response)); +} + +static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) +{ + struct snp_guest_dev *snp_dev = to_snp_dev(file); + struct snp_user_guest_request input; + void __user *argp = (void __user *)arg; + int ret = -ENOTTY; + + if (copy_from_user(&input, argp, sizeof(input))) + return -EFAULT; + + mutex_lock(&snp_cmd_mutex); + switch (ioctl) { + case SNP_GET_REPORT: { + ret = get_report(snp_dev, &input); + break; + } + case SNP_DERIVE_KEY: { + ret = derive_key(snp_dev, &input); + break; + } + default: + break; + } + + mutex_unlock(&snp_cmd_mutex); + + if (copy_to_user(argp, &input, sizeof(input))) + return -EFAULT; + + return ret; +} + +static void free_shared_pages(void *buf, size_t sz) +{ + unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; + + /* If fail to restore the encryption mask then leak it. */ + if (set_memory_encrypted((unsigned long)buf, npages)) + return; + + __free_pages(virt_to_page(buf), get_order(sz)); +} + +static void *alloc_shared_pages(size_t sz) +{ + unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; + struct page *page; + int ret; + + page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(sz)); + if (IS_ERR(page)) + return NULL; + + ret = set_memory_decrypted((unsigned long)page_address(page), npages); + if (ret) { + __free_pages(page, get_order(sz)); + return NULL; + } + + return page_address(page); +} + +static const struct file_operations snp_guest_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = snp_guest_ioctl, +}; + +static int __init snp_guest_probe(struct platform_device *pdev) +{ + struct snp_secrets_page_layout *secrets; + struct device *dev = &pdev->dev; + struct snp_guest_dev *snp_dev; + uint8_t key[VMPCK_KEY_LEN]; + struct miscdevice *misc; + struct resource *res; + void __iomem *base; + int ret; + + snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL); + if (!snp_dev) + return -ENOMEM; + + platform_set_drvdata(pdev, snp_dev); + snp_dev->dev = dev; + + res = platform_get_mem_or_io(pdev, 0); + if (IS_ERR(res)) + return PTR_ERR(res); + + /* Map the secrets page to get the key */ + base = ioremap_encrypted(res->start, resource_size(res)); + if (IS_ERR(base)) + return PTR_ERR(base); + + secrets = (struct snp_secrets_page_layout *)base; + memcpy_fromio(key, secrets->vmpck0, sizeof(key)); + iounmap(base); + + snp_dev->crypto = init_crypto(snp_dev, key, sizeof(key)); + if (!snp_dev->crypto) + return -EIO; + + /* Allocate the shared page used for the request and response message. */ + snp_dev->request = alloc_shared_pages(sizeof(struct snp_guest_msg)); + if (IS_ERR(snp_dev->request)) + return PTR_ERR(snp_dev->request); + + snp_dev->response = alloc_shared_pages(sizeof(struct snp_guest_msg)); + if (IS_ERR(snp_dev->response)) { + ret = PTR_ERR(snp_dev->response); + goto e_free_req; + } + + misc = &snp_dev->misc; + misc->minor = MISC_DYNAMIC_MINOR; + misc->name = DEVICE_NAME; + misc->fops = &snp_guest_fops; + + return misc_register(misc); + +e_free_req: + free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg)); + return ret; +} + +static int __exit snp_guest_remove(struct platform_device *pdev) +{ + struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev); + + free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg)); + free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg)); + deinit_crypto(snp_dev->crypto); + misc_deregister(&snp_dev->misc); + + return 0; +} + +static struct platform_driver snp_guest_driver = { + .remove = __exit_p(snp_guest_remove), + .driver = { + .name = "snp-guest", + }, +}; + +module_platform_driver_probe(snp_guest_driver, snp_guest_probe); + +MODULE_AUTHOR("Brijesh Singh <brijesh.singh@amd.com>"); +MODULE_LICENSE("GPL"); +MODULE_VERSION("1.0.0"); +MODULE_DESCRIPTION("AMD SNP Guest Driver"); diff --git a/drivers/virt/sevguest/snp.h b/drivers/virt/sevguest/snp.h new file mode 100644 index 000000000000..930ffc0f4be3 --- /dev/null +++ b/drivers/virt/sevguest/snp.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + * SEV-SNP API spec is available at https://developer.amd.com/sev + */ + +#ifndef __LINUX_SNP_GUEST_H_ +#define __LINUX_SNP_GUEST_H_ + +#include <linux/types.h> + +#define MAX_AUTHTAG_LEN 32 + +/* See SNP spec SNP_GUEST_REQUEST section for the structure */ +enum msg_type { + SNP_MSG_TYPE_INVALID = 0, + SNP_MSG_CPUID_REQ, + SNP_MSG_CPUID_RSP, + SNP_MSG_KEY_REQ, + SNP_MSG_KEY_RSP, + SNP_MSG_REPORT_REQ, + SNP_MSG_REPORT_RSP, + SNP_MSG_EXPORT_REQ, + SNP_MSG_EXPORT_RSP, + SNP_MSG_IMPORT_REQ, + SNP_MSG_IMPORT_RSP, + SNP_MSG_ABSORB_REQ, + SNP_MSG_ABSORB_RSP, + SNP_MSG_VMRK_REQ, + SNP_MSG_VMRK_RSP, + + SNP_MSG_TYPE_MAX +}; + +enum aead_algo { + SNP_AEAD_INVALID, + SNP_AEAD_AES_256_GCM, +}; + +struct snp_guest_msg_hdr { + u8 authtag[MAX_AUTHTAG_LEN]; + u64 msg_seqno; + u8 rsvd1[8]; + u8 algo; + u8 hdr_version; + u16 hdr_sz; + u8 msg_type; + u8 msg_version; + u16 msg_sz; + u32 rsvd2; + u8 msg_vmpck; + u8 rsvd3[35]; +} __packed; + +struct snp_guest_msg { + struct snp_guest_msg_hdr hdr; + u8 payload[4000]; +} __packed; + +#endif /* __LINUX_SNP_GUEST_H__ */ diff --git a/include/uapi/linux/sev-guest.h b/include/uapi/linux/sev-guest.h new file mode 100644 index 000000000000..0a8454631605 --- /dev/null +++ b/include/uapi/linux/sev-guest.h @@ -0,0 +1,56 @@ +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ +/* + * Userspace interface for AMD SEV and SEV-SNP guest driver. + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + * SEV-SNP API specification is available at: https://developer.amd.com/sev/ + */ + +#ifndef __UAPI_LINUX_SEV_GUEST_H_ +#define __UAPI_LINUX_SEV_GUEST_H_ + +#include <linux/types.h> + +struct snp_user_report_req { + __u8 user_data[64]; +}; + +struct snp_user_report { + struct snp_user_report_req req; + + /* see SEV-SNP spec for the response format */ + __u8 response[4000]; +}; + +struct snp_user_derive_key_req { + __u8 root_key_select; + __u64 guest_field_select; + __u32 vmpl; + __u32 guest_svn; + __u64 tcb_version; +}; + +struct snp_user_derive_key { + struct snp_user_derive_key_req req; + + /* see SEV-SNP spec for the response format */ + __u8 response[64]; +}; + +struct snp_user_guest_request { + /* Message version number (must be non-zero) */ + __u8 msg_version; + __u64 data; + + /* firmware error code on failure (see psp-sev.h) */ + __u32 fw_err; +}; + +#define SNP_GUEST_REQ_IOC_TYPE 'S' +#define SNP_GET_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x0, struct snp_user_guest_request) +#define SNP_DERIVE_KEY _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x1, struct snp_user_guest_request) + +#endif /* __UAPI_LINUX_SEV_GUEST_H_ */
SEV-SNP specification provides the guest a mechanism to communicate with the PSP without risk from a malicious hypervisor who wishes to read, alter, drop or replay the messages sent. The driver uses snp_issue_guest_request() to issue GHCB SNP_GUEST_REQUEST NAE event. This command constructs a trusted channel between the guest and the PSP firmware. The userspace can use the following ioctls provided by the driver: 1. Request an attestation report that can be used to assume the identity and security configuration of the guest. 2. Ask the firmware to provide a key derived from a root key. See SEV-SNP spec section Guest Messages for more details. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- drivers/virt/Kconfig | 3 + drivers/virt/Makefile | 1 + drivers/virt/sevguest/Kconfig | 10 + drivers/virt/sevguest/Makefile | 4 + drivers/virt/sevguest/snp.c | 448 +++++++++++++++++++++++++++++++++ drivers/virt/sevguest/snp.h | 63 +++++ include/uapi/linux/sev-guest.h | 56 +++++ 7 files changed, 585 insertions(+) create mode 100644 drivers/virt/sevguest/Kconfig create mode 100644 drivers/virt/sevguest/Makefile create mode 100644 drivers/virt/sevguest/snp.c create mode 100644 drivers/virt/sevguest/snp.h create mode 100644 include/uapi/linux/sev-guest.h