linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
@ 2019-02-13 19:23 Singh, Brijesh
  2019-02-14 16:57 ` Nathaniel McCallum
  0 siblings, 1 reply; 5+ messages in thread
From: Singh, Brijesh @ 2019-02-13 19:23 UTC (permalink / raw)
  To: linux-crypto
  Cc: linux-kernel, Singh, Brijesh, Natarajan, Janakarajan, Herbert Xu,
	Hook, Gary, Lendacky, Thomas

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@amd.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Gary Hook <gary.hook@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 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@amd.com>
  *
- * 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
  2019-02-13 19:23 [PATCH] crypto: ccp: introduce SEV_GET_ID2 command Singh, Brijesh
@ 2019-02-14 16:57 ` Nathaniel McCallum
  2019-02-14 19:33   ` Singh, Brijesh
  0 siblings, 1 reply; 5+ messages in thread
From: Nathaniel McCallum @ 2019-02-14 16:57 UTC (permalink / raw)
  To: Singh, Brijesh
  Cc: linux-crypto, linux-kernel, Natarajan, Janakarajan, Herbert Xu,
	Hook, Gary, Lendacky, Thomas

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@amd.com> 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@amd.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Gary Hook <gary.hook@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  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@amd.com>
>   *
> - * 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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
  2019-02-14 16:57 ` Nathaniel McCallum
@ 2019-02-14 19:33   ` Singh, Brijesh
  2019-02-22  4:37     ` Herbert Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Singh, Brijesh @ 2019-02-14 19:33 UTC (permalink / raw)
  To: Nathaniel McCallum
  Cc: Singh, Brijesh, linux-crypto, linux-kernel, Natarajan,
	Janakarajan, Herbert Xu, Hook, Gary, Lendacky, Thomas



On 2/14/19 10:57 AM, Nathaniel McCallum wrote:
> 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. 


There are not many programs using the GET_ID today, my preference
is to force userspace running on a kernel which supports the GET_ID2
to use GET_ID2 and not fallback to GET_ID.

The current GET_ID is *broken*.

Here is one case I am trying to navigate:
- AMD releases a new CPU
- The kernel used in your distro does not support this CPU yet.
   You updated the kernel to get the CPU support.
- The GET_ID on this CPU returned a 10 bytes (instead of 64)
- You gave the 64-bytes of data to AMD to get the certificate.
   AMD server rejects the request because ID given to it does not
   exist in its database.

If we drop the support for GET_ID in kernel, then GET_ID will fail and
user will required to take action.


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@amd.com> 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@amd.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Gary Hook <gary.hook@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   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@amd.com>
>>    *
>> - * 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
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
  2019-02-14 19:33   ` Singh, Brijesh
@ 2019-02-22  4:37     ` Herbert Xu
  2019-02-25 22:46       ` Singh, Brijesh
  0 siblings, 1 reply; 5+ messages in thread
From: Herbert Xu @ 2019-02-22  4:37 UTC (permalink / raw)
  To: Singh, Brijesh
  Cc: Nathaniel McCallum, linux-crypto, linux-kernel, Natarajan,
	Janakarajan, Hook, Gary, Lendacky, Thomas

On Thu, Feb 14, 2019 at 07:33:29PM +0000, Singh, Brijesh wrote:
> 
> 
> On 2/14/19 10:57 AM, Nathaniel McCallum wrote:
> > 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. 
> 
> 
> There are not many programs using the GET_ID today, my preference
> is to force userspace running on a kernel which supports the GET_ID2
> to use GET_ID2 and not fallback to GET_ID.
> 
> The current GET_ID is *broken*.
> 
> Here is one case I am trying to navigate:
> - AMD releases a new CPU
> - The kernel used in your distro does not support this CPU yet.
>    You updated the kernel to get the CPU support.
> - The GET_ID on this CPU returned a 10 bytes (instead of 64)
> - You gave the 64-bytes of data to AMD to get the certificate.
>    AMD server rejects the request because ID given to it does not
>    exist in its database.
> 
> If we drop the support for GET_ID in kernel, then GET_ID will fail and
> user will required to take action.

Sorry, but we can't drop a kernel API just to force userspace
to upgrade to a new one.

So I agree with Nathaniel that we should keep compatibility until
such a time when user-space is no longer using the old API.

You can use other mechanisms to encourage user-space to switch
over to the new API, e.g., a once-only warning if the old API
is used.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
  2019-02-22  4:37     ` Herbert Xu
@ 2019-02-25 22:46       ` Singh, Brijesh
  0 siblings, 0 replies; 5+ messages in thread
From: Singh, Brijesh @ 2019-02-25 22:46 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Singh, Brijesh, Nathaniel McCallum, linux-crypto, linux-kernel,
	Natarajan, Janakarajan, Hook, Gary, Lendacky, Thomas



On 2/21/19 10:37 PM, Herbert Xu wrote:
> On Thu, Feb 14, 2019 at 07:33:29PM +0000, Singh, Brijesh wrote:
>>
>>
>> On 2/14/19 10:57 AM, Nathaniel McCallum wrote:
>>> 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.
>>
>>
>> There are not many programs using the GET_ID today, my preference
>> is to force userspace running on a kernel which supports the GET_ID2
>> to use GET_ID2 and not fallback to GET_ID.
>>
>> The current GET_ID is *broken*.
>>
>> Here is one case I am trying to navigate:
>> - AMD releases a new CPU
>> - The kernel used in your distro does not support this CPU yet.
>>     You updated the kernel to get the CPU support.
>> - The GET_ID on this CPU returned a 10 bytes (instead of 64)
>> - You gave the 64-bytes of data to AMD to get the certificate.
>>     AMD server rejects the request because ID given to it does not
>>     exist in its database.
>>
>> If we drop the support for GET_ID in kernel, then GET_ID will fail and
>> user will required to take action.
> 
> Sorry, but we can't drop a kernel API just to force userspace
> to upgrade to a new one.
> 
> So I agree with Nathaniel that we should keep compatibility until
> such a time when user-space is no longer using the old API.
> 
> You can use other mechanisms to encourage user-space to switch
> over to the new API, e.g., a once-only warning if the old API
> is used.
> 


Sure, I will resubmit the patch to keep the old API and maybe
print once-only warning.

thanks

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-02-25 22:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-13 19:23 [PATCH] crypto: ccp: introduce SEV_GET_ID2 command Singh, Brijesh
2019-02-14 16:57 ` Nathaniel McCallum
2019-02-14 19:33   ` Singh, Brijesh
2019-02-22  4:37     ` Herbert Xu
2019-02-25 22:46       ` Singh, Brijesh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).