2020-06-24 01:51:16

by Kees Cook

[permalink] [raw]
Subject: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

In preparation for adding --orphan-handling=warn to more architectures,
make sure unwanted sections don't end up appearing under the .init
section prefix that libstub adds to itself during objcopy.

Signed-off-by: Kees Cook <[email protected]>
---
drivers/firmware/efi/libstub/Makefile | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 75daaf20374e..9d2d2e784bca 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -66,6 +66,9 @@ lib-$(CONFIG_X86) += x86-stub.o
CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)

+# Remove unwanted sections first.
+STUBCOPY_FLAGS-y += --remove-section=.note.gnu.property
+
#
# For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
# .bss section, so the .bss section of the EFI stub needs to be included in the
--
2.25.1


2020-06-24 03:34:53

by Fangrui Song

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On 2020-06-23, Kees Cook wrote:
>In preparation for adding --orphan-handling=warn to more architectures,
>make sure unwanted sections don't end up appearing under the .init
>section prefix that libstub adds to itself during objcopy.
>
>Signed-off-by: Kees Cook <[email protected]>
>---
> drivers/firmware/efi/libstub/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>index 75daaf20374e..9d2d2e784bca 100644
>--- a/drivers/firmware/efi/libstub/Makefile
>+++ b/drivers/firmware/efi/libstub/Makefile
>@@ -66,6 +66,9 @@ lib-$(CONFIG_X86) += x86-stub.o
> CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
>+# Remove unwanted sections first.
>+STUBCOPY_FLAGS-y += --remove-section=.note.gnu.property
>+
> #
> # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> # .bss section, so the .bss section of the EFI stub needs to be included in the

arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on

ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
else
branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
endif

This option creates .note.gnu.property:

% readelf -n drivers/firmware/efi/libstub/efi-stub.o

Displaying notes found in: .note.gnu.property
Owner Data size Description
GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
Properties: AArch64 feature: PAC

If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
-mbranch-protection=none can override -mbranch-protection=pac-ret+leaf

2020-06-24 04:44:55

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> On 2020-06-23, Kees Cook wrote:
> > In preparation for adding --orphan-handling=warn to more architectures,
> > make sure unwanted sections don't end up appearing under the .init
> > section prefix that libstub adds to itself during objcopy.
> >
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > drivers/firmware/efi/libstub/Makefile | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index 75daaf20374e..9d2d2e784bca 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86) += x86-stub.o
> > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> >
> > +# Remove unwanted sections first.
> > +STUBCOPY_FLAGS-y += --remove-section=.note.gnu.property
> > +
> > #
> > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > # .bss section, so the .bss section of the EFI stub needs to be included in the
>
> arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
>
> ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> else
> branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> endif
>
> This option creates .note.gnu.property:
>
> % readelf -n drivers/firmware/efi/libstub/efi-stub.o
>
> Displaying notes found in: .note.gnu.property
> Owner Data size Description
> GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
> Properties: AArch64 feature: PAC
>
> If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf

We want to keep the branch protection enabled. But since it's not a
"regular" ELF, we don't need to keep the property that identifies the
feature.

--
Kees Cook

2020-06-24 10:46:46

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > On 2020-06-23, Kees Cook wrote:
> > > In preparation for adding --orphan-handling=warn to more architectures,
> > > make sure unwanted sections don't end up appearing under the .init
> > > section prefix that libstub adds to itself during objcopy.
> > >
> > > Signed-off-by: Kees Cook <[email protected]>
> > > ---
> > > drivers/firmware/efi/libstub/Makefile | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > index 75daaf20374e..9d2d2e784bca 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86) += x86-stub.o
> > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > >
> > > +# Remove unwanted sections first.
> > > +STUBCOPY_FLAGS-y += --remove-section=.note.gnu.property
> > > +
> > > #
> > > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > > # .bss section, so the .bss section of the EFI stub needs to be included in the
> >
> > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> >
> > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > else
> > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > endif
> >
> > This option creates .note.gnu.property:
> >
> > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> >
> > Displaying notes found in: .note.gnu.property
> > Owner Data size Description
> > GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
> > Properties: AArch64 feature: PAC
> >
> > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
>
> We want to keep the branch protection enabled. But since it's not a
> "regular" ELF, we don't need to keep the property that identifies the
> feature.

