In preparation for using the TSM key facility to convey attestation blobs
to userspace, add an argument to flag whether @arg is a user buffer or a
kernel buffer.

While TSM keys is meant to replace existing confidenital computing
ioctl() implementations for attestation report retrieval the old ioctl()
path needs to stick around for a deprecation period.

No behavior change intended, just introduce the copy wrappers and @type
argument.

Note that these wrappers are similar to copy_{to,from}_sockptr(). If
this approach moves forward that concept is something that can be
generalized into a helper with a generic name.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c |   48 ++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 97dbe715e96a..f48c4764a7a2 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -470,7 +470,32 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 	return 0;
 }
 
-static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+enum snp_arg_type {
+	SNP_UARG,
+	SNP_KARG,
+};
+
+static unsigned long copy_from(void *to, unsigned long from, unsigned long n,
+			       enum snp_arg_type type)
+{
+	if (type == SNP_UARG)
+		return copy_from_user(to, (void __user *)from, n);
+	memcpy(to, (void *)from, n);
+	return 0;
+}
+
+static unsigned long copy_to(unsigned long to, const void *from,
+			     unsigned long n, enum snp_arg_type type)
+{
+	if (type == SNP_UARG)
+		return copy_to_user((void __user *)to, from, n);
+	memcpy((void *)to, from, n);
+	return 0;
+}
+
+static int get_report(struct snp_guest_dev *snp_dev,
+		      struct snp_guest_request_ioctl *arg,
+		      enum snp_arg_type type)
 {
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_report_resp *resp;
@@ -482,7 +507,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from(&req, arg->req_data, sizeof(req), type))
 		return -EFAULT;
 
 	/*
@@ -501,7 +526,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	if (rc)
 		goto e_free;
 
-	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
 		rc = -EFAULT;
 
 e_free:
@@ -550,7 +575,9 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 	return rc;
 }
 
-static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+static int get_ext_report(struct snp_guest_dev *snp_dev,
+			  struct snp_guest_request_ioctl *arg,
+			  enum snp_arg_type type)
 {
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_ext_report_req req;
@@ -562,7 +589,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from(&req, arg->req_data, sizeof(req), type))
 		return -EFAULT;
 
 	/* userspace does not want certificate data */
@@ -611,14 +638,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (ret)
 		goto e_free;
 
