Re: [PATCH Part1 RFC v3 22/22] virt: Add SEV-SNP guest driver

[Brijesh Singh <brijesh.singh@amd.com>]

On 6/30/2021 8:35 AM, Borislav Petkov wrote:
> 
> 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.
> 
Sounds good to me.

>>
>> +	depends on AMD_MEM_ENCRYPT
>> +	help
>> +	  Provides AMD SNP guest request driver. The driver can be used by the
> 
> 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?

Yes, the guest communicates directly with the PSP through the hypervisor. I will fix
the wording.

> 
>> +	  and more.
>> +
>> +	  If you choose 'M' here, this module will be called sevguest.
>> diff --git a/drivers/virt/sevguest/Makefile b/drivers/virt/sevguest/Makefile
>> new file mode 100644
>> index 000000000000..1505df437682
>> --- /dev/null
>> +++ b/drivers/virt/sevguest/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +sevguest-y := snp.o
> 
> 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.
> 

As you have noticed that Dov is submitting the SEV specific driver. I was thinking that 
it will be nice if we have one driver that covers both the SEV and SEV-SNP. That driver
can be called "sevguest". The kernel will install the appropriate platform device. The
sevguest driver can probe for both the "sev-guest" and "snp-guest" and delegate the
ioctl handling accordingly.

In the kernel the directory structure may look like this:

virt/coco/sevguest
  sevguest.c       // common code
  snp.c            // SNP specific ioctl implementation
  sev.c            // SEV specific ioctl or sysfs implementation

Thoughts ?

>> +	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);
> 
> 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.
> 

Noted.

>> +	if (IS_ERR(crypto->tfm))
>> +		goto e_free;
>> +
>> +	if (crypto_aead_setkey(crypto->tfm, key, keylen))
>> +
>> +	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.

Noted.

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

Let me work to go through it and make sure that we don't get into NULL
deference situtation.

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

I am working to add support for "extended guest request" that will make it 3 ioctl.

>> +
>> +struct snp_user_guest_request {
>> +	/* Message version number (must be non-zero) */
>> +	__u8 msg_version;
>> +	__u64 data;
>> +
>> +	/* firmware error code on failure (see psp-sev.h) */
>> +	__u32 fw_err;
>> +};
> 
> All those struct names have a "snp_user" prefix. It seems to me that
> that "user" is superfluous.
> 

I followed the naming convension you recommended during the initial SEV driver
developement. IIRC, the main reason for us having to add "user" in it because
we wanted to distinguious that this structure is not exactly same as the what
is defined in the SEV-SNP firmware spec.


>> +
>> +#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?

Good question, I am not able to find a generic place to document it. Should we
create a documentation "Documentation/virt/coco/sevguest-api.rst" for it ? I am
open to other suggestions. 

-Brijesh