diff mbox series

[v4,8/9] loongarch: avoid orphan input sections

Message ID 20220827083850.2702465-9-ardb@kernel.org
State New
Headers show
Series efi: implement generic compressed boot support | expand

Commit Message

Ard Biesheuvel Aug. 27, 2022, 8:38 a.m. UTC
Ensure that all input sections are listed explicitly in the linker
script, and issue a warning otherwise. This ensures that the binary
image matches the PE/COFF and other image metadata exactly, which is
important for things like code signing.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/loongarch/Kconfig              | 1 +
 arch/loongarch/kernel/vmlinux.lds.S | 2 ++
 2 files changed, 3 insertions(+)

Comments

Ard Biesheuvel Aug. 27, 2022, 1:27 p.m. UTC | #1
On Sat, 27 Aug 2022 at 12:16, 陈华才 <chenhuacai@loongson.cn> wrote:
>
> Hi, Ard,
>
> It seems that this patch is a normal bugfix and has no relation with this series. If so, I prefer to take into loongarch-fixes for 6.0-rc4. Thanks.
>

It does have a relation: the signing tool will complain because the
PE/COFF metadata goes out of sync, like so

  SBSIGN  arch/loongarch/boot/vmlinux.efi.signed
warning: data remaining[20754944 vs 20755016]: gaps between PE/COFF sections?

This is because of the *(.rela.dyn) section created by the linker,
which is appended after the .data section, and so the PE/COFF .data
section size does not match the file size.

*However*, you explained to me that vmlinux is not a PIC binary, and
the link should be non-PIE as well. So this looks like another
toolchain issue to me, that you may want to look into.

(The .rela.dyn section typically contains relocations applied by
ld.so, either to PIE executables, or to shared libraries. It is
probably harmless given that the loongarch kernel runs from its link
time address, but it is still odd that this section is being
generated)

Anyway, I will drop this patch from the series - I will leave it up to
you entirely how you prefer to fix this.





>
> > -----原始邮件-----
> > 发件人: "Ard Biesheuvel" <ardb@kernel.org>
> > 发送时间:2022-08-27 16:38:49 (星期六)
> > 收件人: linux-efi@vger.kernel.org
> > 抄送: linux-arm-kernel@lists.infradead.org, "Ard Biesheuvel" <ardb@kernel.org>, "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>, "Matthew Garrett" <mjg59@srcf.ucam.org>, "Peter Jones" <pjones@redhat.com>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Heinrich Schuchardt" <heinrich.schuchardt@canonical.com>, "AKASHI Takahiro" <takahiro.akashi@linaro.org>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Atish Patra" <atishp@atishpatra.org>, "Arnd Bergmann" <arnd@arndb.de>, "Huacai Chen" <chenhuacai@loongson.cn>, "Xi Ruoyao" <xry111@xry111.site>, "Lennart Poettering" <lennart@poettering.net>, "Jeremy Linton" <jeremy.linton@arm.com>
> > 主题: [PATCH v4 8/9] loongarch: avoid orphan input sections
> >
> > Ensure that all input sections are listed explicitly in the linker
> > script, and issue a warning otherwise. This ensures that the binary
> > image matches the PE/COFF and other image metadata exactly, which is
> > important for things like code signing.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/loongarch/Kconfig              | 1 +
> >  arch/loongarch/kernel/vmlinux.lds.S | 2 ++
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> > index fca106a8b8af..407502da4335 100644
> > --- a/arch/loongarch/Kconfig
> > +++ b/arch/loongarch/Kconfig
> > @@ -51,6 +51,7 @@ config LOONGARCH
> >       select ARCH_USE_CMPXCHG_LOCKREF
> >       select ARCH_USE_QUEUED_RWLOCKS
> >       select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
> > +     select ARCH_WANT_LD_ORPHAN_WARN
> >       select ARCH_WANTS_NO_INSTR
> >       select BUILDTIME_TABLE_SORT
> >       select COMMON_CLK
> > diff --git a/arch/loongarch/kernel/vmlinux.lds.S b/arch/loongarch/kernel/vmlinux.lds.S
> > index 36d042739f3c..17d33308dfba 100644
> > --- a/arch/loongarch/kernel/vmlinux.lds.S
> > +++ b/arch/loongarch/kernel/vmlinux.lds.S
> > @@ -74,6 +74,8 @@ SECTIONS
> >               EXIT_DATA
> >       }
> >
> > +     .rela.dyn : { *(.rela.dyn) *(.rela*) }
> > +
> >  #ifdef CONFIG_SMP
> >       PERCPU_SECTION(1 << CONFIG_L1_CACHE_SHIFT)
> >  #endif
> > --
> > 2.35.1
>
>
> 本邮件及其附件含有龙芯中科的商业秘密信息,仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用(包括但不限于全部或部分地泄露、复制或散发)本邮件及其附件中的信息。如果您错收本邮件,请您立即电话或邮件通知发件人并删除本邮件。
> This email and its attachments contain confidential information from Loongson Technology , which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this email in error, please notify the sender by phone or email immediately and delete it.
Xi Ruoyao Aug. 28, 2022, 3:31 a.m. UTC | #2
On Sat, 2022-08-27 at 15:27 +0200, Ard Biesheuvel wrote:
> It does have a relation: the signing tool will complain because the
> PE/COFF metadata goes out of sync, like so
> 
>   SBSIGN  arch/loongarch/boot/vmlinux.efi.signed
> warning: data remaining[20754944 vs 20755016]: gaps between PE/COFF
> sections?
> 
> This is because of the *(.rela.dyn) section created by the linker,
> which is appended after the .data section, and so the PE/COFF .data
> section size does not match the file size.
> 
> *However*, you explained to me that vmlinux is not a PIC binary, and
> the link should be non-PIE as well. So this looks like another
> toolchain issue to me, that you may want to look into.

