linux-kernel - Re: [PATCH] arm64: vdso: use $(LD) instead of $(CC) to link VDSO

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 12 Apr 2019 09:50:13 +0100
From:   Will Deacon <will.deacon@....com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Olof Johansson <olof@...om.net>, Arnd Bergmann <arnd@...db.de>,
        Kees Cook <keescook@...omium.org>,
        Catalin Marinas <catalin.marinas@....com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arm64: vdso: use $(LD) instead of $(CC) to link VDSO

Hi all,

On Fri, Apr 12, 2019 at 10:21:38AM +0900, Masahiro Yamada wrote:
> On Fri, Apr 12, 2019 at 3:20 AM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
> > On Thu, Apr 11, 2019 at 2:30 AM Masahiro Yamada
> > <yamada.masahiro@...ionext.com> wrote:
> > >
> > > We use $(LD) to link vmlinux, modules, decompressors, etc.
> > >
> > > VDSO is the only exceptional case where $(CC) is used as the linker
> > > driver, but I do not know why we need to do so. VDSO uses a special
> > > linker script, and does not link standard libraries at all.
> > >
> > > I changed the Makefile to use $(LD) rather than $(CC). I tested this,
> > > and VDSO worked for me.
> > >
> > > Users will be able to use their favorite linker (e.g. lld instead of
> > > of bfd) by passing LD= from the command line.
> > >
> > > My plan is to rewrite all VDSO Makefiles to use $(LD), then delete
> > > cc-ldoption.
> > >
> > > Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> > > ---
> > >
> > >  arch/arm64/kernel/vdso/Makefile | 13 +++----------
> > >  1 file changed, 3 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> > > index a0af6bf6c11b..744b9dbaba03 100644
> > > --- a/arch/arm64/kernel/vdso/Makefile
> > > +++ b/arch/arm64/kernel/vdso/Makefile
> > > @@ -12,17 +12,12 @@ obj-vdso := gettimeofday.o note.o sigreturn.o
> > >  targets := $(obj-vdso) vdso.so vdso.so.dbg
> > >  obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
> > >
> > > -ccflags-y := -shared -fno-common -fno-builtin
> >
> > Thanks for the patch; in general LGTM. Just some small questions:
> >
> > Looks like -shared and -fno-common are linker flags forwarded along,
> > but -fno-builtin is meant for the compiler, right?
> 
> Definitely, -shared must be passed to the linker
> to create a shared object.
> It is listed in the linker option list:
> https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Link-Options.html#Link-Options
> 
> 
> 
> I think -fno-common is a compiler flag instead of a linker one
> as far as I understand this:
> https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Code-Gen-Options.html#Code-Gen-Options
> 
> 
> -fno-builtin is a compiler flag:
> https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/C-Dialect-Options.html#C-Dialect-Options
> 
> 
> 
> > Do we want to keep
> > that in ccflags?
> 
> All source files in arch/arm64/kernel/vdso/
> are written in assembly (gettimeofday.S note.S sigreturn.S).
> 
> So, -fno-common -fno-builtin for ccflags-y
> are not used.
> 
> That's why I dropped them.
> 
> If we want to keep them just in case,
> it is fine with me too.

I think dropping this is fine for now, but be aware that there is ongoing
work to rewrite the vdso in C, in which case we may want that back. Much
of the Makefile will probably get reworked when that happens anyway, so
no need to worry.

> >  I'm not sure why it would have been added
> > intentionally; maybe Will knows?  Maybe it's ok to drop.
> 
> They have been here since the initial VDSO support
> by commit 9031fefde6f2a.
> 
> Will, do you remember why you added these flags?

Shamelessly lifted from ppc :)

> > > -ccflags-y += -nostdlib -Wl,-soname=linux-vdso.so.1 \
> > > -               $(call cc-ldoption, -Wl$(comma)--hash-style=sysv)
> > > +ldflags-y := -shared -nostdlib -soname=linux-vdso.so.1 \
> > > +               $(call ld-option, --hash-style=sysv) -n -T
> >
> > Forgive my ignorance, but the man page for ld makes it looks like the
> > -T argument requires a subsequent argument `scriptfile`.  Is that what
> > `$(real-prereqs)` was doing below? If so, is dropping it in this patch
> > intentional?
> 
> I just re-used cmd_ld in scripts/Makefile.lib
> 
> 
> It will be expanded like follows:
> 
> cmd_ld = $(LD) $(ld_flags) $(real-prereqs) -o $@
> 
>  ->
> 
>  $(LD) $(KBUILD_LDFLAGS) $(ldflags-y) $(LDFLAGS_$(@F)) $(real-prereqs) -o $@
> 
>  ->
> 
> $(LD) -shared -nostdlib -soname=linux-vdso.so.1 \
>      --hash-style=sysv -n -T \
>     $(obj)/vdso.lds $(obj-vdso) -o $(obj)/vdso.so.dbg

Yeah, this still works, but boy is it ugly to have that dangling -T and
then grabbing the linker script from extra-y! That's also my fault though,
so I think your patch is ok.

Will