I'm a little concerned that this immediately disables SEV_GET_ID.
IMHO, we should continue to support both for a period of time. One
justification for immediate disablement would be that if keeping it
around is likely to enabled incorrect or insecure userspace behavior
with a firmware change. Given that this value is passed directly to
the AMD server, I don't think either of these is the case and it can
probably be worked around on the server side.
On Wed, Feb 13, 2019 at 1:24 PM Singh, Brijesh <brijesh.singh@xxxxxxx> wrote:
>
> The current definition and implementation of the SEV_GET_ID command
> does not provide the length of the unique ID returned by the firmware.
> As per the firmware specification, the firmware may return an ID
> length that is not restricted to 64 bytes as assumed by the SEV_GET_ID
> command.
>
> Introduce the SEV_GET_ID2 command to allow for querying and returing
> the length of the ID. Deprecate the SEV_GET_ID in the favor of
> SEV_GET_ID2.
>
> Cc: Janakarajan Natarajan <Janakarajan.Natarajan@xxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Cc: Gary Hook <gary.hook@xxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> ---
> drivers/crypto/ccp/psp-dev.c | 65 +++++++++++++++++++++++++-----------
> include/uapi/linux/psp-sev.h | 18 +++++++---
> 2 files changed, 60 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b16be8a11d92..b510900a9a83 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -584,40 +584,63 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp)
>
> static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> {
> + struct sev_user_data_get_id2 input;
> struct sev_data_get_id *data;
> - u64 data_size, user_size;
> - void *id_blob, *mem;
> + void *id_blob = NULL;
> int ret;
>
> - /* SEV GET_ID available from SEV API v0.16 and up */
> + /* SEV GET_ID is available from SEV API v0.16 and up */
> if (!SEV_VERSION_GREATER_OR_EQUAL(0, 16))
> return -ENOTSUPP;
>
> - /* SEV FW expects the buffer it fills with the ID to be
> - * 8-byte aligned. Memory allocated should be enough to
> - * hold data structure + alignment padding + memory
> - * where SEV FW writes the ID.
> - */
> - data_size = ALIGN(sizeof(struct sev_data_get_id), 8);
> - user_size = sizeof(struct sev_user_data_get_id);
> + if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
> + return -EFAULT;
>
> - mem = kzalloc(data_size + user_size, GFP_KERNEL);
> - if (!mem)
> + /* Check if we have write access to the userspace buffer */
> + if (input.address &&
> + input.length &&
> + !access_ok(input.address, input.length))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> return -ENOMEM;
>
> - data = mem;
> - id_blob = mem + data_size;
> + if (input.address && input.length) {
> + id_blob = kmalloc(input.length, GFP_KERNEL);
> + if (!id_blob) {
> + kfree(data);
> + return -ENOMEM;
> + }
>
> - data->address = __psp_pa(id_blob);
> - data->len = user_size;
> + data->address = __psp_pa(id_blob);
> + data->len = input.length;
> + }
>
> ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
> - if (!ret) {
> - if (copy_to_user((void __user *)argp->data, id_blob, data->len))
> +
> + /*
> + * Firmware will return the length of the ID value (either the minimum
> + * required length or the actual length written), return it to the user.
> + */
> + input.length = data->len;
> +
> + if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> +
> + if (id_blob) {
> + if (copy_to_user((void __user *)input.address,
> + id_blob, data->len)) {
> ret = -EFAULT;
> + goto e_free;
> + }
> }
>
> - kfree(mem);
> +e_free:
> + kfree(id_blob);
> + kfree(data);
>
> return ret;
> }
> @@ -760,6 +783,10 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> ret = sev_ioctl_do_pdh_export(&input);
> break;
> case SEV_GET_ID:
> + /* SEV_GET_ID is deprecated */
> + ret = -ENOTSUPP;
> + break;
> + case SEV_GET_ID2:
> ret = sev_ioctl_do_get_id(&input);
> break;
> default:
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index ac8c60bcc83b..43521d500c2b 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@ -6,8 +6,7 @@
> *
> * Author: Brijesh Singh <brijesh.singh@xxxxxxx>
> *
> - * SEV spec 0.14 is available at:
> - * http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> + * SEV API specification is available at: https://developer.amd.com/sev/
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -30,7 +29,8 @@ enum {
> SEV_PDH_GEN,
> SEV_PDH_CERT_EXPORT,
> SEV_PEK_CERT_IMPORT,
> - SEV_GET_ID,
> + SEV_GET_ID, /* This command is deprecated, use SEV_GET_ID2 */
> + SEV_GET_ID2,
>
> SEV_MAX,
> };
> @@ -125,7 +125,7 @@ struct sev_user_data_pdh_cert_export {
> } __packed;
>
> /**
> - * struct sev_user_data_get_id - GET_ID command parameters
> + * struct sev_user_data_get_id - GET_ID command parameters (deprecated)
> *
> * @socket1: Buffer to pass unique ID of first socket
> * @socket2: Buffer to pass unique ID of second socket
> @@ -135,6 +135,16 @@ struct sev_user_data_get_id {
> __u8 socket2[64]; /* Out */
> } __packed;
>
> +/**
> + * struct sev_user_data_get_id2 - GET_ID command parameters
> + * @address: Buffer to store unique ID
> + * @length: length of the unique ID
> + */
> +struct sev_user_data_get_id2 {
> + __u64 address; /* In */
> + __u32 length; /* In/Out */
> +} __packed;
> +
> /**
> * struct sev_issue_cmd - SEV ioctl parameters
> *
> --
> 2.17.1
>
