[Buildroot] [PATCH v2 1/1] boot/arm-trusted-firmware: Forward stack protection configuration

[Christoph Müllner]

On 11/23/20 1:39 PM, Baruch Siach wrote:
> Hi Christoph,
> 
> On Mon, Nov 23 2020, Christoph Müllner wrote:
>> On 11/22/20 6:27 PM, Baruch Siach wrote:
>>> On Sun, Nov 22 2020, Christoph Müllner wrote:
>>>> TF-A supports stack smashing protection (-fstack-protector-*).
>>>> However it currenlty fails to build when built with BR2_SSP_*
>>>> enabled, because stack protection needs to be enabled for the
>>>> TF-A build process itself as well to generate the required
>>>> symbols (e.g. __stack_chk_guard).
>>>
>>> So you are saying that the toolchain wrapper actually breaks ATF build
>>> when SSP is enabled. Is that correct? If so, this patch is not (only)
>>> about enabling the SSP feature for ATF, but about fixing the ATF
>>> build. Can you add the build failure error message to the commit log?
>>
>> I double checked that.
>> When SSP is enabled and the build system does not provide
>> the ENABLE_STACK_PROTECTOR flags, then the TF-A build process
>> used to break at link time.
>>
>> This behavior was changed a year ago from from "linking breaks"
>> to "silently disable the feature":
>>   https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=7af195e29a4213eefac0661d84e1c9c20476e166
>>
>> So now we end up with a TF-A without stack protection
>> in case we enable BR2_SSP_*.
>>
>> So Buildroot expects that stack protection is enabled when
>> "-fstack-protector*" is enabled, but TF-A requires additional
>> flags.
>>
>> FWIW, the link errors with older TF-A (I tested with v2.2) builds are:
>>
>>> [...]
>>> params_setup.c:(.text.params_early_setup+0xc): undefined reference to `__stack_chk_guard'
>>> /home/cm/build-debug/host/bin/aarch64-none-linux-gnu-ld: params_setup.c:(.text.params_early_setup+0x14): undefined reference to `__stack_chk_guard'
>>> /home/cm/build-debug/host/bin/aarch64-none-linux-gnu-ld: params_setup.c:(.text.params_early_setup+0x104): undefined reference to `__stack_chk_guard'
>>> /home/cm/build-debug/host/bin/aarch64-none-linux-gnu-ld: params_setup.c:(.text.params_early_setup+0x118): undefined reference to `__stack_chk_fail'
>>> /home/cm/build-debug/host/bin/aarch64-none-linux-gnu-ld: ./build/px30/release/bl31/pmu.o: in function `rockchip_soc_sys_pwr_dm_suspend':
>>> pmu.c:(.text.rockchip_soc_sys_pwr_dm_suspend+0xc): undefined reference to `__stack_chk_guard'
>>> [...]
>>
>>> Also, the subject line should say something like "fix build with SSP
>>> enabled".
>>>
>>> This patch should be applied to the master branch, and backported to
>>> stable branches, I believe.
>>
>> I am not sure about this.
>> Is an enabled, but silently disabled, hardening feature considered as bug?
>> If so, the we should reach out to the TF-A devs and ask for a different
>> build policy here (i.e prefer hardening over build success).
> 
> I agree that a silently disabled feature is not a bug. But a build
> failure is a bug.
> 
> arm-trusted-firmware.mk is meant to support older ATF version as well,
> just like uboot.mk, and linux.mk. We have configurations under configs/
> that set custom, vendor provided, ATF versions. In many (most?) cases
> these versions predate ATF commit 7af195e29a4. So build breaks for them
> once you enable SSP.
> 
> So I still think that the commit log should mention the fix to build
> breakage of older ATF versions.

Ok, will send out a v4.

> 
> baruch
> 
>>>> So in case we see that BR2_SSP_* is enabled, let's enable
>>>> the corresponding build flag for TF-A as documented in
>>>> the TF-A user guide.
>>>>
>>>> Tested on a Rockchip PX30 based system.
>>>>
>>>> Signed-off-by: Christoph Müllner <christoph.muellner at theobroma-systems.com>
>>>> ---
>>>>  boot/arm-trusted-firmware/arm-trusted-firmware.mk | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>>> index a3553e36cf..0597cecf71 100644
>>>> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>>> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>>> @@ -100,6 +100,14 @@ ARM_TRUSTED_FIRMWARE_MAKE_OPTS += MV_DDR_PATH=$(MV_DDR_MARVELL_DIR)
>>>>  ARM_TRUSTED_FIRMWARE_DEPENDENCIES += mv-ddr-marvell
>>>>  endif
>>>>  
>>>> +ifeq ($(BR2_SSP_REGULAR),y)
>>>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=default
>>>> +else ifeq ($(BR2_SSP_STRONG),y)
>>>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=strong
>>>> +else ifeq ($(BR2_SSP_ALL),y)
>>>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=all
>>>> +endif
>>>> +
>>>>  ARM_TRUSTED_FIRMWARE_MAKE_TARGETS = all
>>>>  
>>>>  ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_FIP),y)
>