On Wed, Jan 31, 2024 at 5:42 AM Ard Biesheuvel [off-list ref] wrote:

Hi Kevin,

On Tue, 30 Jan 2024 at 23:09, Kevin Loughlin [off-list ref] wrote:

quoted

The compiler is not required to generate RIP-relative accesses for
SEV/SME global variables in early boot. While an attempt was made to
force RIP-relative addressing for certain global SEV/SME variables via
inline assembly (see snp_cpuid_get_table() for example), RIP-relative
addressing must be pervasively- enforced for SEV/SME global variables
when accessed prior to page table fixups.

__startup_64() already handles this issue for select non-SEV/SME global
variables using fixup_pointer(), which adjusts the pointer relative to
a `physaddr` argument. To avoid having to pass around this `physaddr`
argument across all functions needing to apply pointer fixups, this
patch introduces the macro GET_RIP_RELATIVE_PTR() (an abstraction of
the existing snp_cpuid_get_table()), which generates an RIP-relative
pointer to a passed variable. Similarly, PTR_TO_RIP_RELATIVE_PTR() is
introduced to fixup an existing pointer value with RIP-relative logic.

Applying these macros to early SEV/SME code (alongside Adam Dunlap's
necessary "[PATCH v2] x86/asm: Force native_apic_mem_read to use mov")
enables previously-failing boots of clang builds to succeed, while
preserving successful boot of gcc builds. Tested with and without SEV,
SEV-ES, SEV-SNP enabled in guests built via both gcc and clang.

Fixes: 95d33bfaa3e1 ("x86/sev: Register GHCB memory when SEV-SNP is active")
Fixes: ee0bfa08a345 ("x86/compressed/64: Add support for SEV-SNP CPUID table in #VC handlers")
Fixes: 1cd9c22fee3a ("x86/mm/encrypt: Move page table helpers into separate translation unit")
Fixes: c9f09539e16e ("x86/head/64: Check SEV encryption before switching to kernel page-table")
Fixes: b577f542f93c ("x86/coco: Add API to handle encryption mask")
Signed-off-by: Kevin Loughlin <redacted>
---
 arch/x86/coco/core.c               | 22 ++++++++-----
 arch/x86/include/asm/mem_encrypt.h | 32 +++++++++++++++---
 arch/x86/kernel/head64.c           | 31 ++++++++++--------
 arch/x86/kernel/head_64.S          |  4 ++-
 arch/x86/kernel/sev-shared.c       | 52 ++++++++++++++----------------
 arch/x86/kernel/sev.c              | 13 +++++---
 arch/x86/mm/mem_encrypt_identity.c | 50 ++++++++++++++--------------
 7 files changed, 122 insertions(+), 82 deletions(-)

OK, so the purpose of this patch is to have something that can be
backported before applying the changes I proposed to fix this more
comprehensively, right?

Correct.

I think that makes sense, although I'd like to understand how far this
would need to be backported, and for which purpose.

It would need to be backported to the first SEV-SNP support merged
upstream, which I believe was in 5.19. The rationale for the backport
is to provide an upstream fix for clang builds of SEV-SNP guests [0].

[0] https://lore.kernel.org/lkml/CAJ5mJ6j-Vw2P=QLK-J_J79S35UggvZPtm5sia74=enR1qq9X9A@mail.gmail.com/ (local)

quoted

diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index eeec9986570e..8c45b5643f48 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c

@@ -5,6 +5,11 @@
  * Copyright (C) 2021 Advanced Micro Devices, Inc.
  *
  * Author: Tom Lendacky <thomas.lendacky@amd.com>
+ *
+ * WARNING!!
+ * Select functions in this file can execute prior to page table fixups and thus
+ * require pointer fixups for global variable accesses. See WARNING in
+ * arch/x86/kernel/head64.c.
  */

 #include <linux/export.h>

