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 <redacted>
---
 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.

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?

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.

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;
+}

...

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette