[PATCH] x86/sev: Fix position dependent variable references in startup code

[Ard Biesheuvel]

On Tue, 27 Feb 2024 at 20:06, Tom Lendacky <thomas.lendacky at amd.com> wrote:
>
> On 2/3/24 06:53, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb at kernel.org>
> >
> > The early startup code executes from a 1:1 mapping of memory, which
> > differs from the mapping that the code was linked and/or relocated to
> > run at. The latter mapping is not active yet at this point, and so
> > symbol references that rely on it will fault. Given that the core kernel
> > is built without -fPIC, symbol references are typically emitted as
> > absolute, and so any such references occuring in the early startup code
> > will therefore crash the kernel.
> >
> > While an attempt was made to work around this for the early SEV/SME
> > startup code, by forcing 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 RIP_RELATIVE_REF(), which generates a
> > RIP-relative reference to a given global variable. The macro is used
> > where necessary to force RIP-relative accesses to global variables.
> >
> > For backporting purposes, this patch makes no attempt at cleaning up
> > other occurrences of this pattern, involving either inline asm or
> > fixup_pointer(). Those will be addressed by subsequent patches.
> >
> > Link: https://lore.kernel.org/all/20240130220845.1978329-1-kevinloughlin@google.com/
> > Co-developed-by: Kevin Loughlin <kevinloughlin at google.com>
> > Signed-off-by: Kevin Loughlin <kevinloughlin at google.com>
> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>
> >
> >   out:
> > -     if (sme_me_mask) {
> > -             physical_mask &= ~sme_me_mask;
> > -             cc_vendor = CC_VENDOR_AMD;
> > -             cc_set_mask(sme_me_mask);
> > -     }
> > +     RIP_RELATIVE_REF(sme_me_mask) = me_mask;
> > +     physical_mask &= ~me_mask;
> > +     cc_vendor = CC_VENDOR_AMD;
>
> Sorry, just noticed this, but should physical_mask and cc_vendor also be
> RIP_RELATIVE_REF()?
>

Yes, they should. And as a matter of fact, this kind of proves my
point that we should have instrumentation for this.

I have no insight into the heuristics that the compilers use when
choosing between absolute and RIP-relative references, but both
physical_mask and cc_vendor happen to be accessed via a RIP-relative
reference here, even when using non-PIC mode. I suspect that the main
factor here is whether or not the address of the variable is [also]
taken, but I suppose the encoding size could be relevant as well.

I'll follow up with an additional patch for these two, but the
instrumentation will have to wait for the next cycle.