diff mbox series

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

Message ID 20201122143731.3374802-2-christoph.muellner@theobroma-systems.com
State Changes Requested
Headers show
Series [v2,1/1] boot/arm-trusted-firmware: Forward stack protection configuration | expand

Commit Message

Christoph Müllner Nov. 22, 2020, 2:37 p.m. UTC
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 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@theobroma-systems.com>
---
 boot/arm-trusted-firmware/arm-trusted-firmware.mk | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Baruch Siach Nov. 22, 2020, 5:27 p.m. UTC | #1
Hi Christoph,

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?

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.

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@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)
Christoph Müllner Nov. 23, 2020, 9:43 a.m. UTC | #2
Hi Baruch,

On 11/22/20 6:27 PM, Baruch Siach wrote:
> Hi Christoph,
> 
> 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).

BR,
Christoph

> 
> 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@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)
> 
>
Baruch Siach Nov. 23, 2020, 12:39 p.m. UTC | #3
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.

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@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)
Christoph Müllner Nov. 23, 2020, 1:14 p.m. UTC | #4
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@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)
>
diff mbox series

Patch

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)