[v5] Add/enable stack protector

[PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

Posted by Volodymyr Babchuk 8 months, 3 weeks ago

Both GCC and Clang support -fstack-protector feature, which add stack
canaries to functions where stack corruption is possible. This series
makes possible to use this feature in Xen. I tested this on ARM64 and
it is working as intended. Tested both with GCC and Clang.

It is hard to enable this feature on x86, as GCC stores stack canary
in %fs:40 by default, but Xen can't use %fs for various reasons. It is
possibly to change stack canary location new newer GCC versions, but
attempt to do this uncovered a whole host problems with GNU ld.
So, this series focus mostly on ARM.

Previous version of the series was acked for 4.20 release.

Changes in v5:

 - ARM code calls boot_stack_chk_guard_setup() from early C code
 - Bringed back stack-protector.h because C code needs to call
   boot_stack_chk_guard_setup()
 - Fixed formatting
 - Added Andrew's R-b tag

Changes in v4:

 - Added patch to CHANGELOG.md
 - Removed stack-protector.h because we dropped support for
   Xen's built-in RNG code and rely only on own implementation
 - Changes in individual patches are covered in their respect commit
 messages

Changes in v3:

 - Removed patch for riscv
 - Changes in individual patches are covered in their respect commit
 messages

Changes in v2:

 - Patch "xen: common: add ability to enable stack protector" was
   divided into two patches.
 - Rebase onto Andrew's patch that removes -fno-stack-protector-all
 - Tested on RISC-V thanks to Oleksii Kurochko
 - Changes in individual patches covered in their respect commit
 messages

Volodymyr Babchuk (4):
  common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature
  CHANGELOG.md: Mention stack-protector feature

 CHANGELOG.md                         |  1 +
 Config.mk                            |  2 +-
 stubdom/Makefile                     |  2 ++
 tools/firmware/Rules.mk              |  2 ++
 tools/tests/x86_emulator/testcase.mk |  2 +-
 xen/Makefile                         |  6 ++++
 xen/arch/arm/Kconfig                 |  1 +
 xen/arch/arm/setup.c                 |  3 ++
 xen/arch/x86/boot/Makefile           |  1 +
 xen/common/Kconfig                   | 15 ++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 51 ++++++++++++++++++++++++++++
 xen/include/xen/stack-protector.h    | 14 ++++++++
 13 files changed, 99 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.47.1

Re: [PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

Posted by Andrew Cooper 8 months, 3 weeks ago

On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
> Volodymyr Babchuk (4):
>   common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
>   xen: common: add ability to enable stack protector
>   xen: arm: enable stack protector feature
>   CHANGELOG.md: Mention stack-protector feature

Given the last minute observation on v4, I ran this through GitlabCI
with STACK_PROTECTOR forced on.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
is the full run.

https://gitlab.com/xen-project/people/andyhhp/xen/-/commit/b07b024f51907bc0f93d581287d36cf5bbfa98e2
is the patch to force things on, including some extra UBSAN because that
got missed.

This is relevant because the only 3 failures present are ARM32 UBSAN
failures in credit2.

Second, in all 3 failures, we've got an `ERROR-EOF!` interrupting the
backtrace, which I presume is something coming out of Expect.  Stefano,
any ideas?

My main observation is that there's no exterior way of telling whether
stack protector is actually active or not.  i.e. nothing printed during
setup.  However, all 4 builds did build common/stack-protector.o so I'm
assuming it was properly active.

If it was going to explode, it would have done before the UBSAN
failures, which are reasonably late on boot.

~Andrew

Re: [PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

Posted by Volodymyr Babchuk 8 months, 2 weeks ago

Hi Andrew,

Andrew Cooper <andrew.cooper3@citrix.com> writes:

> On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
>> Volodymyr Babchuk (4):
>>   common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
>>   xen: common: add ability to enable stack protector
>>   xen: arm: enable stack protector feature
>>   CHANGELOG.md: Mention stack-protector feature
>
> Given the last minute observation on v4, I ran this through GitlabCI
> with STACK_PROTECTOR forced on.
>
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
> is the full run.

I noticed that you enabled both UBSAN and STACK_PROTECTOR for
arm32. Have you tried to run arm32 test with UBSAN only?

[...]

--
WBR, Volodymyr

Re: [PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

Posted by Andrew Cooper 8 months, 2 weeks ago

On 14/02/2025 12:34 am, Volodymyr Babchuk wrote:
> Hi Andrew,
>
> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
>>> Volodymyr Babchuk (4):
>>>   common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
>>>   xen: common: add ability to enable stack protector
>>>   xen: arm: enable stack protector feature
>>>   CHANGELOG.md: Mention stack-protector feature
>> Given the last minute observation on v4, I ran this through GitlabCI
>> with STACK_PROTECTOR forced on.
>>
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
>> is the full run.
> I noticed that you enabled both UBSAN and STACK_PROTECTOR for
> arm32. Have you tried to run arm32 test with UBSAN only?

No, but I'm also confident that the UBSAN failure is unrelated to
STACK_PROTECTOR.

It turns out it's very gnarly to fix.

~Andrew