For the kernel Image, how do we remove these sections? The objcopy flags
in arch/arm64/boot/Makefile look both insufficient and out of date. My
vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
segment.

Will

2020-06-24 10:49:49

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 12:44, Will Deacon <[email protected]> wrote:
>
> On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > On 2020-06-23, Kees Cook wrote:
> > > > In preparation for adding --orphan-handling=warn to more architectures,
> > > > make sure unwanted sections don't end up appearing under the .init
> > > > section prefix that libstub adds to itself during objcopy.
> > > >
> > > > Signed-off-by: Kees Cook <[email protected]>
> > > > ---
> > > > drivers/firmware/efi/libstub/Makefile | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > > index 75daaf20374e..9d2d2e784bca 100644
> > > > --- a/drivers/firmware/efi/libstub/Makefile
> > > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > > @@ -66,6 +66,9 @@ lib-$(CONFIG_X86) += x86-stub.o
> > > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > > CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > > >
> > > > +# Remove unwanted sections first.
> > > > +STUBCOPY_FLAGS-y += --remove-section=.note.gnu.property
> > > > +
> > > > #
> > > > # For x86, bootloaders like systemd-boot or grub-efi do not zero-initialize the
> > > > # .bss section, so the .bss section of the EFI stub needs to be included in the
> > >
> > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > >
> > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > else
> > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > endif
> > >
> > > This option creates .note.gnu.property:
> > >
> > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > >
> > > Displaying notes found in: .note.gnu.property
> > > Owner Data size Description
> > > GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
> > > Properties: AArch64 feature: PAC
> > >
> > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> >
> > We want to keep the branch protection enabled. But since it's not a
> > "regular" ELF, we don't need to keep the property that identifies the
> > feature.
>
> For the kernel Image, how do we remove these sections? The objcopy flags
> in arch/arm64/boot/Makefile look both insufficient and out of date. My
> vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> segment.
>

The latter is the fault of the libstub make rules, that prepend .init
to all section names.

I'm not sure if there is a point to having PAC and/or BTI in the EFI
stub, given that it runs under the control of the firmware, with its
memory mappings and PAC configuration etc.

2020-06-24 11:28:14

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 12:44, Will Deacon <[email protected]> wrote:
> > On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > > >
> > > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > > else
> > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > > endif
> > > >
> > > > This option creates .note.gnu.property:
> > > >
> > > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > > >
> > > > Displaying notes found in: .note.gnu.property
> > > > Owner Data size Description
> > > > GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
> > > > Properties: AArch64 feature: PAC
> > > >
> > > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> > >
> > > We want to keep the branch protection enabled. But since it's not a
> > > "regular" ELF, we don't need to keep the property that identifies the
> > > feature.
> >
> > For the kernel Image, how do we remove these sections? The objcopy flags
> > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > segment.
> >
>
> The latter is the fault of the libstub make rules, that prepend .init
> to all section names.

Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
but I still see this note in vmlinux. It looks like it comes in via the
stub copy of lib-ctype.o, but I don't know why that would force the
note. The cflags look ok to me [1] and I confirmed that the note is
being generated by the compiler.

> I'm not sure if there is a point to having PAC and/or BTI in the EFI
> stub, given that it runs under the control of the firmware, with its
> memory mappings and PAC configuration etc.

Agreed, I just can't figure out how to get rid of the note.

Will

