linux-kernel - Re: [PATCH Part2 RFC v2 16/37] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 10 May 2021 15:07:21 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm list <kvm@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, jroedel@...e.de,
        "Lendacky, Thomas" <thomas.lendacky@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...el.com>,
        David Rientjes <rientjes@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>, peterz@...radead.org,
        "H. Peter Anvin" <hpa@...or.com>, tony.luck@...el.com
Subject: Re: [PATCH Part2 RFC v2 16/37] crypto: ccp: Handle the legacy TMR
 allocation when SNP is enabled


On 5/10/21 1:23 PM, Peter Gonda wrote:
>> +
>> +static int snp_set_rmptable_state(unsigned long paddr, int npages,
>> +                                 struct rmpupdate *val, bool locked, bool need_reclaim)
>> +{
>> +       unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
>> +       unsigned long pfn_end = pfn + npages;
>> +       int rc;
>> +
>> +       while (pfn < pfn_end) {
>> +               if (need_reclaim)
>> +                       if (snp_reclaim_page(pfn_to_page(pfn), locked))
>> +                               return -EFAULT;
>> +
>> +               rc = rmpupdate(pfn_to_page(pfn), val);
>> +               if (rc)
>> +                       return rc;
> This functional can return an error but have partially converted some
> of the npages requested by the caller. Should this function return the
> number of affected pages or something to allow the caller to know if
> some pages need to be reverted? Or should the function attempt to do
> that itself?

I will look into improving this function to cleanup the partial updates
on the failure. Thanks


>
>> +
>> +               pfn++;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void __snp_free_firmware_pages(struct page *page, int order)
>> +{
>> +       struct rmpupdate val = {};
>> +       unsigned long paddr;
>> +
>> +       if (!page)
>> +               return;
>> +
>> +       paddr = __pa((unsigned long)page_address(page));
>> +
>> +       if (snp_set_rmptable_state(paddr, 1 << order, &val, false, true))
>> +               return;
> We now have leaked the given pages right? Should some warning be
> logged or should we track these leaked pages and maybe try and free
> them with a kworker?

I will add the log about it. Only reason I can think of this function
failing is if the firmware fails to clear the immutable bit from the
page, If it did then I don't see any reason why a kworker retry will
succeed. Per the SNP firmware spec, the firmware should be able to clear
immutable bit as long as the firmware is in the INIT state.


>
>> +
>> +       __free_pages(page, order);
>> +}
>> +