Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ca76ed3b-5835-9f1b-7e10-dd417249b7bd@amd.com>
Date:   Thu, 18 May 2023 15:16:18 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2 17/20] x86: efistub: Check SEV/SNP support while
 running in the firmware
Content-Language: en-US
To:     Ard Biesheuvel <ardb@kernel.org>, linux-efi@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Evgeniy Baskov <baskov@ispras.ru>,
        Borislav Petkov <bp@alien8.de>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        Peter Jones <pjones@redhat.com>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Dave Young <dyoung@redhat.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Kees Cook <keescook@chromium.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <20230508070330.582131-1-ardb@kernel.org>
 <20230508070330.582131-18-ardb@kernel.org>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20230508070330.582131-18-ardb@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SitxcXRSeTJLSTZGV1ZTMGtKUEJHa1FnUDFFbDBWNFh0YUdKVzljaGpGMitJ?=
 =?utf-8?B?U0pPYnhtemNmWmNwVXVvYVhKTm1Vd3gxTmFSOFRuamlNZi91TUlGOUs3dm5P?=
 =?utf-8?B?aVB1ZUxaMHJvTEY5aEl4UjRNNUZHZFdGc01SaWtuejA2RXMvRmZPMFpnSDho?=
 =?utf-8?B?eHp3TG1Qdkw0cVVMS0haMmRjMG5LOUpVRzU4MjlHR01VRk8wb3F2bjF0NERy?=
 =?utf-8?B?dG0zVEZ5ZThJdHBBbE1mUG9SZU9Dd0dsZ2IwejdFR1JsNThscURsNmlRMEh2?=
 =?utf-8?B?eU1ZazdOK3l5NTRxTmk2am9NQWZEYWxiR1NHcFVxTFZhb0FIcDlLVkRvck5Q?=
 =?utf-8?B?aXF5enhkaTVzeXh6QkQrZmpFM0RXaStDR0VOb1F1cllmV3NHRmF3eUE3bjRD?=
 =?utf-8?B?dlBZK3I3MUc3NVd4TytBVlR5S2t4VG5rVTU4MFV1R3o3T3I3M010MGlxdTFS?=
 =?utf-8?B?akpQSS9iMWxudjJvYzN3a1NiSndab3NGZnNTcGpXRHBCV2pnc29aRWhxOVhq?=
 =?utf-8?B?d0dTOWFldEdrM1dTYW5VUVdjL2dZaXJXMzZhRkpKZ0UyeFV4VXdUYUE2eHBE?=
 =?utf-8?B?Z3owQ1oxYzIxYTZlczBqUEVwMHJFQWhkbDZldG1FcDdndWRJT3l3Q0RlQ0NR?=
 =?utf-8?B?Q3RKVDdCTTFTOXlhcVZQdDAwT0x6dU9hdXhCZVVmTWpSRXVJY0RQY2Uza3lo?=
 =?utf-8?B?TjU3NXFvOWxNeWhEVUIwcW42SmRrQnRrZGpGR3lJblVkcjY4MDVQakNHd3Ar?=
 =?utf-8?B?KzJ4WUZzRUZyOXduMEpoQ1lBVjVaOFVCcUpoR2ZYRjJibkVjRDIva1VyVXpa?=
 =?utf-8?B?RWprRnZhUHhsLzhGWFFYVHByTWJ0U1NPcW1OOUFaaFA0L3RLaUliQWxmMjQ4?=
 =?utf-8?B?ckVpZzBBaG9QeEw1MUFGeTlFZStCeXMwd002dUVoRnVsaXo4UmVMbEVSRzhp?=
 =?utf-8?B?ZkhwUEtNRU5lZGZNWDN2cFZvMGVQUEw3MFUzMHZHS3puQ1IxS0pRSGpDcjZI?=
 =?utf-8?B?MVRBemJvS3kyRG5jMlFpQ0Z1aGF4M0tVeXF3ZGtrTzYxVjkxVWNXbW9JYXkv?=
 =?utf-8?B?WUNpLzdYL3ZncUlIQ3lOUnBZemRNV2hpbUVFZG1mekhrUW00dVgvMHhEYW9r?=
 =?utf-8?B?ZXg3OFVOZ0JQWCsxUFBGeGp3eHRnQ2dCR0ErWXhzTVdHL2ptSUtsbXBlWnE3?=
 =?utf-8?B?MjRlcWYxMVpqRGxkS0ZBUU45S3RhYnQ4bjY0bXM3ZDZKNW5KWm11UkI1UHps?=
 =?utf-8?B?bHErOUNFK1lWVmxqOWRXNjBMQ2RaWGNaYnlUd2dsdXJYTExRUEl2aFdHVTIy?=
 =?utf-8?B?cHNzWjJoMEpnd0ZSVFZJemgreDF1aEcwVzc4UWdJRjQxSXFnMnZNM1Y1bWhx?=
 =?utf-8?B?K1pHejQ5YktTYVhCUlA5dTE2LzZFcDJhekZkeVlOdHBXbmVudHhHK2g0NHVT?=
 =?utf-8?B?Qm90SjVWVWtwS2kwUFAvYjhTQmR4Z0lYQUk5YWF1bHhaZUx0TEJ4WEZnTlMy?=
 =?utf-8?B?WUFaeHo2VnMrNlljL0QyQXpneE9hUmZLYWZ6SUxqUEZLcmJHdWhoUnNIaXpi?=
 =?utf-8?B?NkxEZmVhQ1lwc3JqRGZQNWR1NUtuN0swcHZJcmJGbWF4bDZ5dS9qUW0rdytR?=
 =?utf-8?B?WXc2UXp4bzlsakhscWRzbnRXYldzRG5HQ0RDT0crVDdkZWxEYXNoVXJleW5S?=
 =?utf-8?B?Z0hXakJHdUZJWWkwRVk0L0hISXd6ZmJVcnRxWHlkRDY3VmMxbENncUh3bVNz?=
 =?utf-8?B?c1hIcDFkZllFSVdrRUlHUytnNXp1MlNHY0cvdEViK0dRbE42SjRYMFJhNTVn?=
 =?utf-8?B?cXRlTW15ZkFNUDRWa0VtS3Zva3ZGMERmdHE4aVlVWlZxMHRXUG1RMy8wbEpw?=
 =?utf-8?B?bm5FLzd6dmM1MVFzN2RqSmF3eDFnQm9PcTFJUzdhcDMxU1dkOFdNdmRSb3dl?=
 =?utf-8?B?d0NRUlVJRzJVVVBmOVNDMVJ2Q3R6YnpHNXlhUnp4dlJmMWIxM1JMUVhRNkxB?=
 =?utf-8?B?NFJkTjIrZUUwT3FjV0Zra3pxOVNQZndDTlB4SmVKTE04WFh1WHJWQUExOUsz?=
 =?utf-8?B?TnUyN0lQOG9KaTZGYW5VTlpKb0IxRy94b3dXakpTRm50ZVFMNXE4aDBqU25s?=
 =?utf-8?Q?jcIepMY3hfizWOpqa6oiHBMgc?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b6a33a8-4ed3-4e61-a7cf-08db57dcbed5
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 20:16:21.1317
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Wfdi341qjsYGclYPyZLtGdnzRtZleQKuh9B2ZRXx9UQVvl670vQACbjK0En9r5rZK+4F3IEnQ2J4O7KKKaZaGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB8054
X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 5/8/23 02:03, Ard Biesheuvel wrote:
> The decompressor executes in an environment with little or no access to
> a console, and without any ability to return an error back to the caller
> (the bootloader). So the only recourse we have when the SEV/SNP context
> is not quite what the kernel expects is to terminate the guest entirely.
> 
> This is a bit harsh, and also unnecessary when booting via the EFI stub,
> given that it provides all the support that SEV guests need to probe the
> underlying platform.
> 
> So let's do the SEV initialization and SNP feature check before calling
> ExitBootServices(), and simply return with an error if the SNP feature
> mask is not as expected.

