linux-kernel - Re: [PATCH] x86/boot: avoid relaxable symbols with Clang

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=ypa8xE-kaDa7XtzPsBH8=Xu_pZj2rnWaeawNs=3dDkw@mail.gmail.com>
Date:   Mon, 10 Aug 2020 11:32:55 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Arvind Sankar <nivedita@...m.mit.edu>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Fangrui Song <maskray@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        e5ten.arch@...il.com, "# 3.4.x" <stable@...r.kernel.org>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Dmitry Golovin <dima@...ovin.in>,
        Marco Elver <elver@...gle.com>, Nick Terrell <terrelln@...com>,
        Daniel Kiper <daniel.kiper@...cle.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/boot: avoid relaxable symbols with Clang

On Fri, Aug 7, 2020 at 6:43 PM Arvind Sankar <nivedita@...m.mit.edu> wrote:
>
> On Fri, Aug 07, 2020 at 02:54:39PM -0700, Nick Desaulniers wrote:
> > On Fri, Aug 7, 2020 at 2:29 PM Arvind Sankar <nivedita@...m.mit.edu> wrote:
> > >
> > > On Fri, Aug 07, 2020 at 12:41:00PM -0700, Nick Desaulniers wrote:
> > > > A recent change to a default value of configuration variable
> > > > (ENABLE_X86_RELAX_RELOCATIONS OFF -> ON) in LLVM now causes Clang's
> > > > integrated assembler to emit R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX
> > > > relocations. LLD will relax instructions with these relocations based on
> > > > whether the image is being linked as position independent or not.  When
> > > > not, then LLD will relax these instructions to use absolute addressing
> > > > mode (R_RELAX_GOT_PC_NOPIC). This causes kernels built with Clang
> > > > and linked with LLD to fail to boot.
> > >
> > > It could also cause kernels compiled with gcc and linked with LLD to
> > > fail in the same way, no? The gcc/gas combination will generate the
> > > relaxed relocations from I think gas-2.26 onward. Although the only
> > > troublesome symbol in the case of gcc/gas is trampoline_32bit_src,
> > > referenced from pgtable_64.c (gcc doesn't use a GOTPC reloc for _pgtable
> > > etc).
> >
> > Thanks for taking a look, and the feedback. I appreciate it!
> >
> > $ gcc --version | head -n 1
> > gcc (Debian 9.3.0-11) 9.3.0
> > $ make -j71 clean defconfig bzImage
> > $ llvm-readelf -r arch/x86/boot/compressed/*.o | grep -e
> > R_X86_64_GOTPCRELX -e R_X86_64_REX_GOTPCRELX
> > 0000000000000114  000000120000002a R_X86_64_REX_GOTPCRELX
> > 0000000000000000 trampoline_32bit_src - 4
> > $ llvm-readelf -r arch/x86/boot/compressed/vmlinux | grep -e
> > R_X86_64_GOTPCRELX -e R_X86_64_REX_GOTPCRELX
> > $
> >
> > So it looks like yes.  I guess then we'd need to add a check for
> > CONFIG_LD_IS_LLD and CONFIG_CC_IS_GCC and binutils version is 2.26+?
> > I don't mind adding support for that combination, but I'd like to skip
> > it in this patch for the sake of backporting something small to stable
> > to get our CI green ASAP, since CONFIG_LD_IS_LLD probably doesn't
> > exist for those stable branches, which will complicate the backport of
> > such a patch.  So I'd do it in a follow up patch if we're cool with
> > that?
> >
>
> What if we did it only if we couldn't enable -pie, like the below patch?
> I think this should cover all the cases without needing LD_IS_LLD
> checks.
>
> For BFD, the only case that should change is binutils-2.26, which
> supports relaxations but not -z noreloc-overflow, and will now have
> relax-relocations disabled. It currently works (with gcc) only because
> the relaxation of
>         movq foo@...PCREL(%rip), %reg
> to
>         movq $foo, %reg
> in the non-pie case was only added in 2.27, which is also when -z
> noreloc-overflow was added, allowing -pie to be enabled. With 2.26, it
> only gets relaxed to
>         leaq foo(%rip), %reg
> which is all LLD currently does as well.

Sure, that will work, too.  If you'd like to send it along, please add my:
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
Tested-by: Nick Desaulniers <ndesaulniers@...gle.com>

>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 8abc30b27ba3..d25bb71f195a 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -60,6 +60,13 @@ else
>  KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
>         && echo "-z noreloc-overflow -pie --no-dynamic-linker")
>  endif
> +
> +# Disable relocation relaxation if not building as PIE
> +ifeq ($(filter -pie,$(KBUILD_LDFLAGS)),)
> +KBUILD_CFLAGS += $(call as-option, -Wa$(comma)-mrelax-relocations=no)
> +KBUILD_AFLAGS += $(call as-option, -Wa$(comma)-mrelax-relocations=no)
> +endif
> +
>  LDFLAGS_vmlinux := -T
>
>  hostprogs      := mkpiggy



-- 
Thanks,
~Nick Desaulniers