Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Alex Thorlton]

On Mon, Aug 01, 2016 at 02:49:57PM +0100, Matt Fleming wrote:
> On Tue, 26 Jul, at 05:38:33PM, Alex Thorlton wrote:
> > This problem has actually been in the UV code for a while, but we didn't
> > catch it until recently, because we had been relying on EFI_OLD_MEMMAP
> > to allow our systems to boot for a period of time.  We noticed the issue
> > when trying to kexec a recent community kernel, where we hit this NULL
> > pointer dereference in efi_sync_low_kernel_mappings:
> > 
> > [    0.337515] BUG: unable to handle kernel NULL pointer dereference at 0000000000000880
> > [    0.346276] IP: [<ffffffff8105df8d>] efi_sync_low_kernel_mappings+0x5d/0x1b0
> > 
> > The problem doesn't show up with EFI_OLD_MEMMAP because we skip the
> > chunk of setup_efi_state that sets the efi_loader_signature for the
> > kexec'd kernel.  When the kexec'd kernel boots, it won't set EFI_BOOT in
> > setup_arch, so we completely avoid the bug.
> > 
> > We always kexec with noefi on the command line, so this shouldn't be an
> > issue, but since we're not actually checking for efi_runtime_disabled in
> > uv_bios_init, we end up trying to do EFI runtime callbacks when we
> > shouldn't be. This patch just adds a check for efi_runtime_disabled in
> > uv_bios_init so that we don't map in uv_systab when runtime_disabled ==
> > true.
> > 
> > Signed-off-by: Alex Thorlton <athorlton@xxxxxxx>
> > Cc: Russ Anderson <rja@xxxxxxx>
> > Cc: Mike Travis <travis@xxxxxxx>
> > Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
> > Cc: Borislav Petkov <bp@xxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: x86@xxxxxxxxxx
> > ---
> >  arch/x86/platform/uv/bios_uv.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> > index 66b2166..0df8a03 100644
> > --- a/arch/x86/platform/uv/bios_uv.c
> > +++ b/arch/x86/platform/uv/bios_uv.c
> > @@ -187,7 +187,8 @@ EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
> >  void uv_bios_init(void)
> >  {
> >  	uv_systab = NULL;
> > -	if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab) {
> > +	if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) ||
> > +	    !efi.uv_systab || efi_runtime_disabled()) {
> >  		pr_crit("UV: UVsystab: missing\n");
> >  		return;
> >  	}
> 
> The fix looks fine, but I'm losing track of which kernels this patch
> should be applied to. Does it just need to be applied for v4.8 or
> earlier kernels too?

Well, we *have* to boot v4.6 and v4.7 with efi=old_map, which will avoid
our kexec problem entirely, so while the patch would apply just fine on
those kernels, and achieve the desired effect, we wouldn't really get
any benefit from it.

So, it definitely needs to go in for v4.8, but it's kind of a toss-up
for the older kernels.  I'll discuss this with the other guys around
here to see what they think, and get back to you a bit later, if that's
alright?

- Alex