[PATCH v5 1/2] x86/purgatory: Disable various profiling and sanitizing options

[PATCH v5 1/2] x86/purgatory: Disable various profiling and sanitizing options

[Hans de Goede]

Since the purgatory is a special stand-alone binary, we need to disable
various profiling and sanitizing options. Having these options enabled
typically will cause dependency on various special symbols exported by
special libs / stubs used by these frameworks. Since the purgatory is
special we do not link against these stubs causing missing symbols in
the purgatory if we do not disable these options.

This commit syncs the set of disabled profiling and sanitizing options
with that from drivers/firmware/efi/libstub/Makefile, adding
-DDISABLE_BRANCH_PROFILING to the CFLAGS and setting:

GCOV_PROFILE                    := n
UBSAN_SANITIZE                  := n

This fixes broken references to ftrace_likely_update when
CONFIG_TRACE_BRANCH_PROFILING is enabled and to __gcov_init and
__gcov_exit when CONFIG_GCOV_KERNEL is enabled.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-Not only add -DDISABLE_BRANCH_PROFILING to the CFLAGS but also set:
 GCOV_PROFILE                    := n
 UBSAN_SANITIZE                  := n

Changes in v4:
-This is a new patch in v4 of this series
---
 arch/x86/purgatory/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index fb4ee5444379..4a35b9b94cb5 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -17,7 +17,9 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
 LDFLAGS_purgatory.ro := -e purgatory_start -r --no-undefined -nostdlib -z nodefaultlib
 targets += purgatory.ro
 
+GCOV_PROFILE	:= n
 KASAN_SANITIZE	:= n
+UBSAN_SANITIZE	:= n
 KCOV_INSTRUMENT := n
 
 # These are adjustments to the compiler flags used for objects that
@@ -25,7 +27,7 @@ KCOV_INSTRUMENT := n
 
 PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
 PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss
-PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN)
+PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
 
 # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That
 # in turn leaves some undefined symbols like __fentry__ in purgatory and not
-- 
2.25.1

[PATCH v5 2/2] x86/purgatory: Make sure we fail the build if purgatory.ro has missing symbols

[Hans de Goede]

Since we link purgatory.ro with -r aka we enable "incremental linking"
no checks for unresolved symbols is done while linking purgatory.ro.

Changes to the sha256 code has caused the purgatory in 5.4-rc1 to have
a missing symbol on memzero_explicit, yet things still happily build.

This commit adds an extra check for unresolved symbols by calling ld
without -r before running bin2c to generate kexec-purgatory.c.

This causes a build of 5.4-rc1 with this patch added to fail as it should:

  CHK     arch/x86/purgatory/purgatory.ro
ld: arch/x86/purgatory/purgatory.ro: in function `sha256_transform':
sha256.c:(.text+0x1c0c): undefined reference to `memzero_explicit'
make[2]: *** [arch/x86/purgatory/Makefile:72:
    arch/x86/purgatory/kexec-purgatory.c] Error 1
make[1]: *** [scripts/Makefile.build:509: arch/x86/purgatory] Error 2
make: *** [Makefile:1650: arch/x86] Error 2

This will help us catch missing symbols in the purgatory sooner.

Note this commit also removes --no-undefined from LDFLAGS_purgatory.ro
as that has no effect.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Add a .gitignore file with purgatory.chk listed in it

Changes in v2:
- Using 2 if_changed lines under a single rule does not work, then
  1 of the 2 will always execute each build.
  Instead add a new (unused) purgatory.chk intermediate which gets
  linked from purgatory.ro without -r to do the missing symbols check
- This also fixes the check generating an a.out file (oops)
---
 arch/x86/purgatory/.gitignore |  1 +
 arch/x86/purgatory/Makefile   | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/purgatory/.gitignore

diff --git a/arch/x86/purgatory/.gitignore b/arch/x86/purgatory/.gitignore
new file mode 100644
index 000000000000..d2be1500671d
--- /dev/null
+++ b/arch/x86/purgatory/.gitignore
@@ -0,0 +1 @@
+purgatory.chk
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 4a35b9b94cb5..85221cb71c72 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -14,8 +14,12 @@ $(obj)/sha256.o: $(srctree)/lib/crypto/sha256.c FORCE
 
 CFLAGS_sha256.o := -D__DISABLE_EXPORTS
 
-LDFLAGS_purgatory.ro := -e purgatory_start -r --no-undefined -nostdlib -z nodefaultlib
-targets += purgatory.ro
+# Since we link purgatory.ro with -r unresolved symbols are not checked, so we
+# also link a purgatory.chk binary without -r to check for unresolved symbols.
+PURGATORY_LDFLAGS := -e purgatory_start -nostdlib -z nodefaultlib
+LDFLAGS_purgatory.ro := -r $(PURGATORY_LDFLAGS)
+LDFLAGS_purgatory.chk := $(PURGATORY_LDFLAGS)
+targets += purgatory.ro purgatory.chk
 
 GCOV_PROFILE	:= n
 KASAN_SANITIZE	:= n
@@ -60,12 +64,15 @@ CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)
 
