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
