[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Ard Biesheuvel]

When building the ARM kernel with CONFIG_EFI=y, the following build
error may occur when using a less recent version of binutils (2.23 or
older):

   STUBCPY drivers/firmware/efi/libstub/lib-sort.stub.o
 00000000 R_ARM_ABS32       sort
 00000004 R_ARM_ABS32       __ksymtab_strings
 drivers/firmware/efi/libstub/lib-sort.stub.o: absolute symbol references
 not allowed in the EFI stub

(and when building with debug symbols, the list above is much longer, and
contains all the internal references between the .debug sections and the
actual code)

This issue is caused by the fact that objcopy v2.23 or earlier does not
support wildcards in its -R and -j options, which means the following
line from the Makefile:

  STUBCOPY_FLAGS-y		:= -R .debug* -R *ksymtab* -R *kcrctab*

fails to take effect, leaving harmless absolute relocations in the binary
that are indistinguishable from relocations that may cause crashes at
runtime due to the fact that these relocations are resolved at link time
using the virtual address of the kernel, which is always different from
the address at which the EFI firmware loads and invokes the stub.

So, as a workaround, disable debug symbols explicitly when building the
stub for ARM, and strip the ksymtab and kcrctab symbols for the only
exported symbol we currently reuse in the stub, which is 'sort'.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

This is a workaround for now. We can revisit this when a need arises to copy
more kernel code into the stub, by which time we could put in a more elaborate
fix, or decide to no longer care about 'older' versions of objcopy.

Since this fixes an ARM specific issue and only affects ARM specific Makefile
variables, I am happy for this to go on top of the arm-soc patch that enables
CONFIG_EFI for ARM's multi_v7_defconfig (queued for v4.9), given that we have
no other changes queued in linux-efi that should conflict with this patch.

Matt, any concerns?

 drivers/firmware/efi/libstub/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index c06945160a41..5e23e2d305e7 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -11,7 +11,7 @@ cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ $(LINUX_INCLUDE) -O2 \
 				   -mno-mmx -mno-sse
 
 cflags-$(CONFIG_ARM64)		:= $(subst -pg,,$(KBUILD_CFLAGS))
-cflags-$(CONFIG_ARM)		:= $(subst -pg,,$(KBUILD_CFLAGS)) \
+cflags-$(CONFIG_ARM)		:= $(subst -pg,,$(KBUILD_CFLAGS)) -g0 \
 				   -fno-builtin -fpic -mno-single-pic-base
 
 cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
@@ -79,5 +79,6 @@ quiet_cmd_stubcopy = STUBCPY $@
 # decompressor. So move our .data to .data.efistub, which is preserved
 # explicitly by the decompressor linker script.
 #
-STUBCOPY_FLAGS-$(CONFIG_ARM)	+= --rename-section .data=.data.efistub
+STUBCOPY_FLAGS-$(CONFIG_ARM)	+= --rename-section .data=.data.efistub \
+				   -R ___ksymtab+sort -R ___kcrctab+sort
 STUBCOPY_RELOC-$(CONFIG_ARM)	:= R_ARM_ABS
-- 
2.7.4

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Matt Fleming]

On Fri, 30 Sep, at 04:01:55PM, Ard Biesheuvel wrote:
> 
> This is a workaround for now. We can revisit this when a need arises to copy
> more kernel code into the stub, by which time we could put in a more elaborate
> fix, or decide to no longer care about 'older' versions of objcopy.
> 
> Since this fixes an ARM specific issue and only affects ARM specific Makefile
> variables, I am happy for this to go on top of the arm-soc patch that enables
> CONFIG_EFI for ARM's multi_v7_defconfig (queued for v4.9), given that we have
> no other changes queued in linux-efi that should conflict with this patch.
> 
> Matt, any concerns?

Not with the patch, but could we clarify the user-visible effects of
not applying it? Are the absolute relocations harmless, or will they
lead to crashes? 

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Jon Hunter]

On 01/10/16 00:01, Ard Biesheuvel wrote:
> When building the ARM kernel with CONFIG_EFI=y, the following build
> error may occur when using a less recent version of binutils (2.23 or
> older):
> 
>    STUBCPY drivers/firmware/efi/libstub/lib-sort.stub.o
>  00000000 R_ARM_ABS32       sort
>  00000004 R_ARM_ABS32       __ksymtab_strings
>  drivers/firmware/efi/libstub/lib-sort.stub.o: absolute symbol references
>  not allowed in the EFI stub
> 
> (and when building with debug symbols, the list above is much longer, and
> contains all the internal references between the .debug sections and the
> actual code)
> 
> This issue is caused by the fact that objcopy v2.23 or earlier does not
> support wildcards in its -R and -j options, which means the following
> line from the Makefile:
> 
>   STUBCOPY_FLAGS-y		:= -R .debug* -R *ksymtab* -R *kcrctab*
> 
> fails to take effect, leaving harmless absolute relocations in the binary
> that are indistinguishable from relocations that may cause crashes at
> runtime due to the fact that these relocations are resolved at link time
> using the virtual address of the kernel, which is always different from
> the address at which the EFI firmware loads and invokes the stub.
> 
> So, as a workaround, disable debug symbols explicitly when building the
> stub for ARM, and strip the ksymtab and kcrctab symbols for the only
> exported symbol we currently reuse in the stub, which is 'sort'.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks fixes the issues I was seeing. So ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Ard Biesheuvel]

