On Wed, Oct 27, 2021 at 10:08 AM Brijesh Singh <brijesh.singh@xxxxxxx> wrote:
>
> Hi Peter,
>
> Somehow this email was filtered out as spam and never reached to my
> inbox. Sorry for the delay in the response.
>
> On 10/20/21 4:33 PM, Peter Gonda wrote:
> > On Fri, Oct 8, 2021 at 12:06 PM Brijesh Singh <brijesh.singh@xxxxxxx> wrote:
> >>
> >> SEV-SNP specification provides the guest a mechanisum 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 or SNP_EXT_GUEST_REQUEST NAE events to
> >> submit the request to PSP.
> >>
> >> The PSP requires that all communication should be encrypted using key
> >> specified through the platform_data.
> >>
> >> The userspace can use SNP_GET_REPORT ioctl() to query the guest
> >> attestation report.
> >>
> >> See SEV-SNP spec section Guest Messages for more details.
> >>
> >> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> >> ---
> >> Documentation/virt/coco/sevguest.rst | 77 ++++
> >> drivers/virt/Kconfig | 3 +
> >> drivers/virt/Makefile | 1 +
> >> drivers/virt/coco/sevguest/Kconfig | 9 +
> >> drivers/virt/coco/sevguest/Makefile | 2 +
> >> drivers/virt/coco/sevguest/sevguest.c | 561 ++++++++++++++++++++++++++
> >> drivers/virt/coco/sevguest/sevguest.h | 98 +++++
> >> include/uapi/linux/sev-guest.h | 44 ++
> >> 8 files changed, 795 insertions(+)
> >> create mode 100644 Documentation/virt/coco/sevguest.rst
> >> create mode 100644 drivers/virt/coco/sevguest/Kconfig
> >> create mode 100644 drivers/virt/coco/sevguest/Makefile
> >> create mode 100644 drivers/virt/coco/sevguest/sevguest.c
> >> create mode 100644 drivers/virt/coco/sevguest/sevguest.h
> >> create mode 100644 include/uapi/linux/sev-guest.h
> >>
> >> diff --git a/Documentation/virt/coco/sevguest.rst b/Documentation/virt/coco/sevguest.rst
> >> new file mode 100644
> >> index 000000000000..002c90946b8a
> >> --- /dev/null
> >> +++ b/Documentation/virt/coco/sevguest.rst
> >> @@ -0,0 +1,77 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +===================================================================
> >> +The Definitive SEV Guest API Documentation
> >> +===================================================================
> >> +
> >> +1. General description
> >> +======================
> >> +
> >> +The SEV API is a set of ioctls that are used by the guest or hypervisor
> >> +to get or set certain aspect of the SEV virtual machine. The ioctls belong
> >> +to the following classes:
> >> +
> >> + - Hypervisor ioctls: These query and set global attributes which affect the
> >> + whole SEV firmware. These ioctl are used by platform provision tools.
> >> +
> >> + - Guest ioctls: These query and set attributes of the SEV virtual machine.
> >> +
> >> +2. API description
> >> +==================
> >> +
> >> +This section describes ioctls that can be used to query or set SEV guests.
> >> +For each ioctl, the following information is provided along with a
> >> +description:
> >> +
> >> + Technology:
> >> + which SEV techology provides this ioctl. sev, sev-es, sev-snp or all.
> >> +
> >> + Type:
> >> + hypervisor or guest. The ioctl can be used inside the guest or the
> >> + hypervisor.
> >> +
> >> + Parameters:
> >> + what parameters are accepted by the ioctl.
> >> +
> >> + Returns:
> >> + the return value. General error numbers (ENOMEM, EINVAL)
> >> + are not detailed, but errors with specific meanings are.
> >> +
> >> +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device.
> >> +The ioctl accepts struct snp_user_guest_request. The input and output structure is
> >> +specified through the req_data and resp_data field respectively. If the ioctl fails
> >> +to execute due to a firmware error, then fw_err code will be set.
> >> +
> >> +::
> >> + struct snp_guest_request_ioctl {
> >> + /* Request and response structure address */
> >> + __u64 req_data;
> >> + __u64 resp_data;
> >> +
> >> + /* firmware error code on failure (see psp-sev.h) */
> >> + __u64 fw_err;
> >> + };
> >> +
> >> +2.1 SNP_GET_REPORT
> >> +------------------
> >> +
> >> +:Technology: sev-snp
> >> +:Type: guest ioctl
> >> +:Parameters (in): struct snp_report_req
> >> +:Returns (out): struct snp_report_resp on success, -negative on error
> >> +
> >> +The SNP_GET_REPORT ioctl can be used to query the attestation report from the
> >> +SEV-SNP firmware. The ioctl uses the SNP_GUEST_REQUEST (MSG_REPORT_REQ) command
> >> +provided by the SEV-SNP firmware to query the attestation report.
> >> +
> >> +On success, the snp_report_resp.data will contains the report. The report
> >> +will contain the format described in the SEV-SNP specification. See the SEV-SNP
> >> +specification for further details.
> >> +
> >> +
> >> +Reference
> >> +---------
> >> +
> >> +SEV-SNP and GHCB specification: developer.amd.com/sev
> >> +
> >> +The driver is based on SEV-SNP firmware spec 0.9 and GHCB spec version 2.0.
> >> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> >> index 8061e8ef449f..e457e47610d3 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/coco/sevguest/Kconfig"
> >> +
> >> endif
> >> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> >> index 3e272ea60cd9..9c704a6fdcda 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) += coco/sevguest/
> >> diff --git a/drivers/virt/coco/sevguest/Kconfig b/drivers/virt/coco/sevguest/Kconfig
> >> new file mode 100644
> >> index 000000000000..96190919cca8
> >> --- /dev/null
> >> +++ b/drivers/virt/coco/sevguest/Kconfig
> >> @@ -0,0 +1,9 @@
> >> +config SEV_GUEST
> >> + tristate "AMD SEV Guest driver"
> >> + default y
> >> + depends on AMD_MEM_ENCRYPT && CRYPTO_AEAD2
> >> + help
> >> + The driver can be used by the SEV-SNP guest to communicate with the PSP to
> >> + request the attestation report and more.
> >> +
> >> + If you choose 'M' here, this module will be called sevguest.
> >> diff --git a/drivers/virt/coco/sevguest/Makefile b/drivers/virt/coco/sevguest/Makefile
> >> new file mode 100644
> >> index 000000000000..b1ffb2b4177b
> >> --- /dev/null
> >> +++ b/drivers/virt/coco/sevguest/Makefile
> >> @@ -0,0 +1,2 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only
> >> +obj-$(CONFIG_SEV_GUEST) += sevguest.o
> >> diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c
> >> new file mode 100644
> >> index 000000000000..2d313fb2ffae
> >> --- /dev/null
> >> +++ b/drivers/virt/coco/sevguest/sevguest.c
> >> @@ -0,0 +1,561 @@
> >> +// 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@xxxxxxx>
> >> + */
> >> +
> >> +#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/psp-sev.h>
> >> +#include <uapi/linux/sev-guest.h>
> >> +#include <uapi/linux/psp-sev.h>
> >> +
> >> +#include <asm/svm.h>
> >> +#include <asm/sev.h>
> >> +
> >> +#include "sevguest.h"
> >> +
> >> +#define DEVICE_NAME "sev-guest"
> >> +#define AAD_LEN 48
> >> +#define MSG_HDR_VER 1
> >> +
> >> +struct snp_guest_crypto {
> >> + struct crypto_aead *tfm;
> >> + u8 *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;
> >> + struct snp_secrets_page_layout *layout;
> >> + struct snp_req_data input;
> >> + u32 *os_area_msg_seqno;
> >> +};
> >> +
> >> +static u32 vmpck_id;
> >> +module_param(vmpck_id, uint, 0444);
> >> +MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP.");
> >> +
> >> +static DEFINE_MUTEX(snp_cmd_mutex);
> >> +
> >> +static inline u64 __snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> >> +{
> >> + u64 count;
> >> +
> >> + /* Read the current message sequence counter from secrets pages */
> >> + count = *snp_dev->os_area_msg_seqno;
> >> +
> >> + return count + 1;
> >> +}
> >> +
> >> +/* Return a non-zero on success */
> >> +static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
> >> +{
> >> + u64 count = __snp_get_msg_seqno(snp_dev);
> >> +
> >> + /*
> >> + * The message sequence counter for the SNP guest request is a 64-bit
> >> + * value but the version 2 of GHCB specification defines a 32-bit storage
> >> + * for the it. If the counter exceeds the 32-bit value then return zero.
> >> + * The caller should check the return value, but if the caller happen to
> >> + * not check the value and use it, then the firmware treats zero as an
> >> + * invalid number and will fail the message request.
> >> + */
> >> + if (count >= UINT_MAX) {
> >> + pr_err_ratelimited("SNP guest request message sequence counter overflow\n");
> >> + return 0;
> >> + }
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static void snp_inc_msg_seqno(struct snp_guest_dev *snp_dev)
> >> +{
> >> + /*
> >> + * The counter is also incremented by the PSP, so increment it by 2
> >> + * and save in secrets page.
> >> + */
> >> + *snp_dev->os_area_msg_seqno += 2;
> >> +}
> >> +
> >> +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, u8 *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,
> >> + u8 *src_buf, u8 *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 __enc_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 dec_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 verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload, u32 sz)
> >> +{
> >> + struct snp_guest_crypto *crypto = snp_dev->crypto;
> >> + struct snp_guest_msg *resp = snp_dev->response;
> >> + struct snp_guest_msg *req = snp_dev->request;
> >> + struct snp_guest_msg_hdr *req_hdr = &req->hdr;
> >> + struct snp_guest_msg_hdr *resp_hdr = &resp->hdr;
> >> +
> >> + dev_dbg(snp_dev->dev, "response [seqno %lld type %d version %d 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 number. */
> >> + 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 greater than our buffer length then return
> >> + * an error.
> >> + */
> >> + if (unlikely((resp_hdr->msg_sz + crypto->a_len) > sz))
> >> + return -EBADMSG;
> >> +
> >> + return dec_payload(snp_dev, resp, payload, resp_hdr->msg_sz + crypto->a_len);
> >> +}
> >> +
> >> +static bool enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8 type,
> >> + void *payload, size_t sz)
> >> +{
> >> + struct snp_guest_msg *req = snp_dev->request;
> >> + struct snp_guest_msg_hdr *hdr = &req->hdr;
> >> +
> >> + memset(req, 0, sizeof(*req));
> >> +
> >> + hdr->algo = SNP_AEAD_AES_256_GCM;
> >> + hdr->hdr_version = MSG_HDR_VER;
> >> + hdr->hdr_sz = sizeof(*hdr);
> >> + hdr->msg_type = type;
> >> + hdr->msg_version = version;
> >> + hdr->msg_seqno = seqno;
> >> + hdr->msg_vmpck = vmpck_id;
> >> + hdr->msg_sz = sz;
> >> +
> >> + /* Verify the sequence number is non-zero */
> >> + if (!hdr->msg_seqno)
> >> + return -ENOSR;
> >> +
> >> + dev_dbg(snp_dev->dev, "request [seqno %lld type %d version %d sz %d]\n",
> >> + hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz);
> >> +
> >> + return __enc_payload(snp_dev, req, payload, sz);
> >> +}
> >> +
> >> +static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, int msg_ver,
> >> + u8 type, void *req_buf, size_t req_sz, void *resp_buf,
> >> + u32 resp_sz, __u64 *fw_err)
> >> +{
> >> + unsigned long err;
> >> + u64 seqno;
> >> + int rc;
> >> +
> >> + /* Get message sequence and verify that its a non-zero */
> >> + seqno = snp_get_msg_seqno(snp_dev);
> >> + if (!seqno)
> >> + return -EIO;
> >> +
> >> + memset(snp_dev->response, 0, sizeof(*snp_dev->response));
> >> +
> >> + /* Encrypt the userspace provided payload */
> >> + rc = enc_payload(snp_dev, seqno, msg_ver, type, req_buf, req_sz);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /* Call firmware to process the request */
> >> + rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> >> + if (fw_err)
> >> + *fw_err = err;
> >> +
> >> + if (rc)
> >> + return rc;
> >> +
> >> + rc = verify_and_dec_payload(snp_dev, resp_buf, resp_sz);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /* Increment to new message sequence after the command is successful. */
> >> + snp_inc_msg_seqno(snp_dev);
> >
> > Thanks for updating this sequence number logic. But I still have some
> > concerns. In verify_and_dec_payload() we check the encryption header
> > but all these fields are accessible to the hypervisor, meaning it can
> > change the header and cause this sequence number to not get
> > incremented. We then will reuse the sequence number for the next
> > command, which isn't great for AES GCM. It seems very hard to tell if
> > the FW actually got our request and created a response there by
> > incrementing the sequence number by 2, or if the hypervisor is acting
> > in bad faith. It seems like to be safe we need to completely stop
> > using this vmpck if we cannot confirm the PSP has gotten our request
> > and created a response. Thoughts?
> >
>
> Very good point, I think we can detect this condition by rearranging the
> checks. The verify_and_dec_payload() is called only after the command is
> succesful and does the following checks
>
> 1) Verifies the header
> 2) Decrypts the payload
> 3) Later we increment the sequence
>
> If we arrange to the below order then we can avoid this condition.
> 1) Decrypt the payload
> 2) Increment the sequence number
> 3) Verify the header
>
> The descryption will succeed only if PSP constructed the payload.
>
> Does this make sense ?
Either ordering seems fine to me. I don't think it changes much though
since the header (bytes 30-50 according to the spec) are included in
the authenticated data of the encryption. So any hypervisor modictions
will lead to a decryption failure right?
Either case if we do fail the decryption, what are your thoughts on
not allowing further use of that VMPCK?
>
> thanks
