Hi Boris,
On 3/3/22 09:28, Borislav Petkov wrote:
On Thu, Feb 24, 2022 at 10:56:24AM -0600, Brijesh Singh wrote:
+static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+{
+ struct snp_guest_crypto *crypto = snp_dev->crypto;
+ struct snp_ext_report_req req = {0};
+ struct snp_report_resp *resp;
+ int ret, npages = 0, resp_len;
+
+ lockdep_assert_held(&snp_cmd_mutex);
+
+ if (!arg->req_data || !arg->resp_data)
+ return -EINVAL;
+
+ if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ return -EFAULT;
+
+ if (req.certs_len) {
+ if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
+ !IS_ALIGNED(req.certs_len, PAGE_SIZE))
+ return -EINVAL;
+ }
+
+ if (req.certs_address && req.certs_len) {
+ if (!access_ok(req.certs_address, req.certs_len))
+ return -EFAULT;
+
+ /*
+ * Initialize the intermediate buffer with all zeros. This buffer
+ * is used in the guest request message to get the certs blob from
+ * the host. If host does not supply any certs in it, then copy
+ * zeros to indicate that certificate data was not provided.
+ */
+ memset(snp_dev->certs_data, 0, req.certs_len);
+
+ npages = req.certs_len >> PAGE_SHIFT;
+ }
I think all those checks should be made more explicit. This makes the
code a lot more readable and straight-forward (pasting the full excerpt
because the incremental diff ontop is less readable):
...
if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
return -EFAULT;
if (!req.certs_len || !req.certs_address)
return -EINVAL;
if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
!IS_ALIGNED(req.certs_len, PAGE_SIZE))
return -EINVAL;
I did not fail on !req.cert_len, because my read of the GHCB spec says
that additional data (certificate blob) is optional. A user could call
SNP_GET_EXT_REPORT without asking for the extended certificate. In this
case, SNP_GET_EXT_REPORT == SNP_GET_REPORT.
Text from the GHCB spec section 4.1.8
---------------
https://developer.amd.com/wp-content/resources/56421.pdf
The SNP Extended Guest Request NAE event is very similar to the SNP
Guest Request NAE event. The difference is related to the additional
data that can be returned based on the guest request. Any SNP Guest
Request that does not support returning additional data must execute as
if invoked as an SNP Guest Request.
--------------
if (!access_ok(req.certs_address, req.certs_len))
return -EFAULT;
/*
* Initialize the intermediate buffer with all zeros. This buffer
* is used in the guest request message to get the certs blob from
* the host. If host does not supply any certs in it, then copy
* zeros to indicate that certificate data was not provided.
*/
memset(snp_dev->certs_data, 0, req.certs_len);
npages = req.certs_len >> PAGE_SHIFT;
