Re: [PATCH v9 41/43] virt: Add SEV-SNP guest driver

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

On 2/6/22 4:39 PM, Borislav Petkov wrote:

> And once you do, do "make htmldocs" to see whether it complains about
> some formatting issues or other errors etc.
> 
> /me goes and does it...
> 
> Ah, here we go:
> 
> Documentation/virt/coco/sevguest.rst:48: WARNING: Inline emphasis start-string without end-string.
> Documentation/virt/coco/sevguest.rst:51: WARNING: Inline emphasis start-string without end-string.
> Documentation/virt/coco/sevguest.rst:55: WARNING: Inline emphasis start-string without end-string.
> Documentation/virt/coco/sevguest.rst:57: WARNING: Definition list ends without a blank line; unexpected unindent.
> 
> There's something it doesn't like about the struct. Yeah, when I look at
> the html output, it is all weird and not monospaced...

I will fix those in next rev.


>> +
>> +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device.
>> +The ioctl accepts struct snp_user_guest_request. The input and output structure is
>> +specified through the req_data and resp_data field respectively. If the ioctl fails
>> +to execute due to a firmware error, then fw_err code will be set otherwise the
>> +fw_err will be set to 0xff.
> 
> fw_err is u64. What does 0xff mean? Everything above the least
> significant byte is reserved 0?
> 

Yep, I will explicitly say that it should be set to 0x00000000000000FF.


>> diff --git a/drivers/virt/coco/sevguest/Kconfig b/drivers/virt/coco/sevguest/Kconfig
>> new file mode 100644
>> index 000000000000..07ab9ec6471c
>> --- /dev/null
>> +++ b/drivers/virt/coco/sevguest/Kconfig
>> @@ -0,0 +1,12 @@
>> +config SEV_GUEST
>> +	tristate "AMD SEV Guest driver"
>> +	default y
> 
> Definitely not. We don't enable drivers by default unless they're
> ubiquitous.
> 

Randy asked me similar question on v7, and here is my response to it.

https://lore.kernel.org/linux-mm/e6b412e4-f38e-d212-f52a-e7bdc9a26eff@infradead.org/

Let me know if you still think that we should make it 'n'. I am not dead 
against it but I have feeling that once distro's starts building SNP 
aware guest kernel, then we may get asked to enable it by default so 
that attestation report can be obtained by the initial ramdisk.



>> +	 */
>> +	if (count >= UINT_MAX) {
>> +		pr_err_ratelimited("SNP guest request message sequence counter overflow\n");
> 
> How does error message help the user? Is she supposed to reboot the
> machine or so?
> 
> Because it sounds to me like if this goes over 32-bit, this driver stops
> working. So what resets those sequence numbers?

After this condition is met, a guest will no longer get the attestation 
report. It's up to the userspace to reboot the guest or continue without 
attestation.

The only thing that will reset the counter is re-launching the guest to 
go through the entire PSP initialization sequence once again.


>> +
>> +	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");
> 
> And? < 12 is bad? Make that error message more user-friendly pls.
> 
Okay.


> 
> The order of those free calls needs to be the opposite of the kmallocs
> above.
> 
Okay


>> +
>> +	/* Message version must be non-zero */
>> +	if (!input.msg_version)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&snp_cmd_mutex);
> 
> That mutex probably is to be held while issuing SNP commands but then
> you hold it here already for...
> 
>> +
>> +	/* Check if the VMPCK is not empty */
>> +	if (is_vmpck_empty(snp_dev)) {
> 
> ... this here which is not really a SNP command issuing.
> 
> Should that mutex be grabbed only around handle_guest_request() above or
> is it supposed to protect more stuff?


Yep, it need to protect more stuff.

We allocate a shared buffers (request, response, cert-chain) that gets 
populated before issuing the command, and then we copy the result from 
reponse shared to callers buffer after the command completes. So, we 
also want to ensure that the shared buffer is not touched before the 
previous ioctl is finished.


-Brijesh