linux-kernel - Re: [PATCH 05/11] x86/sev: Perform PVALIDATE using the SVSM when not at VMPL0

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]

Message-ID: <3399426b-57f3-17b2-9fa2-9244ee13542e@amd.com>
Date: Sat, 27 Jan 2024 09:18:24 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Dionna Amalie Glaze <dionnaglaze@...gle.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
 Peter Zijlstra <peterz@...radead.org>,
 Dan Williams <dan.j.williams@...el.com>, Michael Roth
 <michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>
Subject: Re: [PATCH 05/11] x86/sev: Perform PVALIDATE using the SVSM when not
 at VMPL0

On 1/26/24 18:59, Dionna Amalie Glaze wrote:
> On Fri, Jan 26, 2024 at 2:18 PM Tom Lendacky <thomas.lendacky@....com> wrote:
>>
>> The PVALIDATE instruction can only be performed at VMPL0. An SVSM will
>> be present when running at VMPL1 or a lower privilege level.
>>
>> When an SVSM is present, use the SVSM_CORE_PVALIDATE call to perform
>> memory validation instead of issuing the PVALIDATE instruction directly.
>>
>> The validation of a single 4K page is now explicitly identified as such
>> in the function name, pvalidate_4k_page(). The pvalidate_pages() function
>> is used for validating 1 or more pages at either 4K or 2M in size. Each
>> function, however, determines whether it can issue the PVALIDATE directly
>> or whether the SVSM needs to be invoked.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>   arch/x86/boot/compressed/sev.c |  42 +++++++-
>>   arch/x86/include/asm/sev.h     |  22 +++++
>>   arch/x86/kernel/sev-shared.c   | 176 ++++++++++++++++++++++++++++++++-
>>   arch/x86/kernel/sev.c          |  25 +++--
>>   4 files changed, 247 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
>> index 5d2403914ceb..3fbb614c31e0 100644
>> --- a/arch/x86/boot/compressed/sev.c
>> +++ b/arch/x86/boot/compressed/sev.c

>> +static int svsm_protocol(struct svsm_call *call)
>> +{
>> +       struct ghcb *ghcb;
>> +       int ret;
>> +
>> +       if (boot_ghcb)
>> +               ghcb = boot_ghcb;
>> +       else
>> +               ghcb = NULL;
>> +
>> +       do {
>> +               ret = ghcb ? __svsm_ghcb_protocol(ghcb, call)
>> +                          : __svsm_msr_protocol(call);
>> +       } while (ret == SVSM_ERR_BUSY);
> 
> Should this loop forever or eventually give up and panic?

The question becomes how many times before giving up. This would likely 
only happen if the hypervisor is causing an issue for the SVSM, so it 
would be similar to a DoS. I'm open to suggestions, though.

On a side not, that does remind that this should also be checking for 
SVSM_ERR_INCOMPLETE.

> 

>>   void snp_set_page_private(unsigned long paddr)
>> @@ -261,6 +289,12 @@ void sev_es_shutdown_ghcb(void)
>>          if (!sev_es_check_cpu_features())
>>                  error("SEV-ES CPU Features missing.");
>>
>> +       /*
>> +        * Ensure that the boot GHCB isn't used for the PVALIDATE when running
> 
> Why the definite article? Which PVALIDATE is this referring to?

I'll clarify the comment, but it is specifically relates to the 
set_page_encrypted() for the boot_ghcb_page that immediately follows. 
Since the GHCB page is being changed to encrypted, we need to use the MSR 
protocol by zeroing out the boot_ghcb variable. The comment should have 
said for the Page State Change and not PVALIDATE.

Thanks,
Tom

>