My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
Turns out that sev_es_negotiate_protocol() used to be called when no #VC
exceptions were being generated before a valid GHCB was setup. Because
of that the current GHCB MSR value was not saved/restored. But now,
sev_es_negotiate_protocol() is called earlier in the boot process and
there are still messages being issued by UEFI, e.g.:

SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)

Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
and an SNP guest crashed after fixing sev_es_negotiate_protocol().

The following changes got me past everything:

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 3a5b0c9c4fcc..23450628d41c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
   */
  static u64 get_hv_features(void)
  {
-	u64 val;
+	u64 val, save;
  
  	if (ghcb_version < 2)
  		return 0;
  
+	save = sev_es_rd_ghcb_msr();
+
  	sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
  	VMGEXIT();
-
  	val = sev_es_rd_ghcb_msr();
+
+	sev_es_wr_ghcb_msr(save);
+
  	if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
  		return 0;
  
@@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)
  
  static bool sev_es_negotiate_protocol(void)
  {
-	u64 val;
+	u64 val, save;
+
+	save = sev_es_rd_ghcb_msr();
  
  	/* Do the GHCB protocol version negotiation */
  	sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
  	VMGEXIT();
  	val = sev_es_rd_ghcb_msr();
  
+	sev_es_wr_ghcb_msr(save);
+
  	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
  		return false;
  

Thanks,
Tom

> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   arch/x86/boot/compressed/sev.c          | 12 ++++++++----
>   arch/x86/include/asm/sev.h              |  4 ++++
>   drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
>   3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 014b89c890887b9a..19c40873fdd209b5 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
>    */
>   #define SNP_FEATURES_PRESENT (0)
>   
> +u64 snp_get_unsupported_features(void)
> +{
> +	if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> +		return 0;
> +	return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +}
> +
>   void snp_check_features(void)
>   {
>   	u64 unsupported;
>   
> -	if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> -		return;
> -
>   	/*
>   	 * Terminate the boot if hypervisor has enabled any feature lacking
>   	 * guest side implementation. Pass on the unsupported features mask through
>   	 * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
>   	 * as part of the guest boot failure.
>   	 */
> -	unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +	unsupported = snp_get_unsupported_features();
>   	if (unsupported) {
>   		if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
>   			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
>   		__sev_es_nmi_complete();
>   }
>   extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +extern void sev_enable(struct boot_params *bp);
>   
>   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
>   {
> @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
>   bool snp_init(struct boot_params *bp);
>   void __init __noreturn snp_abort(void);
>   int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> +u64 snp_get_unsupported_features(void);
>   #else
>   static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>   static inline void sev_es_ist_exit(void) { }
>   static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>   static inline void sev_es_nmi_complete(void) { }
>   static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline void sev_enable(struct boot_params *bp) { }
>   static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>   static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
>   static inline void setup_ghcb(void) { }
> @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>   {
>   	return -ENOTTY;
>   }
> +static inline u64 snp_get_unsupported_features(void) { return 0; }
>   #endif
>   
>   #endif
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index ce8434fce0c37982..33d11ba78f1d8c4f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -15,6 +15,7 @@
>   #include <asm/setup.h>
>   #include <asm/desc.h>
>   #include <asm/boot.h>
> +#include <asm/sev.h>
>   
>   #include "efistub.h"
>   
> @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>   			  &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
>   	p->efi->efi_memmap_size		= map->map_size;
>   
> +	/*
> +	 * Call the SEV init code while still running with the firmware's
> +	 * GDT/IDT, so #VC exceptions will be handled by EFI.
> +	 */
> +	if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> +		u64 unsupported;
> +
> +		sev_enable(p->boot_params);
> +		unsupported = snp_get_unsupported_features();
> +		if (unsupported) {
> +			efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> +				unsupported);
> +			return EFI_UNSUPPORTED;
> +		}
> +	}
> +
>   	return EFI_SUCCESS;
>   }
>