[1] -mlittle-endian -DKASAN_SHADOW_SCALE_SHIFT=3 -Qunused-arguments -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 --target=aarch64-linux-gnu --prefix=/usr/local/google/home/willdeacon/bin/ --gcc-toolchain=/usr/local/google/home/willdeacon -no-integrated-as -Werror=unknown-warning-option -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -fno-asynchronous-unwind-tables -mbranch-protection=pac-ret+leaf+bti -Wa,-march=armv8.3-a -DKASAN_SHADOW_SCALE_SHIFT=3 -fno-delete-null-pointer-checks -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -g -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-tautological-constant-out-of-range-compare -fpie -mbranch-protection=none -I./scripts/dtc/libfdt -Os -DDISABLE_BRANCH_PROFILING -include ./drivers/firmware/efi/libstub/hidden.h -D__NO_FORTIFY -ffreestanding -fno-stack-protector -fno-addrsig -D__DISABLE_EXPORTS -DKBUILD_MODFILE='"drivers/firmware/efi/libstub/lib-ctype"' -DKBUILD_BASENAME='"lib_ctype"' -DKBUILD_MODNAME='"lib_ctype"' -c -o drivers/firmware/efi/libstub/lib-ctype.o lib/ctype.c

2020-06-24 13:50:26

by Dave Martin

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 12:44, Will Deacon <[email protected]> wrote:
> > > On Tue, Jun 23, 2020 at 09:44:11PM -0700, Kees Cook wrote:
> > > > On Tue, Jun 23, 2020 at 08:31:42PM -0700, 'Fangrui Song' via Clang Built Linux wrote:
> > > > > arch/arm64/Kconfig enables ARM64_PTR_AUTH by default. When the config is on
> > > > >
> > > > > ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > > > > else
> > > > > branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > > > > endif
> > > > >
> > > > > This option creates .note.gnu.property:
> > > > >
> > > > > % readelf -n drivers/firmware/efi/libstub/efi-stub.o
> > > > >
> > > > > Displaying notes found in: .note.gnu.property
> > > > > Owner Data size Description
> > > > > GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
> > > > > Properties: AArch64 feature: PAC
> > > > >
> > > > > If .note.gnu.property is not desired in drivers/firmware/efi/libstub, specifying
> > > > > -mbranch-protection=none can override -mbranch-protection=pac-ret+leaf
> > > >
> > > > We want to keep the branch protection enabled. But since it's not a
> > > > "regular" ELF, we don't need to keep the property that identifies the
> > > > feature.
> > >
> > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > segment.
> > >
> >
> > The latter is the fault of the libstub make rules, that prepend .init
> > to all section names.
>
> Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> but I still see this note in vmlinux. It looks like it comes in via the
> stub copy of lib-ctype.o, but I don't know why that would force the
> note. The cflags look ok to me [1] and I confirmed that the note is
> being generated by the compiler.
>
> > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > stub, given that it runs under the control of the firmware, with its
> > memory mappings and PAC configuration etc.
>
> Agreed, I just can't figure out how to get rid of the note.

Because this section is generated by the linker itself I think you might
have to send it to /DISCARD/ in the link, or strip it explicitly after
linking.

Cheers
---Dave

2020-06-24 15:24:45

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> I'm not sure if there is a point to having PAC and/or BTI in the EFI
> stub, given that it runs under the control of the firmware, with its
> memory mappings and PAC configuration etc.

Is BTI being ignored when the firmware runs?

--
Kees Cook

2020-06-24 15:29:30

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 02:48:55PM +0100, Dave Martin wrote:
> On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 12:44, Will Deacon <[email protected]> wrote:
> > > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > > segment.
> > >
> > > The latter is the fault of the libstub make rules, that prepend .init
> > > to all section names.
> >
> > Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> > but I still see this note in vmlinux. It looks like it comes in via the
> > stub copy of lib-ctype.o, but I don't know why that would force the
> > note. The cflags look ok to me [1] and I confirmed that the note is
> > being generated by the compiler.
> >
> > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > stub, given that it runs under the control of the firmware, with its
> > > memory mappings and PAC configuration etc.
> >
> > Agreed, I just can't figure out how to get rid of the note.
>
> Because this section is generated by the linker itself I think you might
> have to send it to /DISCARD/ in the link, or strip it explicitly after
> linking.

Right, but why is the linker generating that section in the first place? I'm
compiling with -mbranch-protection=none and all the other objects linked
into the stub do not have the section.

I wonder if it's because lib/ctype.c doesn't have any executable code...

Will

2020-06-24 15:32:50

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > stub, given that it runs under the control of the firmware, with its
> > memory mappings and PAC configuration etc.
>
> Is BTI being ignored when the firmware runs?
>

