Messages in this thread | | | From | Ard Biesheuvel <> | Date | Fri, 2 Feb 2024 23:47:24 +0100 | Subject | Re: [PATCH v3 1/2] x86/sev: enforce RIP-relative accesses in early SEV/SME code |
| |
On Fri, 2 Feb 2024 at 23:00, Kevin Loughlin <kevinloughlin@google.com> wrote: > > On Wed, Jan 31, 2024 at 12:20 AM Kirill A. Shutemov > <kirill.shutemov@linux.intel.com> wrote: > > > > On Tue, Jan 30, 2024 at 10:08:44PM +0000, Kevin Loughlin wrote: > > > 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)); \ > > > + rip_rel_ptr; \ > > > +}) > > > + > > > > I don't think it is the right place for the macro. The next patch uses for > > things unrelated to memory encryption. > > You're right; with the cleanup, I agree it becomes more general. We > can move it to arch/x86/include/asm/asm.h. > > > > > > @@ -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)++]; > > > > > > > This change doesn't belong to this patch. Maybe move it into the next > > patch and combine with removing fixup_pointer(). > > I'll put it in a separate commit even preceding this one, as it's > actually a bug in the existing fixup pointer logic that I noticed when > transitioning to the use of the RIP-relative macro. Specifically, > early_dynamic_pgts is a global variable just like next_early_pgt and > thus also needs to be fixed up to guarantee the correct address is > used across toolchains.
No, this is not a bug.
early_dynamic_pgts is a two dimensional array
extern pmd_t early_dynamic_pgts[EARLY_DYNAMIC_PAGE_TABLES][PTRS_PER_PMD];
and so this expression
early_dynamic_pgts[(*next_pgt_ptr)++]
produces the kernel virtual address of one of the top level elements, which itself is an array, and so this can be fixed up as usual. IOW, the [] operation does not result in pointer dereference here, only in pointer arithmetic.
| |