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