In my builds:

With Binutils 2.39 and GCC 12, vmlinux rela.dyn contains 48 zero bytes
(or 3 R_LARCH_NONE relocations, which is defined "do thing" and is
ignored by ld, ld.so, and kernel module loader).

With Binutils trunk and GCC trunk, rela.dyn does not exist in
vmlinux.(But for using the trunks of Binutils and GCC we'll need to
either disable CONFIG_MODULES or apply [1].)

[1]:https://lore.kernel.org/loongarch/20220827175436.156464-1-xry111@xry111.site/T/
Ard Biesheuvel Aug. 28, 2022, 9:36 a.m. UTC | #3
On Sun, 28 Aug 2022 at 05:31, Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Sat, 2022-08-27 at 15:27 +0200, Ard Biesheuvel wrote:
> > It does have a relation: the signing tool will complain because the
> > PE/COFF metadata goes out of sync, like so
> >
> >   SBSIGN  arch/loongarch/boot/vmlinux.efi.signed
> > warning: data remaining[20754944 vs 20755016]: gaps between PE/COFF
> > sections?
> >
> > This is because of the *(.rela.dyn) section created by the linker,
> > which is appended after the .data section, and so the PE/COFF .data
> > section size does not match the file size.
> >
> > *However*, you explained to me that vmlinux is not a PIC binary, and
> > the link should be non-PIE as well. So this looks like another
> > toolchain issue to me, that you may want to look into.
>
> In my builds:
>
> With Binutils 2.39 and GCC 12, vmlinux rela.dyn contains 48 zero bytes
> (or 3 R_LARCH_NONE relocations, which is defined "do thing" and is
> ignored by ld, ld.so, and kernel module loader).
>

But does the ELF psABI for LoongArch describe L_ARCH_NONE as a dynamic
relocation? .rela.dyn typically only contains relocations that are
specified as being suitable for runtime relocation.

> With Binutils trunk and GCC trunk, rela.dyn does not exist in
> vmlinux.(But for using the trunks of Binutils and GCC we'll need to
> either disable CONFIG_MODULES or apply [1].)
>

If this is going to land in v6.1 as well, I suggest to drop the patch
entirely. By the time this gets released, you can document which
minimal toolchain version is required for UEFI secure boot signed
kernels.

> [1]:https://lore.kernel.org/loongarch/20220827175436.156464-1-xry111@xry111.site/T/
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
Xi Ruoyao Aug. 28, 2022, 9:44 a.m. UTC | #4
On Sun, 2022-08-28 at 11:36 +0200, Ard Biesheuvel wrote:
> But does the ELF psABI for LoongArch describe L_ARCH_NONE as a dynamic
> relocation? .rela.dyn typically only contains relocations that are
> specified as being suitable for runtime relocation.

In Binutils 2.39 the BFD linker often over-estimate the size of .rela.*.
For example if there is only 42 relocations, it may allocate the space
for 47 relocations and fill the unused space in the section with zero. 
Then 5 R_LARCH_NONE will appear.

This has already caused trouble when we reviewed LoongArch glibc port,
but at last the reviewers considered using NONE relocations as a
"padding" acceptable.  So in glibc ld.so will treat R_LARCH_NONE as a
"dynamic" relocation with no real effect.

The issue (or "bug", if we want a more serious term) seems fixed in
Binutils trunk, but I've not rebuilt the entire system with it so I'm
not sure if it's completely fixed.
Huacai Chen Aug. 29, 2022, 3:59 a.m. UTC | #5
OK, I think this is the root cause, but this patch is still worthy to go 6.0-rc4.

Huacai 


> -----原始邮件-----
> 发件人: "Xi Ruoyao" <xry111@xry111.site>
> 发送时间:2022-08-28 17:44:24 (星期日)
> 收件人: "Ard Biesheuvel" <ardb@kernel.org>
> 抄送: "陈华才" <chenhuacai@loongson.cn>, linux-efi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "James
>  E.J. Bottomley" <James.Bottomley@hansenpartnership.com>, "Matthew Garrett" <mjg59@srcf.ucam.org>, "Peter Jones" <pjones@redhat.com>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Heinrich Schuchardt" <heinrich.schuchardt@canonical.com>, "AKASHI Takahiro" <takahiro.akashi@linaro.org>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Atish
>  Patra" <atishp@atishpatra.org>, "Arnd Bergmann" <arnd@arndb.de>, "Lennart
>  Poettering" <lennart@poettering.net>, "Jeremy Linton" <jeremy.linton@arm.com>
> 主题: Re: [PATCH v4 8/9] loongarch: avoid orphan input sections
> 
> On Sun, 2022-08-28 at 11:36 +0200, Ard Biesheuvel wrote:
> > But does the ELF psABI for LoongArch describe L_ARCH_NONE as a dynamic
> > relocation? .rela.dyn typically only contains relocations that are
> > specified as being suitable for runtime relocation.
> 
> In Binutils 2.39 the BFD linker often over-estimate the size of .rela.*.
> For example if there is only 42 relocations, it may allocate the space
> for 47 relocations and fill the unused space in the section with zero. 
> Then 5 R_LARCH_NONE will appear.
> 
> This has already caused trouble when we reviewed LoongArch glibc port,
> but at last the reviewers considered using NONE relocations as a
> "padding" acceptable.  So in glibc ld.so will treat R_LARCH_NONE as a
> "dynamic" relocation with no real effect.
> 
> The issue (or "bug", if we want a more serious term) seems fixed in
> Binutils trunk, but I've not rebuilt the entire system with it so I'm
> not sure if it's completely fixed.
> 
> -- 
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University


本邮件及其附件含有龙芯中科的商业秘密信息,仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用(包括但不限于全部或部分地泄露、复制或散发)本邮件及其附件中的信息。如果您错收本邮件,请您立即电话或邮件通知发件人并删除本邮件。 
This email and its attachments contain confidential information from Loongson Technology , which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this email in error, please notify the sender by phone or email immediately and delete it.
diff mbox series

Patch

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index fca106a8b8af..407502da4335 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -51,6 +51,7 @@  config LOONGARCH
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
+	select ARCH_WANT_LD_ORPHAN_WARN
 	select ARCH_WANTS_NO_INSTR
 	select BUILDTIME_TABLE_SORT
 	select COMMON_CLK
diff --git a/arch/loongarch/kernel/vmlinux.lds.S b/arch/loongarch/kernel/vmlinux.lds.S
index 36d042739f3c..17d33308dfba 100644
--- a/arch/loongarch/kernel/vmlinux.lds.S
+++ b/arch/loongarch/kernel/vmlinux.lds.S
@@ -74,6 +74,8 @@  SECTIONS
 		EXIT_DATA
 	}
 
+	.rela.dyn : { *(.rela.dyn) *(.rela*) }
+
 #ifdef CONFIG_SMP
 	PERCPU_SECTION(1 << CONFIG_L1_CACHE_SHIFT)
 #endif