Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy <aik@amd.com>]

On 19/10/23 15:43, Dan Williams wrote:
> Alexey Kardashevskiy wrote:
> [..]
>>> Yes, it's been allowed in loop declaration for a few kernels now and the
>>> new __free() helper in cleanup.h will make this model more prominent. My
>>> sense is that it is still not open season on mid-function declarations,
>>> but __attribute__((__cleanup__())) usage needs it.
>>>
>>>> Since you are doing this, move zero_ent below. Or, better, use
>>>> guid_is_null().
>>>
>>> guid_is_null() is not techinically enough given the specification
>>> mandates that the entire entry is zero.
>>
>> Well technically it says that guid, offset and length must be zeroes.
>> So, (guid_is_null() && !offset && !length) and no yet another static
>> declaration of a bunch of zeroes far away. I even wonder if there is a
>> helper to check if some memory is all zeroes :) Up to you.
> 
> Ok, I switched to comparing each field to zero.
> 
> [..]
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	memcpy(&hdr, buf, sizeof(hdr));
>>>>> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
>>>>> +		return -EINVAL;
>>>>> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
>>>>> +		return -EINVAL;
>>>>> +	if (hdr.status)
>>>>> +		return -ENXIO;
>>>>> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
>>>>> +	if (!rbuf)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
>>>>> +	report->outblob = no_free_ptr(rbuf);
>>>>> +	report->outblob_len = hdr.report_size;
>>>>> +
>>>>> +	certs_size = 0;
>>>>> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
>>>>> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
>>>>> +			break;
>>>>> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
>>>>> +	}
>>>>> +
>>>>> +	/* No certs to report */
>>>>> +	if (!certs_size)
>>>>
>>>> Nit: WARN_ON_ONCE(i) here?
>>>
>>> Seems harsh for what could only be a firmware bug, panic_on_warn users
>>> would not appreciate crashing the kernel over something recoverable like
>>> this.
>>
>> This would a HV bug as certificates come from the KVM. And it is
>> (slightly) more likely that the HV is trying to trigger buffer overrun
>> in the guest.
> 
> Added a dev_warn_ratelimited(). WARN_ON_ONCE() is too heavy as I expect
> panic_on_warn policy has more reason to be deployed in a confidential VM
> than other places. >>>>
>>>>> +		return 0;
>>>>> +
>>>>> +	/*
>>>>> +	 * cert_table reports more data than fits in ext_size the
>>>>> +	 * userspace cert_table walker can decide what happens next,
>>>>> +	 * truncate the output
>>>>> +	 */
>>>>> +	if (certs_size > ext_size)
>>>>> +		certs_size = ext_size;
>>>>
>>>> This sounds more like the HV provided a broken table with offset(s)
>>>> ouside of the certs buffer. The HV is expected instead return
>>>> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
>>>> guest to retry.
>>>
>>> The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
>>> prepared to retry. Retry support would be a follow-on new capability.
>>
>> My point is that you should not get into the situation when this
>> calculated certs_size is greater than ext_size. If this is the case
>> because someone sent too many certificaties via /dev/sev or kvmfd on the
>> host, the GHCB call won't return any certs and will ask for a retry instead.
> 
> I understand, but given this needs to walk the entries anyway to size
> the buffer correctly this sanity check is "free".

I do not mind the check at all but this hides a potential bug or 
malicious misbehavior attempt. I'd think a CoCo VM is very paranoid 
about such things.


>>>>> +
>>>>> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>>>> +	if (!cbuf)
>>>>> +		return -ENOMEM;
>>>>
>>>> In a such (unlikely) event the function returns an error but does not
>>>> free report->outblob which is going to leak if consequent call succeded.
>>>> This new no_free_ptr business is confusing at times :(
>>>
>>> If this fails it results in the attribute read failing and
>>> read_generation does not advance. The next read attempt will free the
>>> partially completed report and retry,
>>
>> Ah ok. In general, it just feels like every use of no_free_ptr() defeats
>> the whole purpose of __free(xxx).
> 
> no_free_ptr() is there to say "we correctly populated this buffer,
> there are no more error returns in this function, transition the
> responsibility of freeing this buffer to the object it was assigned".


That's what I thought but there is one error return between the first 
no_free_ptr() and "return 0". I agree there is no outblob leak after all 
but the blob hangs around for some time and all this __free() machinery 
is supposed to auto-clean everything right on the spot before returning 
-ENOMEM. Grouping all these no_free_ptr() in the end would make it 
cleaner... Again, I do not insist.

And btw thanks for doing this! I never liked those ioctls :)


-- 
Alexey