diff mbox series

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

Message ID 20201120212912.2993985-1-christoph.muellner@theobroma-systems.com
State New
Headers show
Series [1/1] boot/arm-trusted-firmware: Forward stack protection configuration | expand

Commit Message

Christoph Müllner Nov. 20, 2020, 9:29 p.m. UTC
TF-A supports stack smashing protection (-fstack-protector-*).
Since we already forward the required compiler flag, let's
also tell TF-A that we actually want the required symbols
(e.g. __stack_chk_guard) to be available.

Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
---
 boot/arm-trusted-firmware/arm-trusted-firmware.mk | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Baruch Siach Nov. 21, 2020, 4:59 p.m. UTC | #1
Hi Christoph,

On Fri, Nov 20 2020, Christoph Müllner wrote:
> TF-A supports stack smashing protection (-fstack-protector-*).
> Since we already forward the required compiler flag, let's
> also tell TF-A that we actually want the required symbols
> (e.g. __stack_chk_guard) to be available.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
> ---
>  boot/arm-trusted-firmware/arm-trusted-firmware.mk | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> index a3553e36cf..f5be39100e 100644
> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
> @@ -100,6 +100,18 @@ 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
> +endif
> +
> +ifeq ($(BR2_SSP_STRONG),y)
> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=strong
> +endif
> +
> +ifeq ($(BR2_SSP_ALL),y)
> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=all
> +endif

It looks like the toolchain wrapper does that already. See
toolchain/toolchain-wrapper.mk. Are you sure this is needed?

baruch
Christoph Müllner Nov. 21, 2020, 5:28 p.m. UTC | #2
On 11/21/20 5:59 PM, Baruch Siach wrote:
> Hi Christoph,
> 
> On Fri, Nov 20 2020, Christoph Müllner wrote:
>> TF-A supports stack smashing protection (-fstack-protector-*).
>> Since we already forward the required compiler flag, let's
>> also tell TF-A that we actually want the required symbols
>> (e.g. __stack_chk_guard) to be available.
>>
>> Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
>> ---
>>  boot/arm-trusted-firmware/arm-trusted-firmware.mk | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>> index a3553e36cf..f5be39100e 100644
>> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>> @@ -100,6 +100,18 @@ 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
>> +endif
>> +
>> +ifeq ($(BR2_SSP_STRONG),y)
>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=strong
>> +endif
>> +
>> +ifeq ($(BR2_SSP_ALL),y)
>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=all
>> +endif
> 
> It looks like the toolchain wrapper does that already. See
> toolchain/toolchain-wrapper.mk. Are you sure this is needed?

Hi Baruch,

I don't see this functionality there.

Note, that you need two things for getting the stack protector working:

1) Toolchain to emit additional code requiring external symbols
2) Runtime that provides the required external symbols

The toolchain wrapper correctly addresses 1) by providing "-fstack-protector*" flags
and your libc will then provide the required symbols for 2).
However, TF-A does not link against libc and requires these build flags
(ENABLE_STACK_PROTECTOR=x) in order to emit the required symbols.

BR
Christoph
Baruch Siach Nov. 21, 2020, 5:49 p.m. UTC | #3
Hi Christoph,

On Sat, Nov 21 2020, Christoph Müllner wrote:
> On 11/21/20 5:59 PM, Baruch Siach wrote:
>> On Fri, Nov 20 2020, Christoph Müllner wrote:
>>> TF-A supports stack smashing protection (-fstack-protector-*).
>>> Since we already forward the required compiler flag, let's
>>> also tell TF-A that we actually want the required symbols
>>> (e.g. __stack_chk_guard) to be available.
>>>
>>> Signed-off-by: Christoph Müllner <christoph.muellner@theobroma-systems.com>
>>> ---
>>>  boot/arm-trusted-firmware/arm-trusted-firmware.mk | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/boot/arm-trusted-firmware/arm-trusted-firmware.mk b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>> index a3553e36cf..f5be39100e 100644
>>> --- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>> +++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
>>> @@ -100,6 +100,18 @@ 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
>>> +endif
>>> +
>>> +ifeq ($(BR2_SSP_STRONG),y)
>>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=strong
>>> +endif
>>> +
>>> +ifeq ($(BR2_SSP_ALL),y)
>>> +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=all
>>> +endif
>> 
>> It looks like the toolchain wrapper does that already. See
>> toolchain/toolchain-wrapper.mk. Are you sure this is needed?
>
> Hi Baruch,
>
> I don't see this functionality there.
>
> Note, that you need two things for getting the stack protector working:
>
> 1) Toolchain to emit additional code requiring external symbols
> 2) Runtime that provides the required external symbols
>
> The toolchain wrapper correctly addresses 1) by providing "-fstack-protector*" flags
> and your libc will then provide the required symbols for 2).
> However, TF-A does not link against libc and requires these build flags
> (ENABLE_STACK_PROTECTOR=x) in order to emit the required symbols.

Thanks for the additional explanation. Your commit log mentions that
already, but maybe it needs to be more verbose. Especially the fact that
ATF needs to provide the symbols itself because it is self contained
would be nice to mention in the commit log.

One more thing. Since all these BR2_SSP_ options are mutually
exclusive, I think you should use 'else ifeq' instead of separate
conditionals.

Thanks,
baruch
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..f5be39100e 100644
--- a/boot/arm-trusted-firmware/arm-trusted-firmware.mk
+++ b/boot/arm-trusted-firmware/arm-trusted-firmware.mk
@@ -100,6 +100,18 @@  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
+endif
+
+ifeq ($(BR2_SSP_STRONG),y)
+ARM_TRUSTED_FIRMWARE_MAKE_OPTS += ENABLE_STACK_PROTECTOR=strong
+endif
+
+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)