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

[Brijesh Singh]

Hi Boris,


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

>> +
>> +	/* 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()...
>
> ...
>
>>  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.
>
>> +
>>  	misc = &snp_dev->misc;
>>  	misc->minor = MISC_DYNAMIC_MINOR;
>>  	misc->name = DEVICE_NAME;
>