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@xxxxxxxxxxxxx/
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