@@ -61,33 +66,34 @@ static __maybe_unused __always_inline bool amd_cc_platform_vtom(enum cc_attr att
 static bool noinstr amd_cc_platform_has(enum cc_attr attr)
 {
 #ifdef CONFIG_AMD_MEM_ENCRYPT
+       const u64 sev_status_fixed_up = sev_get_status_fixup();

-       if (sev_status & MSR_AMD64_SNP_VTOM)
+       if (sev_status_fixed_up & MSR_AMD64_SNP_VTOM)
                return amd_cc_platform_vtom(attr);

        switch (attr) {
        case CC_ATTR_MEM_ENCRYPT:
-               return sme_me_mask;
+               return sme_get_me_mask_fixup();

        case CC_ATTR_HOST_MEM_ENCRYPT:
-               return sme_me_mask && !(sev_status & MSR_AMD64_SEV_ENABLED);
+               return sme_get_me_mask_fixup() && !(sev_status_fixed_up & MSR_AMD64_SEV_ENABLED);

        case CC_ATTR_GUEST_MEM_ENCRYPT:
-               return sev_status & MSR_AMD64_SEV_ENABLED;
+               return sev_status_fixed_up & MSR_AMD64_SEV_ENABLED;

        case CC_ATTR_GUEST_STATE_ENCRYPT:
-               return sev_status & MSR_AMD64_SEV_ES_ENABLED;
+               return sev_status_fixed_up & MSR_AMD64_SEV_ES_ENABLED;

        /*
         * With SEV, the rep string I/O instructions need to be unrolled
         * but SEV-ES supports them through the #VC handler.
         */
        case CC_ATTR_GUEST_UNROLL_STRING_IO:
-               return (sev_status & MSR_AMD64_SEV_ENABLED) &&
-                       !(sev_status & MSR_AMD64_SEV_ES_ENABLED);
+               return (sev_status_fixed_up & MSR_AMD64_SEV_ENABLED) &&
+                       !(sev_status_fixed_up & MSR_AMD64_SEV_ES_ENABLED);

        case CC_ATTR_GUEST_SEV_SNP:
-               return sev_status & MSR_AMD64_SEV_SNP_ENABLED;
+               return sev_status_fixed_up & MSR_AMD64_SEV_SNP_ENABLED;

        default:
                return false;

Is this code actually called early enough to matter here?

I think you're right that we don't need this; it looks like
cc_platform_has() is avoided early. An example of such avoidance is in
sme_encrypt_kernel() in arch/x86/mm/mem_encrypt_identity.c.

quoted

diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index 359ada486fa9..b65e66ee79c4 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h

@@ -17,6 +17,20 @@

 #include <asm/bootparam.h>

+/*
+ * Like the address operator "&", evaluates to the address of a LHS variable
+ * "var", but also enforces the use of RIP-relative logic. This macro can be
+ * used to safely access global data variables prior to kernel relocation.
+ */
+#define RIP_RELATIVE_ADDR(var)         \
+({                                     \
+       void *rip_rel_ptr;              \
+       asm ("lea "#var"(%%rip), %0"    \
+               : "=r" (rip_rel_ptr)    \
+               : "p" (&var));          \

I'd prefer to make this

asm ("lea %c1(%%rip), %0" : "=r" (rip_rel_ptr) : "i" (&var));

the difference being that the compiler is forced to double check that
#var and &var actually refer to the same global variable.

That also means we can make it static inline.

static inline __attribute_const__ rip_relative_ptr(const void *var)
{
    void *rip_rel_ptr;

    asm ("lea %c1(%%rip), %0" : "=r" (rip_rel_ptr) : "i" (&var));
    return rip_rel_ptr;
}

#define RIP_RELATIVE_ADDR(var) rip_relative_ptr(&var)

Good idea, works for me.

quoted

 #ifdef CONFIG_X86_MEM_ENCRYPT
 void __init mem_encrypt_init(void);
 void __init mem_encrypt_setup_arch(void);

@@ -58,6 +72,16 @@ void __init mem_encrypt_free_decrypted_mem(void);

 void __init sev_es_init_vc_handling(void);

+static __always_inline u64 sme_get_me_mask_fixup(void)

Just call this sme_get_me_mask(void) as before, and keep the existing
users. The RIP-relative reference will always work correctly so no
need to avoid it later.

Agreed. In general, I will rework to minimize the changes for
backport-friendliness.

quoted

+{
+       return *((u64 *) RIP_RELATIVE_ADDR(sme_me_mask));

Can we move the cast into the macro?

#define RIP_RELATIVE_REF(var) (*(typeof(&var))rip_relative_ptr(&var))

and make this

    return RIP_RELATIVE_REF(sme_me_mask);

Yes. I will double check that there are no instances where we actually
want the pointer instead of the dereferenced value, but I believe this
always works.

quoted

+}
+
+static __always_inline u64 sev_get_status_fixup(void)

Can we drop the _fixup suffix here? Or if we need to convey the fact
that this is a special accessor that can be used early, use _early
instead.

I'll just drop the suffix. I prefer not to use "early" in order to
avoid conflating the meaning with "runs before page table fixups",
which I don't think is true for all existing functions using the early
suffix.

quoted

+{
+       return *((u64 *) RIP_RELATIVE_ADDR(sev_status));
+}
+
 #define __bss_decrypted __section(".bss..decrypted")

 #else  /* !CONFIG_AMD_MEM_ENCRYPT */

@@ -89,6 +113,9 @@ early_set_mem_enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool en

 static inline void mem_encrypt_free_decrypted_mem(void) { }

+static inline u64 sme_get_me_mask_fixup(void) { return 0; }
+static inline u64 sev_get_status_fixup(void) { return 0; }
+
 #define __bss_decrypted

 #endif /* CONFIG_AMD_MEM_ENCRYPT */

@@ -106,11 +133,6 @@ void add_encrypt_protection_map(void);

 extern char __start_bss_decrypted[], __end_bss_decrypted[], __start_bss_decrypted_unused[];

-static inline u64 sme_get_me_mask(void)
-{
-       return sme_me_mask;
-}
-
 #endif /* __ASSEMBLY__ */

 #endif /* __X86_MEM_ENCRYPT_H__ */

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index dc0956067944..d159239997f4 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c

@@ -128,6 +128,7 @@ static bool __head check_la57_support(unsigned long physaddr)

 static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdval_t *pmd)
 {
+       const u64 sme_me_mask_fixed_up = sme_get_me_mask_fixup();
        unsigned long vaddr, vaddr_end;
        int i;

@@ -140,7 +141,7 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
         * there is no need to zero it after changing the memory encryption
         * attribute.
         */
-       if (sme_get_me_mask()) {
+       if (sme_me_mask_fixed_up) {
                vaddr = (unsigned long)__start_bss_decrypted;
                vaddr_end = (unsigned long)__end_bss_decrypted;

@@ -158,7 +159,7 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
                        early_snp_set_memory_shared(__pa(vaddr), __pa(vaddr), PTRS_PER_PMD);

                        i = pmd_index(vaddr);
-                       pmd[i] -= sme_get_me_mask();
+                       pmd[i] -= sme_me_mask_fixed_up;
                }
        }

@@ -166,18 +167,22 @@ static unsigned long __head sme_postprocess_startup(struct boot_params *bp, pmdv
         * Return the SME encryption mask (if SME is active) to be used as a
         * modifier for the initial pgdir entry programmed into CR3.
         */
-       return sme_get_me_mask();
+       return sme_me_mask_fixed_up;

Just use sme_get_me_mask() as before in this file.

Will do.

quoted

 }

-/* Code in __startup_64() can be relocated during execution, but the compiler
+/*
+ * WARNING!!
+ * Code in __startup_64() can be relocated during execution, but the compiler
  * doesn't have to generate PC-relative relocations when accessing globals from
  * that function. Clang actually does not generate them, which leads to
  * boot-time crashes. To work around this problem, every global pointer must
- * be adjusted using fixup_pointer().
+ * be adjusted using fixup_pointer() or RIP_RELATIVE_ADDR().
  */
 unsigned long __head __startup_64(unsigned long physaddr,
                                  struct boot_params *bp)
 {
+       const u64 sme_me_mask_fixed_up = sme_get_me_mask_fixup();
+       pmd_t **early_dynamic_pgts_ptr;
        unsigned long load_delta, *p;
        unsigned long pgtable_flags;
        pgdval_t *pgd;

@@ -206,7 +211,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
                for (;;);

        /* Include the SME encryption mask in the fixup value */
-       load_delta += sme_get_me_mask();
+       load_delta += sme_me_mask_fixed_up;

        /* Fixup the physical addresses in the page table */

@@ -239,14 +244,14 @@ unsigned long __head __startup_64(unsigned long physaddr,
         */

        next_pgt_ptr = fixup_pointer(&next_early_pgt, physaddr);
-       pud = fixup_pointer(early_dynamic_pgts[(*next_pgt_ptr)++], physaddr);
-       pmd = fixup_pointer(early_dynamic_pgts[(*next_pgt_ptr)++], physaddr);
+       early_dynamic_pgts_ptr = fixup_pointer(early_dynamic_pgts, physaddr);
+       pud = (pudval_t *) early_dynamic_pgts_ptr[(*next_pgt_ptr)++];
+       pmd = (pmdval_t *) early_dynamic_pgts_ptr[(*next_pgt_ptr)++];

Better to introduce early_dynamic_pgts_ptr in a separate patch if it
is just an optimization but doesn't actually fix anything.

Yeah, we can just drop. I mistakenly previously believed
early_dynamic_pgts also needed a fixup.

quoted

-       pgtable_flags = _KERNPG_TABLE_NOENC + sme_get_me_mask();
+       pgtable_flags = _KERNPG_TABLE_NOENC + sme_me_mask_fixed_up;

        if (la57) {
-               p4d = fixup_pointer(early_dynamic_pgts[(*next_pgt_ptr)++],
-                                   physaddr);
+               p4d = (p4dval_t *) early_dynamic_pgts_ptr[(*next_pgt_ptr)++];

                i = (physaddr >> PGDIR_SHIFT) % PTRS_PER_PGD;
                pgd[i + 0] = (pgdval_t)p4d + pgtable_flags;

@@ -269,7 +274,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
        /* Filter out unsupported __PAGE_KERNEL_* bits: */
        mask_ptr = fixup_pointer(&__supported_pte_mask, physaddr);
        pmd_entry &= *mask_ptr;
-       pmd_entry += sme_get_me_mask();
+       pmd_entry += sme_me_mask_fixed_up;
        pmd_entry +=  physaddr;

        for (i = 0; i < DIV_ROUND_UP(_end - _text, PMD_SIZE); i++) {

@@ -313,7 +318,7 @@ unsigned long __head __startup_64(unsigned long physaddr,
         * Fixup phys_base - remove the memory encryption mask to obtain
         * the true physical address.
         */
-       *fixup_long(&phys_base, physaddr) += load_delta - sme_get_me_mask();
+       *fixup_long(&phys_base, physaddr) += load_delta - sme_me_mask_fixed_up;

        return sme_postprocess_startup(bp, pmd);
 }

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index d4918d03efb4..b9e52cee6e00 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S

@@ -176,9 +176,11 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
        /*
         * Retrieve the modifier (SME encryption mask if SME is active) to be
         * added to the initial pgdir entry that will be programmed into CR3.
+        * Since we may have not completed page table fixups, use RIP-relative
+        * addressing for sme_me_mask.

This runs on the secondary path only, so this comment is inaccurate.

Good catch, thanks. I'll drop it.

quoted

         */
 #ifdef CONFIG_AMD_MEM_ENCRYPT
-       movq    sme_me_mask, %rax
+       movq    sme_me_mask(%rip), %rax
 #else
        xorq    %rax, %rax
 #endif

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 1d24ec679915..9ea6bea37e1d 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c

@@ -7,6 +7,11 @@
  * This file is not compiled stand-alone. It contains code shared
  * between the pre-decompression boot code and the running Linux kernel
  * and is included directly into both code-bases.
+ *
+ * WARNING!!
+ * Select functions in this file can execute prior to page table fixups and thus
+ * require pointer fixups for global variable accesses. See WARNING in
+ * arch/x86/kernel/head64.c.
  */

 #ifndef __BOOT_COMPRESSED

@@ -318,23 +323,6 @@ static int sev_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid
                    : __sev_cpuid_hv_msr(leaf);
 }

-/*
- * This may be called early while still running on the initial identity
- * mapping. Use RIP-relative addressing to obtain the correct address
- * while running with the initial identity mapping as well as the
- * switch-over to kernel virtual addresses later.
- */
-static const struct snp_cpuid_table *snp_cpuid_get_table(void)
-{

You could just make this return the RIP_RELATIVE_ADDR() result, right?

Yes, I'll do that to minimize changes for backport-friendliness.

quoted

-       void *ptr;
-
-       asm ("lea cpuid_table_copy(%%rip), %0"
-            : "=r" (ptr)
-            : "p" (&cpuid_table_copy));
-
-       return ptr;
-}
-
 /*
  * The SNP Firmware ABI, Revision 0.9, Section 7.1, details the use of
  * XCR0_IN and XSS_IN to encode multiple versions of 0xD subfunctions 0

@@ -357,7 +345,7 @@ static const struct snp_cpuid_table *snp_cpuid_get_table(void)
  */
 static u32 snp_cpuid_calc_xsave_size(u64 xfeatures_en, bool compacted)
 {
-       const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
+       const struct snp_cpuid_table *cpuid_table = RIP_RELATIVE_ADDR(cpuid_table_copy);
        u64 xfeatures_found = 0;
        u32 xsave_size = 0x240;
        int i;

@@ -394,7 +382,7 @@ static u32 snp_cpuid_calc_xsave_size(u64 xfeatures_en, bool compacted)
 static bool
 snp_cpuid_get_validated_func(struct cpuid_leaf *leaf)
 {
-       const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
+       const struct snp_cpuid_table *cpuid_table = RIP_RELATIVE_ADDR(cpuid_table_copy);
        int i;

        for (i = 0; i < cpuid_table->count; i++) {

@@ -530,7 +518,8 @@ static int snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
  */
 static int snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
 {
-       const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
+       const u32 *cpuid_std_range_max_ptr, *cpuid_hyp_range_max_ptr, *cpuid_ext_range_max_ptr;
+       const struct snp_cpuid_table *cpuid_table = RIP_RELATIVE_ADDR(cpuid_table_copy);

        if (!cpuid_table->count)
                return -EOPNOTSUPP;

@@ -555,10 +544,14 @@ static int snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_le
                 */
                leaf->eax = leaf->ebx = leaf->ecx = leaf->edx = 0;

+               cpuid_std_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_std_range_max);
+               cpuid_hyp_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_hyp_range_max);
+               cpuid_ext_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_ext_range_max);
+
                /* Skip post-processing for out-of-range zero leafs. */
-               if (!(leaf->fn <= cpuid_std_range_max ||
-                     (leaf->fn >= 0x40000000 && leaf->fn <= cpuid_hyp_range_max) ||
-                     (leaf->fn >= 0x80000000 && leaf->fn <= cpuid_ext_range_max)))
+               if (!(leaf->fn <= *cpuid_std_range_max_ptr ||
+                     (leaf->fn >= 0x40000000 && leaf->fn <= *cpuid_hyp_range_max_ptr) ||
+                     (leaf->fn >= 0x80000000 && leaf->fn <= *cpuid_ext_range_max_ptr)))
                        return 0;
        }

@@ -1045,6 +1038,7 @@ static struct cc_blob_sev_info *find_cc_blob_setup_data(struct boot_params *bp)
  */
 static void __init setup_cpuid_table(const struct cc_blob_sev_info *cc_info)
 {
+       u32 *cpuid_std_range_max_ptr, *cpuid_hyp_range_max_ptr, *cpuid_ext_range_max_ptr;
        const struct snp_cpuid_table *cpuid_table_fw, *cpuid_table;
        int i;

@@ -1055,19 +1049,23 @@ static void __init setup_cpuid_table(const struct cc_blob_sev_info *cc_info)
        if (!cpuid_table_fw->count || cpuid_table_fw->count > SNP_CPUID_COUNT_MAX)
                sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID);

-       cpuid_table = snp_cpuid_get_table();
+       cpuid_table = RIP_RELATIVE_ADDR(cpuid_table_copy);
        memcpy((void *)cpuid_table, cpuid_table_fw, sizeof(*cpuid_table));

+       cpuid_std_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_std_range_max);
+       cpuid_hyp_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_hyp_range_max);
+       cpuid_ext_range_max_ptr = RIP_RELATIVE_ADDR(cpuid_ext_range_max);
+

Can we cache the values here rather than the pointers?

Yes, I'll do so.