+$(obj)/purgatory.chk: $(obj)/purgatory.ro FORCE
+		$(call if_changed,ld)
+
 targets += kexec-purgatory.c
 
 quiet_cmd_bin2c = BIN2C   $@
       cmd_bin2c = $(objtree)/scripts/bin2c kexec_purgatory < $< > $@
 
-$(obj)/kexec-purgatory.c: $(obj)/purgatory.ro FORCE
+$(obj)/kexec-purgatory.c: $(obj)/purgatory.ro $(obj)/purgatory.chk FORCE
 	$(call if_changed,bin2c)
 
 obj-$(CONFIG_KEXEC_FILE)	+= kexec-purgatory.o
-- 
2.25.1

Re: [PATCH v5 1/2] x86/purgatory: Disable various profiling and sanitizing options

[Borislav Petkov]

On Thu, Mar 12, 2020 at 12:49:50PM +0100, Hans de Goede wrote:
> Since the purgatory is a special stand-alone binary, we need to disable

Pls use passive voice in your commit message: no "we" or "I", etc, and
describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

> various profiling and sanitizing options. Having these options enabled
> typically will cause dependency on various special symbols exported by
> special libs / stubs used by these frameworks. Since the purgatory is
> special we do not link against these stubs causing missing symbols in
> the purgatory if we do not disable these options.
> 
> This commit syncs the set of disabled profiling and sanitizing options

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

Those two review comments apply to patch 2's commit message too, pls fix
them there too.

> with that from drivers/firmware/efi/libstub/Makefile, adding
> -DDISABLE_BRANCH_PROFILING to the CFLAGS and setting:
> 
> GCOV_PROFILE                    := n
> UBSAN_SANITIZE                  := n
> 
> This fixes broken references to ftrace_likely_update when
> CONFIG_TRACE_BRANCH_PROFILING is enabled and to __gcov_init and
> __gcov_exit when CONFIG_GCOV_KERNEL is enabled.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -Not only add -DDISABLE_BRANCH_PROFILING to the CFLAGS but also set:
>  GCOV_PROFILE                    := n
>  UBSAN_SANITIZE                  := n
> 
> Changes in v4:
> -This is a new patch in v4 of this series
> ---
>  arch/x86/purgatory/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

$ test-apply.sh -g /tmp/01-x86-purgatory-disable_various_profiling_and_sanitizing_options.patch
checking file arch/x86/purgatory/Makefile
Hunk #1 FAILED at 17.
Hunk #2 succeeded at 27 (offset 2 lines).
1 out of 2 hunks FAILED

This happens because tip/master already has KCSAN merged in and it adds

KCSAN_SANITIZE  := n

there.

Please redo the patches against current tip/master.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 1/2] x86/purgatory: Disable various profiling and sanitizing options

[Hans de Goede]

Hi,

On 3/13/20 7:28 PM, Borislav Petkov wrote:
> On Thu, Mar 12, 2020 at 12:49:50PM +0100, Hans de Goede wrote:
>> Since the purgatory is a special stand-alone binary, we need to disable
> 
> Pls use passive voice in your commit message: no "we" or "I", etc, and
> describe your changes in imperative mood.
> 
> Also, pls read section "2) Describe your changes" in
> Documentation/process/submitting-patches.rst for more details.

Ok, fixed for v6 and I've also rebased the series on tip/master for v6.

Regards,

Hans



> 
>> various profiling and sanitizing options. Having these options enabled
>> typically will cause dependency on various special symbols exported by
>> special libs / stubs used by these frameworks. Since the purgatory is
>> special we do not link against these stubs causing missing symbols in
>> the purgatory if we do not disable these options.
>>
>> This commit syncs the set of disabled profiling and sanitizing options
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.
> 
> Those two review comments apply to patch 2's commit message too, pls fix
> them there too.
> 
>> with that from drivers/firmware/efi/libstub/Makefile, adding
>> -DDISABLE_BRANCH_PROFILING to the CFLAGS and setting:
>>
>> GCOV_PROFILE                    := n
>> UBSAN_SANITIZE                  := n
>>
>> This fixes broken references to ftrace_likely_update when
>> CONFIG_TRACE_BRANCH_PROFILING is enabled and to __gcov_init and
>> __gcov_exit when CONFIG_GCOV_KERNEL is enabled.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> -Not only add -DDISABLE_BRANCH_PROFILING to the CFLAGS but also set:
>>   GCOV_PROFILE                    := n
>>   UBSAN_SANITIZE                  := n
>>
>> Changes in v4:
>> -This is a new patch in v4 of this series
>> ---
>>   arch/x86/purgatory/Makefile | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> $ test-apply.sh -g /tmp/01-x86-purgatory-disable_various_profiling_and_sanitizing_options.patch
> checking file arch/x86/purgatory/Makefile
> Hunk #1 FAILED at 17.
> Hunk #2 succeeded at 27 (offset 2 lines).
> 1 out of 2 hunks FAILED
> 
> This happens because tip/master already has KCSAN merged in and it adds
> 
> KCSAN_SANITIZE  := n
> 
> there.
> 
> Please redo the patches against current tip/master.
> 
> Thx.
>