On 3 October 2016 at 13:52, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 30 Sep, at 04:01:55PM, Ard Biesheuvel wrote:
>>
>> This is a workaround for now. We can revisit this when a need arises to copy
>> more kernel code into the stub, by which time we could put in a more elaborate
>> fix, or decide to no longer care about 'older' versions of objcopy.
>>
>> Since this fixes an ARM specific issue and only affects ARM specific Makefile
>> variables, I am happy for this to go on top of the arm-soc patch that enables
>> CONFIG_EFI for ARM's multi_v7_defconfig (queued for v4.9), given that we have
>> no other changes queued in linux-efi that should conflict with this patch.
>>
>> Matt, any concerns?
>
> Not with the patch, but could we clarify the user-visible effects of
> not applying it? Are the absolute relocations harmless, or will they
> lead to crashes?

These relocations are harmless, since the debug ones are only
interpreted by the debugger, and the ones generated by
EXPORT_SYMBOL(sort) will never be referenced, since the symbols they
contain are either renamed to __efistub_xxx (arm64), or they are not
part of the kernel proper (arm)

So both cases are false positives, but the diagnostic is important,
and so breaking the build is appropriate for any other absolute
relocation that may appear.

The effect of the patch is not that the diagnostic is ignored, but
that these relocations are not generated in the first place (-g0) or
removed explicitly (ksymtab/krcrctab+sort) rather than via a wildcard.
So other than not breaking the build, this patch should have no user
observeable differences.

-- 
Ard.

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Matt Fleming]

On Tue, 04 Oct, at 11:34:31AM, Ard Biesheuvel wrote:
> 
> These relocations are harmless, since the debug ones are only
> interpreted by the debugger, and the ones generated by
> EXPORT_SYMBOL(sort) will never be referenced, since the symbols they
> contain are either renamed to __efistub_xxx (arm64), or they are not
> part of the kernel proper (arm)
> 
> So both cases are false positives, but the diagnostic is important,
> and so breaking the build is appropriate for any other absolute
> relocation that may appear.
> 
> The effect of the patch is not that the diagnostic is ignored, but
> that these relocations are not generated in the first place (-g0) or
> removed explicitly (ksymtab/krcrctab+sort) rather than via a wildcard.
> So other than not breaking the build, this patch should have no user
> observeable differences.

Thanks Ard, sounds reasonable. Feel free to take this through
whichever tree you think is best.

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Ard Biesheuvel]

On 4 October 2016 at 22:30, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 04 Oct, at 11:34:31AM, Ard Biesheuvel wrote:
>>
>> These relocations are harmless, since the debug ones are only
>> interpreted by the debugger, and the ones generated by
>> EXPORT_SYMBOL(sort) will never be referenced, since the symbols they
>> contain are either renamed to __efistub_xxx (arm64), or they are not
>> part of the kernel proper (arm)
>>
>> So both cases are false positives, but the diagnostic is important,
>> and so breaking the build is appropriate for any other absolute
>> relocation that may appear.
>>
>> The effect of the patch is not that the diagnostic is ignored, but
>> that these relocations are not generated in the first place (-g0) or
>> removed explicitly (ksymtab/krcrctab+sort) rather than via a wildcard.
>> So other than not breaking the build, this patch should have no user
>> observeable differences.
>
> Thanks Ard, sounds reasonable. Feel free to take this through
> whichever tree you think is best.
>
> Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

Thanks Matt.

Arnd: could you take this on top of the patch that adds CONFIG_EFI to
multi_v7_defconfig? That would minimize the breakage, I think.

[PATCH] efi/arm: fix absolute relocation detection for older toolchains

[Jon Hunter]

On 05/10/16 18:30, Ard Biesheuvel wrote:
> On 4 October 2016 at 22:30, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> On Tue, 04 Oct, at 11:34:31AM, Ard Biesheuvel wrote:
>>>
>>> These relocations are harmless, since the debug ones are only
>>> interpreted by the debugger, and the ones generated by
>>> EXPORT_SYMBOL(sort) will never be referenced, since the symbols they
>>> contain are either renamed to __efistub_xxx (arm64), or they are not
>>> part of the kernel proper (arm)
>>>
>>> So both cases are false positives, but the diagnostic is important,
>>> and so breaking the build is appropriate for any other absolute
>>> relocation that may appear.
>>>
>>> The effect of the patch is not that the diagnostic is ignored, but
>>> that these relocations are not generated in the first place (-g0) or
>>> removed explicitly (ksymtab/krcrctab+sort) rather than via a wildcard.
>>> So other than not breaking the build, this patch should have no user
>>> observeable differences.
>>
>> Thanks Ard, sounds reasonable. Feel free to take this through
>> whichever tree you think is best.
>>
>> Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
> 
> Thanks Matt.
> 
> Arnd: could you take this on top of the patch that adds CONFIG_EFI to
> multi_v7_defconfig? That would minimize the breakage, I think.

Can someone pick up this fix? -next has been broken for me since 20th
Sept :-(

Cheers
Jon

-- 
nvpublic