linux-kernel - Re: [PATCH v5] x86/sev: Add SEV-SNP guest feature negotiation support

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: <20230116133948.0000474b@gmail.com>
Date:   Mon, 16 Jan 2023 13:39:48 +0200
From:   Zhi Wang <zhi.wang.linux@...il.com>
To:     "Nikunj A. Dadhania" <nikunj@....com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        bp@...en8.de, mingo@...hat.com, tglx@...utronix.de,
        dave.hansen@...ux.intel.com, seanjc@...gle.com,
        pbonzini@...hat.com, thomas.lendacky@....com, michael.roth@....com,
        David Rientjes <rientjes@...gle.com>, stable@...nel.org
Subject: Re: [PATCH v5] x86/sev: Add SEV-SNP guest feature negotiation
 support

On Mon, 16 Jan 2023 13:53:56 +0530
"Nikunj A. Dadhania" <nikunj@....com> wrote:

> On 13/01/23 17:23, Zhi Wang wrote:
> > On Thu, 12 Jan 2023 14:11:39 +0530
> > Nikunj A Dadhania <nikunj@....com> wrote:
> > 
> 
> >> diff --git a/Documentation/x86/amd-memory-encryption.rst
> >> b/Documentation/x86/amd-memory-encryption.rst index
> >> a1940ebe7be5..b3adc39d7735 100644 ---
> >> a/Documentation/x86/amd-memory-encryption.rst +++
> >> b/Documentation/x86/amd-memory-encryption.rst @@ -95,3 +95,39 @@ by
> >> supplying mem_encrypt=on on the kernel command line.  However, if BIOS
> >> does not enable SME, then Linux will not be able to activate memory
> >> encryption, even if configured to do so by default or the mem_encrypt=on
> >> command line parameter is specified. +
> >> +Secure Nested Paging (SNP)
> >> +==========================
> >> +
> >> +SEV-SNP introduces new features (SEV_FEATURES[1:63]) which can be
> >> enabled +by the hypervisor for security enhancements. Some of these
> >> features need +guest side implementation to function correctly. The
> >> below table lists the +expected guest behavior with various possible
> >> scenarios of guest/hypervisor +SNP feature support.
> >> +
> 
> > "guest needs implementation" seems a little bit confusing. I suppose it 
> > means the feature is mandatory for the guest. 
> 
> That is not correct. None of these features are mandatory for the guest.
> The hypervisor can enable this feature without the knowledge of guest 
> kernel support. So there should be a mechanism in the guest to detect this
> and fail the boot if needed.
> 
> > If so, on the second row 
> > guest can boot without it. Some explanation? 
> 
> In the first and second row, HV has not enabled the feature, so the 
> guest should boot fine irrespective of "Guest needs implementation".
> 

Feel free to educate me if I understand correctly or not:

There are two kinds of features in SEV_FEATURES:

1. Features that HV can freely enable/disable and they won't distrub the guest.

HV   | Guest needs impl | Guest has impl    | Result
Y/N          N            X (not necessary)    Boot

2. Features that a guest has to be aware of and handle when HV enables them.

HV   | Guest needs impl | Guest has impl | Result
N            Y            X (Dont care)     Boot
Y            Y                  N           Fail
Y            Y                  Y           Boot

> >> +|      No         |      No       |      No       |     Boot         |
> 
> >> +|      No         |      Yes      |      No       |     Boot         |
> 
> 
> >> ++-----------------+---------------+---------------+------------------+
> >> +| Feature Enabled | Guest needs   | Guest has     | Guest boot       |
> >> +| by the HV       | implementation| implementation| behaviour        |
> >> ++=================+===============+===============+==================+>> +|      No         |      No       |      No       |     Boot         |
> >> +|                 |               |               |                  |
> >> ++-----------------+---------------+---------------+------------------+
> >> +|      No         |      Yes      |      No       |     Boot         |
> >> +|                 |               |               |                  |
> >> ++-----------------+---------------+---------------+------------------+
> >> +|      No         |      Yes      |      Yes      |     Boot         |
> >> +|                 |               |               |                  |
> >> ++-----------------+---------------+---------------+------------------+
> >> +|      Yes        |      No       |      No       | Boot with        |
> >> +|                 |               |               | feature enabled  |
> >> ++-----------------+---------------+---------------+------------------+
> >> +|      Yes        |      Yes      |      No       | Graceful boot    |
> >> +|                 |               |               | failure          |
> >> ++-----------------+---------------+---------------+------------------+
> >> +|      Yes        |      Yes      |      Yes      | Boot with        |
> >> +|                 |               |               | feature enabled  |
> >> ++-----------------+---------------+---------------+------------------+
> >> +
> >> +More details in AMD64 APM[1] Vol 2: 15.34.10 SEV_STATUS MSR
> >> +
> >> +[1] https://www.amd.com/system/files/TechDocs/40332_4.05.pdf
> > 
> > Probably update the link here as well.
> 
> Sure.
> 
> >> diff --git a/arch/x86/include/uapi/asm/svm.h
> >> b/arch/x86/include/uapi/asm/svm.h index f69c168391aa..a04fe07eb9a8 100644
> >> --- a/arch/x86/include/uapi/asm/svm.h
> >> +++ b/arch/x86/include/uapi/asm/svm.h
> >> @@ -116,6 +116,12 @@
> >>  #define SVM_VMGEXIT_AP_CREATE			1
> >>  #define SVM_VMGEXIT_AP_DESTROY			2
> >>  #define SVM_VMGEXIT_HV_FEATURES			0x8000fffd
> >> +#define SVM_VMGEXIT_TERM_REQUEST		0x8000fffe
> >> +#define SVM_VMGEXIT_TERM_REASON(reason_set, reason_code)	\
> >> +	/* SW_EXITINFO1[3:0] */					\
> >> +	(((((u64)reason_set) &  0xf)) |				\
> >                                ^
> > One extra space before 0xf should be removed.
> 
> Sure.
> 
> Thanks for the review.
> 
> Nikunj
>