Re: [PATCH v15 01/13] x86/sev: Carve out and export SNP guest messaging init routines

["Nikunj A. Dadhania" <nikunj@amd.com>]

On 12/3/2024 7:49 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 02:30:33PM +0530, Nikunj A Dadhania wrote:
 
>> @@ -2667,3 +2662,179 @@ static int __init sev_sysfs_init(void)
>>  }
>>  arch_initcall(sev_sysfs_init);
>>  #endif // CONFIG_SYSFS
>> +
>> +static void free_shared_pages(void *buf, size_t sz)
>> +{
>> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>> +	int ret;
>> +
>> +	if (!buf)
>> +		return;
>> +
>> +	ret = set_memory_encrypted((unsigned long)buf, npages);
>> +	if (ret) {
>> +		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
> 
> Looking at where this lands:
> 
> set_memory_encrypted
> |-> __set_memory_enc_dec
> 
> and that doing now:
> 
>         if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
>                 if (!down_read_trylock(&mem_enc_lock))
>                         return -EBUSY;
> 
> 
> after
> 
> 859e63b789d6 ("x86/tdx: Convert shared memory back to private on kexec")
> 
> we probably should pay attention to this here firing and maybe turning that
> _trylock() into a normal down_read*
> 
> Anyway, just something to pay attention to in the future.

Yes, will keep an eye.

> 
>> +		return;
>> +	}
>> +
>> +	__free_pages(virt_to_page(buf), get_order(sz));
>> +}
> 
> ...
> 
>> +struct snp_msg_desc *snp_msg_alloc(void)
>> +{
>> +	struct snp_msg_desc *mdesc;
>> +	void __iomem *mem;
>> +
>> +	BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
>> +
>> +	mdesc = kzalloc(sizeof(struct snp_msg_desc), GFP_KERNEL);
> 
> The above ones use GFP_KERNEL_ACCOUNT. What's the difference?

The above ones I have retained old code.

GFP_KERNEL_ACCOUNT allocation are accounted in kmemcg and the below note from[1]
----------------------------------------------------------------------------
Untrusted allocations triggered from userspace should be a subject of kmem
accounting and must have __GFP_ACCOUNT bit set. There is the handy
GFP_KERNEL_ACCOUNT shortcut for GFP_KERNEL allocations that should be accounted.
----------------------------------------------------------------------------

For mdesc, I had kept it similar to snp_dev allocation, that is why it is 
having GFP_KERNEL.

        snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
        if (!snp_dev)
-               goto e_unmap;
-
-       mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);

Let me know if mdesc allocation need to be GFP_KERNEL_ACCOUNT.

>> +void snp_msg_free(struct snp_msg_desc *mdesc)
>> +{
>> +	if (!mdesc)
>> +		return;
>> +
>> +	mdesc->vmpck = NULL;
>> +	mdesc->os_area_msg_seqno = NULL;
> 
> 	memset(mdesc, ...);
> 
> at the end instead of those assignments.

Sure.

> 
>> +	kfree(mdesc->ctx);
>> +
>> +	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
>> +	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
>> +	iounmap((__force void __iomem *)mdesc->secrets);
> 
> 
>> +	kfree(mdesc);
>> +}
>> +EXPORT_SYMBOL_GPL(snp_msg_free);
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index b699771be029..5268511bc9b8 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> 
> ...
> 
>> @@ -993,115 +898,57 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>>  		return -ENODEV;
>>  
>> -	if (!dev->platform_data)
>> -		return -ENODEV;
>> -
>> -	data = (struct sev_guest_platform_data *)dev->platform_data;
>> -	mapping = ioremap_encrypted(data->secrets_gpa, PAGE_SIZE);
>> -	if (!mapping)
>> -		return -ENODEV;
>> -
>> -	secrets = (__force void *)mapping;
>> -
>> -	ret = -ENOMEM;
>>  	snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
>>  	if (!snp_dev)
>> -		goto e_unmap;
>> -
>> -	mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
>> -	if (!mdesc)
>> -		goto e_unmap;
>> -
>> -	/* Adjust the default VMPCK key based on the executing VMPL level */
>> -	if (vmpck_id == -1)
>> -		vmpck_id = snp_vmpl;
>> +		return -ENOMEM;
>>  
>> -	ret = -EINVAL;
>> -	mdesc->vmpck = get_vmpck(vmpck_id, secrets, &mdesc->os_area_msg_seqno);
>> -	if (!mdesc->vmpck) {
>> -		dev_err(dev, "Invalid VMPCK%d communication key\n", vmpck_id);
>> -		goto e_unmap;
>> -	}
>> +	mdesc = snp_msg_alloc();
>> +	if (IS_ERR_OR_NULL(mdesc))
>> +		return -ENOMEM;
>>  
>> -	/* Verify that VMPCK is not zero. */
>> -	if (is_vmpck_empty(mdesc)) {
>> -		dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id);
>> -		goto e_unmap;
>> -	}
>> +	ret = snp_msg_init(mdesc, vmpck_id);
>> +	if (ret)
>> +		return -EIO;
> 
> You just leaked mdesc here.

Right

> Audit all your error paths.

Sure I will audit and send updated patch.

Regards
Nikunj


1) https://www.kernel.org/doc/html/v6.12/core-api/memory-allocation.html