linux-kernel - Re: [PATCH] arm64: Disable LLD linker ASSERT()s for the time being

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: <20250602114348.GA1227@willie-the-truck>
Date: Mon, 2 Jun 2025 12:43:49 +0100
From: Will Deacon <will@...nel.org>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Arnd Bergmann <arnd@...db.de>, Ard Biesheuvel <ardb+git@...gle.com>,
	linux-arm-kernel@...ts.infradead.org, llvm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, catalin.marinas@....com,
	nathan@...nel.org
Subject: Re: [PATCH] arm64: Disable LLD linker ASSERT()s for the time being

On Mon, Jun 02, 2025 at 12:18:33PM +0200, Ard Biesheuvel wrote:
> On Mon, 2 Jun 2025 at 12:09, Will Deacon <will@...nel.org> wrote:
> >
> > On Fri, May 30, 2025 at 04:23:16PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 May 2025 at 15:38, Will Deacon <will@...nel.org> wrote:
> > > >
> > > > On Thu, May 29, 2025 at 09:35:08AM +0200, Ard Biesheuvel wrote:
> > > > > From: Ard Biesheuvel <ardb@...nel.org>
> > > > >
> > > > > It turns out that the way LLD handles ASSERT()s in the linker script can
> > > > > result in spurious failures, so disable them for the newly introduced
> > > > > BSS symbol export checks.
> > > > >
> > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/2094
> > > > > Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> > > > > ---
> > > > >  arch/arm64/kernel/image-vars.h | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> > > > > index c5266430284b..86f088a16147 100644
> > > > > --- a/arch/arm64/kernel/image-vars.h
> > > > > +++ b/arch/arm64/kernel/image-vars.h
> > > > > @@ -10,6 +10,10 @@
> > > > >  #error This file should only be included in vmlinux.lds.S
> > > > >  #endif
> > > > >
> > > > > +#if defined(CONFIG_LD_IS_LLD) && CONFIG_LLD_VERSION < 210000
> > > > > +#define ASSERT(...)
> > > > > +#endif
> > > > > +
> > > > >  #define PI_EXPORT_SYM(sym)           \
> > > > >       __PI_EXPORT_SYM(sym, __pi_ ## sym, Cannot export BSS symbol sym to startup code)
> > > > >  #define __PI_EXPORT_SYM(sym, pisym, msg)\
> > > > > @@ -142,4 +146,6 @@ KVM_NVHE_ALIAS(kvm_protected_mode_initialized);
> > > > >  _kernel_codesize = ABSOLUTE(__inittext_end - _text);
> > > > >  #endif
> > > > >
> > > > > +#undef ASSERT
> > > >
> > > > What about the ASSERT()s at the end of vmlinux.lds.S? Are they not
> > > > affected by the bug, for some reason?
> > > >
> > > > Also, even with this patch applied, I still see a link failure:
> > > >
> > > >   | ld.lld: error: assignment to symbol __init_end does not converge
> > > >
> > > > with the .config you sent me off-list.
> > > >
> > >
> > > That is a different error that has been lurking for a while now; Arnd
> > > occasionally hits it but I haven't seen any other reports of it. AIUI,
> > > the issue is that INIT_IDMAP_DIR_PAGES and INIT_DIR_SIZE are defined
> > > in terms of (_end - KIMAGE_VADDR), resulting in a circular dependency.
> >
> > Ok, I'll ignore that one for the moment, then...
> >
> > > The config in the kernel test robot's report [0] appears to build fine
> > > with this patch applied.
> > >
> > >
> > > [0] https://lore.kernel.org/all/202505261019.OUlitN6m-lkp@intel.com/T/#u
> >
> > ... but I'm still not sure why the ASSERT()s in vmlinux.lds.S are not
> > affected. Is it just that we've not hit a .config which breaks with
> > those yet, or is it something more fundamental than that?
> 
> The former, as far as I can tell. The BSS patch just adds a fair
> amount of ASSERT()s so the attack surface has become larger. And
> perhaps those checks are more susceptible due to the fact that they
> compare symbols living in different sections? But that is just
> conjecture.
> 
> > I'd have
> > thought we'd need to so something like below (on top of your patch) to
> > fix this issue properly.
> >
> 
> Yes, it is the more thorough fix, but we'd lose coverage for those
> ASSERT()s which are arguably more important than the ones I added.

Alright, thanks. Let's go with what you have and I'll try to stick some
of this rationale in the commit message.

Will