-	if (npages &&
-	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
-			 req.certs_len)) {
+	if (npages && copy_to(req.certs_address, snp_dev->certs_data,
+			      req.certs_len, type)) {
 		ret = -EFAULT;
 		goto e_free;
 	}
 
-	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
 		ret = -EFAULT;
 
 e_free:
@@ -653,13 +679,13 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 
 	switch (ioctl) {
 	case SNP_GET_REPORT:
-		ret = get_report(snp_dev, &input);
+		ret = get_report(snp_dev, &input, SNP_UARG);
 		break;
 	case SNP_GET_DERIVED_KEY:
 		ret = get_derived_key(snp_dev, &input);
 		break;
 	case SNP_GET_EXT_REPORT:
-		ret = get_ext_report(snp_dev, &input);
+		ret = get_ext_report(snp_dev, &input, SNP_UARG);
 		break;
 	default:
 		break;

On 8/14/23 02:43, Dan Williams wrote:
> In preparation for using the TSM key facility to convey attestation blobs
> to userspace, add an argument to flag whether @arg is a user buffer or a
> kernel buffer.
> 
> While TSM keys is meant to replace existing confidenital computing

s/confidenital/confidential/

> ioctl() implementations for attestation report retrieval the old ioctl()
> path needs to stick around for a deprecation period.
> 
> No behavior change intended, just introduce the copy wrappers and @type
> argument.
> 
> Note that these wrappers are similar to copy_{to,from}_sockptr(). If
> this approach moves forward that concept is something that can be
> generalized into a helper with a generic name.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/virt/coco/sev-guest/sev-guest.c |   48 ++++++++++++++++++++++++-------
>   1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 97dbe715e96a..f48c4764a7a2 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -470,7 +470,32 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
>   	return 0;
>   }
>   
> -static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> +enum snp_arg_type {
> +	SNP_UARG,
> +	SNP_KARG,
> +};
> +
> +static unsigned long copy_from(void *to, unsigned long from, unsigned long n,
> +			       enum snp_arg_type type)
> +{
> +	if (type == SNP_UARG)
> +		return copy_from_user(to, (void __user *)from, n);

I'm a fan of blank lines to make reading functions easier. A blank line 
here and below after the memcpy() would be nice.

Ditto in the copy_to() function.

> +	memcpy(to, (void *)from, n);
> +	return 0;
> +}
> +
> +static unsigned long copy_to(unsigned long to, const void *from,
> +			     unsigned long n, enum snp_arg_type type)
> +{
> +	if (type == SNP_UARG)
> +		return copy_to_user((void __user *)to, from, n);
> +	memcpy((void *)to, from, n);
> +	return 0;
> +}
> +
> +static int get_report(struct snp_guest_dev *snp_dev,
> +		      struct snp_guest_request_ioctl *arg,
> +		      enum snp_arg_type type)

You can go out to 100 characters now, so you can put "struct .. *arg" on 
the top line and just put the enum on a new line.

>   {
>   	struct snp_guest_crypto *crypto = snp_dev->crypto;
>   	struct snp_report_resp *resp;
> @@ -482,7 +507,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
>   	if (!arg->req_data || !arg->resp_data)
>   		return -EINVAL;
>   
> -	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> +	if (copy_from(&req, arg->req_data, sizeof(req), type))
>   		return -EFAULT;
>   
>   	/*
> @@ -501,7 +526,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
>   	if (rc)
>   		goto e_free;
>   
> -	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> +	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
>   		rc = -EFAULT;
>   
>   e_free:
> @@ -550,7 +575,9 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
>   	return rc;
>   }
>   
> -static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> +static int get_ext_report(struct snp_guest_dev *snp_dev,
> +			  struct snp_guest_request_ioctl *arg,
> +			  enum snp_arg_type type)

Ditto here on the 100 characters.

>   {
>   	struct snp_guest_crypto *crypto = snp_dev->crypto;
>   	struct snp_ext_report_req req;
> @@ -562,7 +589,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   	if (!arg->req_data || !arg->resp_data)
>   		return -EINVAL;
>   
> -	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> +	if (copy_from(&req, arg->req_data, sizeof(req), type))
>   		return -EFAULT;
>   
>   	/* userspace does not want certificate data */
> @@ -611,14 +638,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   	if (ret)
>   		goto e_free;
>   
> -	if (npages &&
> -	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
> -			 req.certs_len)) {
> +	if (npages && copy_to(req.certs_address, snp_dev->certs_data,
> +			      req.certs_len, type)) {

This can also be a single line now.

Thanks,
Tom

>   		ret = -EFAULT;
>   		goto e_free;
>   	}
>   
> -	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> +	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
>   		ret = -EFAULT;
>   
>   e_free:
> @@ -653,13 +679,13 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
>   
>   	switch (ioctl) {
>   	case SNP_GET_REPORT:
> -		ret = get_report(snp_dev, &input);
> +		ret = get_report(snp_dev, &input, SNP_UARG);
>   		break;
>   	case SNP_GET_DERIVED_KEY:
>   		ret = get_derived_key(snp_dev, &input);
>   		break;
>   	case SNP_GET_EXT_REPORT:
> -		ret = get_ext_report(snp_dev, &input);
> +		ret = get_ext_report(snp_dev, &input, SNP_UARG);
>   		break;
>   	default:
>   		break;
>

Tom Lendacky wrote:
> On 8/14/23 02:43, Dan Williams wrote:
> > In preparation for using the TSM key facility to convey attestation blobs
> > to userspace, add an argument to flag whether @arg is a user buffer or a
> > kernel buffer.
> > 
> > While TSM keys is meant to replace existing confidenital computing
> 
> s/confidenital/confidential/
> 
> > ioctl() implementations for attestation report retrieval the old ioctl()
> > path needs to stick around for a deprecation period.
> > 
> > No behavior change intended, just introduce the copy wrappers and @type
> > argument.
> > 
> > Note that these wrappers are similar to copy_{to,from}_sockptr(). If
> > this approach moves forward that concept is something that can be
> > generalized into a helper with a generic name.
> > 
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   drivers/virt/coco/sev-guest/sev-guest.c |   48 ++++++++++++++++++++++++-------
> >   1 file changed, 37 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > index 97dbe715e96a..f48c4764a7a2 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -470,7 +470,32 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> >   	return 0;
> >   }
> >   
> > -static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> > +enum snp_arg_type {
> > +	SNP_UARG,
> > +	SNP_KARG,
> > +};
> > +
> > +static unsigned long copy_from(void *to, unsigned long from, unsigned long n,
> > +			       enum snp_arg_type type)
> > +{
> > +	if (type == SNP_UARG)
> > +		return copy_from_user(to, (void __user *)from, n);
> 
> I'm a fan of blank lines to make reading functions easier. A blank line 
> here and below after the memcpy() would be nice.
> 
> Ditto in the copy_to() function.
> 
> > +	memcpy(to, (void *)from, n);
> > +	return 0;
> > +}
> > +
> > +static unsigned long copy_to(unsigned long to, const void *from,
> > +			     unsigned long n, enum snp_arg_type type)
> > +{
> > +	if (type == SNP_UARG)
> > +		return copy_to_user((void __user *)to, from, n);
> > +	memcpy((void *)to, from, n);
> > +	return 0;
> > +}
> > +
> > +static int get_report(struct snp_guest_dev *snp_dev,
> > +		      struct snp_guest_request_ioctl *arg,
> > +		      enum snp_arg_type type)
> 
> You can go out to 100 characters now, so you can put "struct .. *arg" on 
> the top line and just put the enum on a new line.
> 
> >   {
> >   	struct snp_guest_crypto *crypto = snp_dev->crypto;
> >   	struct snp_report_resp *resp;
> > @@ -482,7 +507,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
> >   	if (!arg->req_data || !arg->resp_data)
> >   		return -EINVAL;
> >   
> > -	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> > +	if (copy_from(&req, arg->req_data, sizeof(req), type))
> >   		return -EFAULT;
> >   
> >   	/*
> > @@ -501,7 +526,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
> >   	if (rc)
> >   		goto e_free;
> >   
> > -	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> > +	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
> >   		rc = -EFAULT;
> >   
> >   e_free:
> > @@ -550,7 +575,9 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
> >   	return rc;
> >   }
> >   
> > -static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> > +static int get_ext_report(struct snp_guest_dev *snp_dev,
> > +			  struct snp_guest_request_ioctl *arg,
> > +			  enum snp_arg_type type)
> 
> Ditto here on the 100 characters.
> 
> >   {
> >   	struct snp_guest_crypto *crypto = snp_dev->crypto;
> >   	struct snp_ext_report_req req;
> > @@ -562,7 +589,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
> >   	if (!arg->req_data || !arg->resp_data)
> >   		return -EINVAL;
> >   
> > -	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> > +	if (copy_from(&req, arg->req_data, sizeof(req), type))
> >   		return -EFAULT;
> >   
> >   	/* userspace does not want certificate data */
> > @@ -611,14 +638,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
> >   	if (ret)
> >   		goto e_free;
> >   
> > -	if (npages &&
> > -	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
> > -			 req.certs_len)) {
> > +	if (npages && copy_to(req.certs_address, snp_dev->certs_data,
> > +			      req.certs_len, type)) {
> 
> This can also be a single line now.

So the kernel's .clang-format template still enforces the 80 column
limit. I am ok to bump that to 100 temporarily for my edits to this
file, but in general I tend to just let clang-format do its thing. One
less thing to worry about.

>
>         switch (ioctl) {
>         case SNP_GET_REPORT:
> -               ret = get_report(snp_dev, &input);
> +               ret = get_report(snp_dev, &input, SNP_UARG);
>                 break;
>         case SNP_GET_DERIVED_KEY:
>                 ret = get_derived_key(snp_dev, &input);
>                 break;

Do we have an agreement around the continued existence of sev-guest
for supporting derived keys, is that similarly slated for the chopping
block, or is it left undecided?
It appears your choice to not include the uarg/karg extension here is
deliberate.

>         case SNP_GET_EXT_REPORT:
> -               ret = get_ext_report(snp_dev, &input);
> +               ret = get_ext_report(snp_dev, &input, SNP_UARG);
>                 break;
>         default:
>                 break;
>

Reviewed-by: Dionna Glaze <dionnaglaze@google.com>

-- 
-Dionna Glaze, PhD (she/her)

Dionna Amalie Glaze wrote:
> >
> >         switch (ioctl) {
> >         case SNP_GET_REPORT:
> > -               ret = get_report(snp_dev, &input);
> > +               ret = get_report(snp_dev, &input, SNP_UARG);
> >                 break;
> >         case SNP_GET_DERIVED_KEY:
> >                 ret = get_derived_key(snp_dev, &input);
> >                 break;
> 
> Do we have an agreement around the continued existence of sev-guest
> for supporting derived keys, is that similarly slated for the chopping
> block, or is it left undecided?
> It appears your choice to not include the uarg/karg extension here is
> deliberate.

I do want to understand the argument from James a bit more, but the
exlcusion here was simply because there is currently no support for this
concept from other vendors.

That said, if it remains a vendor unique concept and continues getting
suspicious looks from folks like James, it may indeed be something the
kernel wants to jettison.

> >         case SNP_GET_EXT_REPORT:
> > -               ret = get_ext_report(snp_dev, &input);
> > +               ret = get_ext_report(snp_dev, &input, SNP_UARG);
> >                 break;
> >         default:
> >                 break;
> >
> 
> Reviewed-by: Dionna Glaze <dionnaglaze@google.com>

Thanks for all your help on this!

On 8/14/23 18:24, Dan Williams wrote:
> Dionna Amalie Glaze wrote:
>>>
>>>          switch (ioctl) {
>>>          case SNP_GET_REPORT:
>>> -               ret = get_report(snp_dev, &input);
>>> +               ret = get_report(snp_dev, &input, SNP_UARG);
>>>                  break;
>>>          case SNP_GET_DERIVED_KEY:
>>>                  ret = get_derived_key(snp_dev, &input);
>>>                  break;
>>
>> Do we have an agreement around the continued existence of sev-guest
>> for supporting derived keys, is that similarly slated for the chopping
>> block, or is it left undecided?
>> It appears your choice to not include the uarg/karg extension here is
>> deliberate.
> 
> I do want to understand the argument from James a bit more, but the
> exlcusion here was simply because there is currently no support for this
> concept from other vendors.
> 
> That said, if it remains a vendor unique concept and continues getting
> suspicious looks from folks like James, it may indeed be something the
> kernel wants to jettison.

I'm not sure why we would want to jettison it. Just because other vendors 
don't have a key derivation function doesn't mean it can't be useful to 
customers that want to use it on AMD platforms.

Thanks,
Tom

> 
>>>          case SNP_GET_EXT_REPORT:
>>> -               ret = get_ext_report(snp_dev, &input);
>>> +               ret = get_ext_report(snp_dev, &input, SNP_UARG);
>>>                  break;
>>>          default:
>>>                  break;
>>>
>>
>> Reviewed-by: Dionna Glaze <dionnaglaze@google.com>
> 
> Thanks for all your help on this!

Tom Lendacky wrote:
> On 8/14/23 18:24, Dan Williams wrote:
> > Dionna Amalie Glaze wrote:
> >>>
> >>>          switch (ioctl) {
> >>>          case SNP_GET_REPORT:
> >>> -               ret = get_report(snp_dev, &input);
> >>> +               ret = get_report(snp_dev, &input, SNP_UARG);
> >>>                  break;
> >>>          case SNP_GET_DERIVED_KEY:
> >>>                  ret = get_derived_key(snp_dev, &input);
> >>>                  break;
> >>
> >> Do we have an agreement around the continued existence of sev-guest
> >> for supporting derived keys, is that similarly slated for the chopping
> >> block, or is it left undecided?
> >> It appears your choice to not include the uarg/karg extension here is
> >> deliberate.
> > 
> > I do want to understand the argument from James a bit more, but the
> > exlcusion here was simply because there is currently no support for this
> > concept from other vendors.
> > 
> > That said, if it remains a vendor unique concept and continues getting
> > suspicious looks from folks like James, it may indeed be something the
> > kernel wants to jettison.
> 
> I'm not sure why we would want to jettison it. Just because other vendors 
> don't have a key derivation function doesn't mean it can't be useful to 
> customers that want to use it on AMD platforms.

Definitely, instead it was this comment from James that gave me pause:

"To get a bit off topic, I'm not sure derived keys are much use.  The
problem is in SNP that by the time the PSP does the derivation, the key
is both tied to the physical system and derived from a measurement too
general to differentiate between VM images (so one VM could read
another VMs stored secrets)."

http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com

> Definitely, instead it was this comment from James that gave me pause:
>
> "To get a bit off topic, I'm not sure derived keys are much use.  The
> problem is in SNP that by the time the PSP does the derivation, the key
> is both tied to the physical system and derived from a measurement too
> general to differentiate between VM images (so one VM could read
> another VMs stored secrets)."
>

Key derivation on AMD SEV-SNP is not necessarily tied to a physical
system with the introduction of VLEK-based attestation. It's now tied
to a CSP's fleet of machines. We can use key derivation in the SVSM as
a basis for further key derivation based on measurement registers, so
the utility increases to provide something like persisted sealed data
that can only be unsealed when the SVSM witnesses a particular runtime
measurement configuration.
We can use NIST 800-90A Rev. 1 for combining keys from the PSP with
measurement register values for example.

> http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com




--
-Dionna Glaze, PhD (she/her)