mbox series

[v3,0/3] Add stack protector

Message ID 20241211020424.401614-1-volodymyr_babchuk@epam.com (mailing list archive)
Headers show
Series Add stack protector | expand

Message

Volodymyr Babchuk Dec. 11, 2024, 2:04 a.m. UTC 
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
this will change minimal GCC requirement, which is also hard due to
various reasons. So, this series focus mostly on ARM and RISCV.

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 (3):
  common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature

 Config.mk                            |  2 +-
 stubdom/Makefile                     |  3 ++
 tools/firmware/Rules.mk              |  2 ++
 tools/tests/x86_emulator/testcase.mk |  2 +-
 xen/Makefile                         |  6 ++++
 xen/arch/arm/Kconfig                 |  1 +
 xen/arch/arm/arm64/head.S            |  3 ++
 xen/arch/arm/setup.c                 |  3 ++
 xen/common/Kconfig                   | 15 +++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 47 ++++++++++++++++++++++++++++
 xen/include/asm-generic/random.h     |  5 +++
 xen/include/xen/stack-protector.h    | 30 ++++++++++++++++++
 13 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

Comments

Jan Beulich Dec. 11, 2024, 7:46 a.m. UTC | #1 
On 11.12.2024 03:04, Volodymyr Babchuk wrote:
> 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
> this will change minimal GCC requirement, which is also hard due to
> various reasons. So, this series focus mostly on ARM and RISCV.

Why exactly would it not be possible to offer the feature when new enough
gcc is in use?

Jan
Volodymyr Babchuk Dec. 12, 2024, 12:13 a.m. UTC | #2 
Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>> 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
>> this will change minimal GCC requirement, which is also hard due to
>> various reasons. So, this series focus mostly on ARM and RISCV.
>
> Why exactly would it not be possible to offer the feature when new enough
> gcc is in use?

It is possible to use this feature with a modern enough GCC, yes. Are
you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
x86 platform?
Andrew Cooper Dec. 12, 2024, 1:17 a.m. UTC | #3 
On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
> Hello Jan,
>
> Jan Beulich <jbeulich@suse.com> writes:
>
>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>> 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
>>> this will change minimal GCC requirement, which is also hard due to
>>> various reasons. So, this series focus mostly on ARM and RISCV.
>> Why exactly would it not be possible to offer the feature when new enough
>> gcc is in use?
> It is possible to use this feature with a modern enough GCC, yes. Are
> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
> x86 platform?

(With the knowledge that this is a disputed Kconfig pattern, and will
need rebasing), the way I want this to work is simply:

diff --git a/xen/Makefile b/xen/Makefile
index 0de0101fd0bf..5d0a88fb3c3f 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -434,6 +434,9 @@ endif
 
 ifeq ($(CONFIG_STACK_PROTECTOR),y)
 CFLAGS += -fstack-protector
+ifeq ($(CONFIG_X86),y)
+CFLAGS += -mstack-protector-guard=global
+endif
 else
 CFLAGS += -fno-stack-protector
 endif
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9cdd04721afa..7951ca908b36 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -28,6 +28,7 @@ config X86
        select HAS_PCI_MSI
        select HAS_PIRQ
        select HAS_SCHED_GRANULARITY
+       select HAS_STACK_PROTECTOR if
$(cc-option,-mstack-protector-guard=global)
        select HAS_UBSAN
        select HAS_VMAP
        select HAS_VPCI if HVM



Sadly, it doesn't build.  I get a handful of:

prelink.o: in function `cmdline_parse':
/home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
--no-relax
/home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
--no-relax

which is more toolchain-whispering than I feel like doing tonight.

~Andrew
Andrew Cooper Dec. 12, 2024, 1:19 a.m. UTC | #4 
On 12/12/2024 1:17 am, Andrew Cooper wrote:
> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>> Hello Jan,
>>
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>>> 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
>>>> this will change minimal GCC requirement, which is also hard due to
>>>> various reasons. So, this series focus mostly on ARM and RISCV.
>>> Why exactly would it not be possible to offer the feature when new enough
>>> gcc is in use?
>> It is possible to use this feature with a modern enough GCC, yes. Are
>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>> x86 platform?
> (With the knowledge that this is a disputed Kconfig pattern, and will
> need rebasing), the way I want this to work is simply:
>
> diff --git a/xen/Makefile b/xen/Makefile
> index 0de0101fd0bf..5d0a88fb3c3f 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -434,6 +434,9 @@ endif
>  
>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>  CFLAGS += -fstack-protector
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -mstack-protector-guard=global
> +endif
>  else
>  CFLAGS += -fno-stack-protector
>  endif
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..7951ca908b36 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>         select HAS_PCI_MSI
>         select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
> +       select HAS_STACK_PROTECTOR if
> $(cc-option,-mstack-protector-guard=global)
>         select HAS_UBSAN
>         select HAS_VMAP
>         select HAS_VPCI if HVM
>
>
>
> Sadly, it doesn't build.  I get a handful of:
>
> prelink.o: in function `cmdline_parse':
> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
>
> which is more toolchain-whispering than I feel like doing tonight.

P.S.  Irrespective of the x86 side of things, you need a final patch on
your series adjusting CHANGELOG.md.

~Andrew