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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8965fa19-8a9b-403e-a542-8566f30f3fee@amd.com>
Date: Thu, 5 Dec 2024 11:53:53 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
 kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
 dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
 pbonzini@...hat.com
Subject: Re: [PATCH v15 01/13] x86/sev: Carve out and export SNP guest
 messaging init routines

On 12/5/2024 1:32 AM, Borislav Petkov wrote:
> On Wed, Dec 04, 2024 at 03:30:13PM +0530, Nikunj A. Dadhania wrote:
>> The above ones I have retained old code.
> 
> Right.
> 
>> 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.
>> ----------------------------------------------------------------------------
> 
> Interesting.
> 
>> 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.
> 
> Let's audit that thing:
> 
> * snp_init_crypto - not really untrusted allocation. It is on the driver probe
> path.
> 
> * get_report - I don't think so:
> 
>         /*      
>          * The intermediate response buffer is used while decrypting the
>          * response payload. Make sure that it has enough space to cover the
>          * authtag.
>          */
>         resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize;
>         report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
> 
> That resp_len is limited and that's on the guest_ioctl path which cannot
> happen concurrently?

It is a trusted allocation, but should it be accounted as it is part of
the userspace ioctl path ?

> 
> * get_ext_report - ditto
> 
> * alloc_shared_pages - all the allocations are limited but I guess that could
> remain _ACCOUNT as a measure for future robustness.

Ok.

> And that was it.
> 
> So AFAICT, only one use case is semi-valid.
> 
> So maybe we should convert those remaining ones to boring GFP_KERNEL...
> 

Sure, let me add this as a pre-patch.

Regards,
Nikunj