Given that it requires the 'guarded' attribute to be set in the page
tables, and the fact that the UEFI spec does not require it for
executables that it invokes, nor describes any means of annotating
such executables as having been built with BTI annotations, I think we
can safely assume that the EFI stub will execute with BTI disabled in
the foreseeable future.

2020-06-24 15:47:24

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> >
> > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > stub, given that it runs under the control of the firmware, with its
> > > memory mappings and PAC configuration etc.
> >
> > Is BTI being ignored when the firmware runs?
>
> Given that it requires the 'guarded' attribute to be set in the page
> tables, and the fact that the UEFI spec does not require it for
> executables that it invokes, nor describes any means of annotating
> such executables as having been built with BTI annotations, I think we
> can safely assume that the EFI stub will execute with BTI disabled in
> the foreseeable future.

yaaaaaay. *sigh* How long until EFI catches up?

That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
it, we'll be ready?

--
Kees Cook

2020-06-24 15:49:37

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > stub, given that it runs under the control of the firmware, with its
> > > > memory mappings and PAC configuration etc.
> > >
> > > Is BTI being ignored when the firmware runs?
> >
> > Given that it requires the 'guarded' attribute to be set in the page
> > tables, and the fact that the UEFI spec does not require it for
> > executables that it invokes, nor describes any means of annotating
> > such executables as having been built with BTI annotations, I think we
> > can safely assume that the EFI stub will execute with BTI disabled in
> > the foreseeable future.
>
> yaaaaaay. *sigh* How long until EFI catches up?
>
> That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> it, we'll be ready?
>

Sure. Although I anticipate that we'll need to set some flag in the
PE/COFF header to enable it, and so any BTI opcodes we emit without
that will never take effect in practice.

2020-06-24 16:30:02

by Dave Martin

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 04:26:46PM +0100, Will Deacon wrote:
> On Wed, Jun 24, 2020 at 02:48:55PM +0100, Dave Martin wrote:
> > On Wed, Jun 24, 2020 at 12:26:47PM +0100, Will Deacon wrote:
> > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 12:44, Will Deacon <[email protected]> wrote:
> > > > > For the kernel Image, how do we remove these sections? The objcopy flags
> > > > > in arch/arm64/boot/Makefile look both insufficient and out of date. My
> > > > > vmlinux ends up with both a ".notes" and a ".init.note.gnu.property"
> > > > > segment.
> > > >
> > > > The latter is the fault of the libstub make rules, that prepend .init
> > > > to all section names.
> > >
> > > Hmm. I tried adding -mbranch-protection=none to arm64 cflags for the stub,
> > > but I still see this note in vmlinux. It looks like it comes in via the
> > > stub copy of lib-ctype.o, but I don't know why that would force the
> > > note. The cflags look ok to me [1] and I confirmed that the note is
> > > being generated by the compiler.
> > >
> > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > stub, given that it runs under the control of the firmware, with its
> > > > memory mappings and PAC configuration etc.
> > >
> > > Agreed, I just can't figure out how to get rid of the note.
> >
> > Because this section is generated by the linker itself I think you might
> > have to send it to /DISCARD/ in the link, or strip it explicitly after
> > linking.
>
> Right, but why is the linker generating that section in the first place? I'm
> compiling with -mbranch-protection=none and all the other objects linked
> into the stub do not have the section.
>
> I wonder if it's because lib/ctype.c doesn't have any executable code...

What compiler and flags are you using for the affected object? I don't
see this with gcc so far.

I wonder if this is a hole in the specs: the property could logically
be emitted in any codeless object, since turning on BTI will obviously
not break that object.

For different linkers and compilers to interoperate though, the specs
would need to say what to do in that situation.

Cheers
---Dave



>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2020-06-24 16:34:32

