Re: [PATCH Part1 v5 38/38] virt: sevguest: Add support to get extended report

Hi Boris,


On 9/8/21 12:53 PM, Borislav Petkov wrote:

quoted

+
+	/* If certs length is invalid then copy the returned length */
+	if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
+		req.certs_len = input.data_npages << PAGE_SHIFT;
+
+		if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
+			ret = -EFAULT;
+
+		goto e_free;
+	}
+
+	if (ret)
+		goto e_free;

This one is really confusing. You assign ret in the if branch
above but then you test ret outside too, just in case the
snp_issue_guest_request() call above has failed.

But then if that call has failed, you still go and do some cleanup work
for invalid certs length...

So that get_ext_report() function is doing too many things at once and
is crying to be split.

I will try to see what I can come up with to make it easy to read.

For example, the glue around snp_issue_guest_request() is already carved
out in handle_guest_request(). Why aren't you calling that function here
too?

The handle_guest_request() uses the VMGEXIT_GUEST_REQUEST which does not
require the memory for the certificate blobs etc. But based on your
earlier comment that we should let the driver use the VMGEXIT code
rather than enum will help in this case. I will be reworking
handle_guest_request() so that it can be used for both the cases (with
and without certificate).

That'll keep the enc, request, dec payload game separate and then the
rest of the logic can remain in get_ext_report()...

...

quoted

 static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
 {
 	struct snp_guest_dev *snp_dev = to_snp_dev(file);

@@ -368,6 +480,10 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 		ret = get_derived_key(snp_dev, &input);
 		break;
 	}
+	case SNP_GET_EXT_REPORT: {
+		ret = get_ext_report(snp_dev, &input);
+		break;
+	}
 	default:
 		break;
 	}

@@ -453,6 +569,12 @@ static int __init snp_guest_probe(struct platform_device *pdev)
 		goto e_free_req;
 	}
 
+	snp_dev->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE);
+	if (IS_ERR(snp_dev->certs_data)) {
+		ret = PTR_ERR(snp_dev->certs_data);
+		goto e_free_resp;
+	}

Same comments here as for patch 37.

quoted

+
 	misc = &snp_dev->misc;
 	misc->minor = MISC_DYNAMIC_MINOR;
 	misc->name = DEVICE_NAME;