by Dave Martin

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
> >
> > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > stub, given that it runs under the control of the firmware, with its
> > > > > memory mappings and PAC configuration etc.
> > > >
> > > > Is BTI being ignored when the firmware runs?
> > >
> > > Given that it requires the 'guarded' attribute to be set in the page
> > > tables, and the fact that the UEFI spec does not require it for
> > > executables that it invokes, nor describes any means of annotating
> > > such executables as having been built with BTI annotations, I think we
> > > can safely assume that the EFI stub will execute with BTI disabled in
> > > the foreseeable future.
> >
> > yaaaaaay. *sigh* How long until EFI catches up?
> >
> > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > it, we'll be ready?
> >
>
> Sure. Although I anticipate that we'll need to set some flag in the
> PE/COFF header to enable it, and so any BTI opcodes we emit without
> that will never take effect in practice.

In the meantime, it is possible to build all the in-tree parts of EFI
for BTI, and just turn it off for out-of-tree EFI binaries?

If there's no easy way to do this though, I guess we should wait for /
push for a PE/COFF flag to describe this properly.

Cheers
---Dave

2020-06-24 16:42:43

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 18:29, Dave Martin <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > memory mappings and PAC configuration etc.
> > > > >
> > > > > Is BTI being ignored when the firmware runs?
> > > >
> > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > tables, and the fact that the UEFI spec does not require it for
> > > > executables that it invokes, nor describes any means of annotating
> > > > such executables as having been built with BTI annotations, I think we
> > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > the foreseeable future.
> > >
> > > yaaaaaay. *sigh* How long until EFI catches up?
> > >
> > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > it, we'll be ready?
> > >
> >
> > Sure. Although I anticipate that we'll need to set some flag in the
> > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > that will never take effect in practice.
>
> In the meantime, it is possible to build all the in-tree parts of EFI
> for BTI, and just turn it off for out-of-tree EFI binaries?
>

Not sure I understand the question. What do you mean by out-of-tree
EFI binaries? And how would the firmware (which is out of tree itself,
and is in charge of the page tables, vector table, timer interrupt etc
when the EFI stub executes) distinguish such binaries from the EFI
stub?


> If there's no easy way to do this though, I guess we should wait for /
> push for a PE/COFF flag to describe this properly.
>

Yeah good point. I will take this to the forum.

2020-06-24 17:17:36

by Dave Martin

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> On Wed, 24 Jun 2020 at 18:29, Dave Martin <[email protected]> wrote:
> >
> > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > memory mappings and PAC configuration etc.
> > > > > >
> > > > > > Is BTI being ignored when the firmware runs?
> > > > >
> > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > executables that it invokes, nor describes any means of annotating
> > > > > such executables as having been built with BTI annotations, I think we
> > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > the foreseeable future.
> > > >
> > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > >
> > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > it, we'll be ready?
> > > >
> > >
> > > Sure. Although I anticipate that we'll need to set some flag in the
> > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > that will never take effect in practice.
> >
> > In the meantime, it is possible to build all the in-tree parts of EFI
> > for BTI, and just turn it off for out-of-tree EFI binaries?
> >
>
> Not sure I understand the question. What do you mean by out-of-tree
> EFI binaries? And how would the firmware (which is out of tree itself,
> and is in charge of the page tables, vector table, timer interrupt etc
> when the EFI stub executes) distinguish such binaries from the EFI
> stub?

I'm not an EFI expert, but I'm guessing that you configure EFI with
certain compiler flags and build it. Possibly some standalone EFI
executables are built out of the same tree and shipped with the
firmware from the same build, but I'm speculating. If not, we can just
run all EFI executables with BTI off.

> > If there's no easy way to do this though, I guess we should wait for /
> > push for a PE/COFF flag to describe this properly.
> >
>
> Yeah good point. I will take this to the forum.

In the interim, we could set the GP bit in EFI's page tables for the
executable code from the firmware image if we want this protection, but
turn it off in pages mapping the executable code of EFI executables.
This is better than nothing.

Cheers
---Dave

2020-06-24 18:24:53

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 19:16, Dave Martin <[email protected]> wrote:
>
> On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> > On Wed, 24 Jun 2020 at 18:29, Dave Martin <[email protected]> wrote:
> > >
> > > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > > > > > >
> > > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > > memory mappings and PAC configuration etc.
> > > > > > >
> > > > > > > Is BTI being ignored when the firmware runs?
> > > > > >
> > > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > > executables that it invokes, nor describes any means of annotating
> > > > > > such executables as having been built with BTI annotations, I think we
> > > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > > the foreseeable future.
> > > > >
> > > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > > >
> > > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > > it, we'll be ready?
> > > > >
> > > >
> > > > Sure. Although I anticipate that we'll need to set some flag in the
> > > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > > that will never take effect in practice.
> > >
> > > In the meantime, it is possible to build all the in-tree parts of EFI
> > > for BTI, and just turn it off for out-of-tree EFI binaries?
> > >
> >
> > Not sure I understand the question. What do you mean by out-of-tree
> > EFI binaries? And how would the firmware (which is out of tree itself,
> > and is in charge of the page tables, vector table, timer interrupt etc
> > when the EFI stub executes) distinguish such binaries from the EFI
> > stub?
>
> I'm not an EFI expert, but I'm guessing that you configure EFI with
> certain compiler flags and build it.

'EFI' is not something you build. It is a specification that describes
how a conformant firmware implementation interfaces with a conformant
OS.

Sorry to be pedantic, but that is really quite relevant. By adhering
to the EFI spec rigorously, we no longer have to care about who
implements the opposite side, and how.

So yes, of course there are ways to build the opposite side with BTI
enabled, in a way that all its constituent pieces keep working as
expected. A typical EDK2 based implementation of EFI consists of
50-100 individual PE/COFF executables that all get loaded, relocated
and started like ordinary user space programs.

What we cannot do, though, is invent our own Linux specific way of
decorating the kernel's PE/COFF header with an annotation that
instructs a Linux specific EFI loader when to enable the GP bit for
the .text pages.

> Possibly some standalone EFI
> executables are built out of the same tree and shipped with the
> firmware from the same build, but I'm speculating. If not, we can just
> run all EFI executables with BTI off.
>
> > > If there's no easy way to do this though, I guess we should wait for /
> > > push for a PE/COFF flag to describe this properly.
> > >
> >
> > Yeah good point. I will take this to the forum.
>
> In the interim, we could set the GP bit in EFI's page tables for the
> executable code from the firmware image if we want this protection, but
> turn it off in pages mapping the executable code of EFI executables.
> This is better than nothing.
>

We need to distinguish between the EFI stub and the EFI runtime services here.

The EFI stub consists of kernel code that executes in the context of
the firmware, at which point the loader has no control whatsoever over
page tables, vector tables, etc. This is the stage where the loading
and starting of PE/COFF images takes place. If we want to enable BTI
for code running in this context, we need PE/COFF annotations, as
discussed above.

The EFI runtime services are firmware code that gets invoked by the OS
at runtime. Whether or not such code is emitted with BTI annotations
is a separate matter (but should also be taken to the forum
nonetheless), and does not need any changes at the PE/COFF level.
However, for this code, I'd like the sandboxing to be much more
rigorous than it is today, to the point where the security it provides
doesn't even matter deeply to the OS itself. (I had some patches a
while ago that reused the KPTI infrastructure to unmap the entire
kernel while EFI runtime services are in progress. There was also an
intern in the team that implemented something similar on top of KVM)

2020-06-24 18:59:01

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] efi/libstub: Remove .note.gnu.property

On Wed, 24 Jun 2020 at 20:23, Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 24 Jun 2020 at 19:16, Dave Martin <[email protected]> wrote:
> >
> > On Wed, Jun 24, 2020 at 06:40:48PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 24 Jun 2020 at 18:29, Dave Martin <[email protected]> wrote:
> > > >
> > > > On Wed, Jun 24, 2020 at 05:48:41PM +0200, Ard Biesheuvel wrote:
> > > > > On Wed, 24 Jun 2020 at 17:45, Kees Cook <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jun 24, 2020 at 05:31:06PM +0200, Ard Biesheuvel wrote:
> > > > > > > On Wed, 24 Jun 2020 at 17:21, Kees Cook <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jun 24, 2020 at 12:46:32PM +0200, Ard Biesheuvel wrote:
> > > > > > > > > I'm not sure if there is a point to having PAC and/or BTI in the EFI
> > > > > > > > > stub, given that it runs under the control of the firmware, with its
> > > > > > > > > memory mappings and PAC configuration etc.
> > > > > > > >
> > > > > > > > Is BTI being ignored when the firmware runs?
> > > > > > >
> > > > > > > Given that it requires the 'guarded' attribute to be set in the page
> > > > > > > tables, and the fact that the UEFI spec does not require it for
> > > > > > > executables that it invokes, nor describes any means of annotating
> > > > > > > such executables as having been built with BTI annotations, I think we
> > > > > > > can safely assume that the EFI stub will execute with BTI disabled in
> > > > > > > the foreseeable future.
> > > > > >
> > > > > > yaaaaaay. *sigh* How long until EFI catches up?
> > > > > >
> > > > > > That said, BTI shouldn't _hurt_, right? If EFI ever decides to enable
> > > > > > it, we'll be ready?
> > > > > >
> > > > >
> > > > > Sure. Although I anticipate that we'll need to set some flag in the
> > > > > PE/COFF header to enable it, and so any BTI opcodes we emit without
> > > > > that will never take effect in practice.
> > > >
> > > > In the meantime, it is possible to build all the in-tree parts of EFI
> > > > for BTI, and just turn it off for out-of-tree EFI binaries?
> > > >
> > >
> > > Not sure I understand the question. What do you mean by out-of-tree
> > > EFI binaries? And how would the firmware (which is out of tree itself,
> > > and is in charge of the page tables, vector table, timer interrupt etc
> > > when the EFI stub executes) distinguish such binaries from the EFI
> > > stub?
> >
> > I'm not an EFI expert, but I'm guessing that you configure EFI with
> > certain compiler flags and build it.
>
> 'EFI' is not something you build. It is a specification that describes
> how a conformant firmware implementation interfaces with a conformant
> OS.
>
> Sorry to be pedantic, but that is really quite relevant. By adhering
> to the EFI spec rigorously, we no longer have to care about who
> implements the opposite side, and how.
>
> So yes, of course there are ways to build the opposite side with BTI
> enabled, in a way that all its constituent pieces keep working as
> expected. A typical EDK2 based implementation of EFI consists of
> 50-100 individual PE/COFF executables that all get loaded, relocated
> and started like ordinary user space programs.
>
> What we cannot do, though, is invent our own Linux specific way of
> decorating the kernel's PE/COFF header with an annotation that
> instructs a Linux specific EFI loader when to enable the GP bit for
> the .text pages.
>
> > Possibly some standalone EFI
> > executables are built out of the same tree and shipped with the
> > firmware from the same build, but I'm speculating. If not, we can just
> > run all EFI executables with BTI off.
> >
> > > > If there's no easy way to do this though, I guess we should wait for /
> > > > push for a PE/COFF flag to describe this properly.
> > > >
> > >
> > > Yeah good point. I will take this to the forum.
> >
> > In the interim, we could set the GP bit in EFI's page tables for the
> > executable code from the firmware image if we want this protection, but
> > turn it off in pages mapping the executable code of EFI executables.
> > This is better than nothing.
> >
>
> We need to distinguish between the EFI stub and the EFI runtime services here.
>
> The EFI stub consists of kernel code that executes in the context of
> the firmware, at which point the loader has no control whatsoever over
> page tables, vector tables, etc. This is the stage where the loading
> and starting of PE/COFF images takes place. If we want to enable BTI
> for code running in this context, we need PE/COFF annotations, as
> discussed above.
>
> The EFI runtime services are firmware code that gets invoked by the OS
> at runtime. Whether or not such code is emitted with BTI annotations
> is a separate matter (but should also be taken to the forum
> nonetheless), and does not need any changes at the PE/COFF level.
> However, for this code, I'd like the sandboxing to be much more
> rigorous than it is today, to the point where the security it provides

... the security *bti* provides ...

> doesn't even matter deeply to the OS itself. (I had some patches a
> while ago that reused the KPTI infrastructure to unmap the entire
> kernel while EFI runtime services are in progress. There was also an
> intern in the team that implemented something similar on top of KVM)