[PATCH v4 0/4] Add/enable stack protector

Volodymyr Babchuk posted 4 patches 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250114042553.1624831-1-volodymyr._5Fbabchuk@epam.com
There is a newer version of this series
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/arm64/head.S            |  3 ++
xen/arch/x86/boot/Makefile           |  1 +
xen/common/Kconfig                   | 15 ++++++++
xen/common/Makefile                  |  1 +
xen/common/stack-protector.c         | 51 ++++++++++++++++++++++++++++
12 files changed, 85 insertions(+), 2 deletions(-)
create mode 100644 xen/common/stack-protector.c
[PATCH v4 0/4] Add/enable stack protector
Posted by Volodymyr Babchuk 1 year 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.

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/arm64/head.S            |  3 ++
 xen/arch/x86/boot/Makefile           |  1 +
 xen/common/Kconfig                   | 15 ++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 51 ++++++++++++++++++++++++++++
 12 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c

-- 
2.47.1
Re: [PATCH v4 0/4] Add/enable stack protector
Posted by Andrew Cooper 1 year ago
On 14/01/2025 4:25 am, 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

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

There's one minor formatting error which can be fixed on commit.

~Andrew
Re: [PATCH v4 0/4] Add/enable stack protector
Posted by Volodymyr Babchuk 12 months ago
Hi Andrew,

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

> On 14/01/2025 4:25 am, 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
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> There's one minor formatting error which can be fixed on commit.
>
> ~Andrew

Thanks for the review. I noticed that this series is not committed. Is
there anything else required from my side?

-- 
WBR, Volodymyr
Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Andrew Cooper 12 months ago
On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
> Hi Andrew,
>
> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> On 14/01/2025 4:25 am, 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
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> There's one minor formatting error which can be fixed on commit.
>>
>> ~Andrew
> Thanks for the review. I noticed that this series is not committed. Is
> there anything else required from my side?
>

You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
enough.

And at this point at rc4, you'll need to persuade Oleksii to take it for
4.20.

Personally I think it's low risk and worthwhile to take for 4.20, and it
was technically completed in time - it just fell between the cracks.

~Andrew

Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Oleksii Kurochko 12 months ago
On 2/13/25 3:07 PM, Andrew Cooper wrote:
> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>> Hi Andrew,
>>
>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>
>>> On 14/01/2025 4:25 am, 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
>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>
>>> There's one minor formatting error which can be fixed on commit.
>>>
>>> ~Andrew
>> Thanks for the review. I noticed that this series is not committed. Is
>> there anything else required from my side?
>>
> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
> enough.
>
> And at this point at rc4, you'll need to persuade Oleksii to take it for
> 4.20.
>
> Personally I think it's low risk and worthwhile to take for 4.20, and it
> was technically completed in time - it just fell between the cracks.

I think the same it's low risk patch series, so we can take it for 4.20:
  Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>

Thanks.

~ Oleksii

>
> ~Andrew
Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Oleksii Kurochko 12 months ago
On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>
>
> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>> Hi Andrew,
>>>
>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>
>>>> On 14/01/2025 4:25 am, 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
>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>
>>>> There's one minor formatting error which can be fixed on commit.
>>>>
>>>> ~Andrew
>>> Thanks for the review. I noticed that this series is not committed. Is
>>> there anything else required from my side?
>>>
>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>> enough.

Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

>> And at this point at rc4, you'll need to persuade Oleksii to take it for
>> 4.20.
>>
>> Personally I think it's low risk and worthwhile to take for 4.20, and it
>> was technically completed in time - it just fell between the cracks.
> I think the same it's low risk patch series, so we can take it for 4.20:
>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>
>
> Thanks.
>
> ~ Oleksii
>> ~Andrew
Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Jan Beulich 12 months ago
On 13.02.2025 15:26, Oleksii Kurochko wrote:
> 
> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
> 
> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

At a guess Andrew found Volodymyr in the ARM section of maintainers, but
then didn't pay close attention to the R: (rather than M:).

Jan

Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Andrew Cooper 12 months ago
On 13/02/2025 2:30 pm, Jan Beulich wrote:
> On 13.02.2025 15:26, Oleksii Kurochko wrote:
>> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>>
>>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>>
>>>>>> On 14/01/2025 4:25 am, 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
>>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>>
>>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>>
>>>>>> ~Andrew
>>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>>> there anything else required from my side?
>>>>>
>>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>>> enough.
>> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
>> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.
> At a guess Andrew found Volodymyr in the ARM section of maintainers, but
> then didn't pay close attention to the R: (rather than M:).

My apologies.  Yes, I did fail to read the small print.

~Andrew

Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Andrew Cooper 12 months ago
On 13/02/2025 2:26 pm, Oleksii Kurochko wrote:
>
>
> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

Volodymyr is an ARM maintainer (so qualifies for the ARM requirement),
and my R-by covers the "looked over by any other person" requirement.

~Andrew

Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Julien Grall 12 months ago
Hi,

On 13/02/2025 14:21, Oleksii Kurochko wrote:
> 
> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>> Hi Andrew,
>>>
>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>
>>>> On 14/01/2025 4:25 am, 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
>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>
>>>> There's one minor formatting error which can be fixed on commit.
>>>>
>>>> ~Andrew
>>> Thanks for the review. I noticed that this series is not committed. Is
>>> there anything else required from my side?
>>>
>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>> enough.

I beg to differ. For low level code, you really ought to have Arm folks 
to confirm this is correct. In fact, I don't think patch #3 it is. So ...

>>
>> And at this point at rc4, you'll need to persuade Oleksii to take it for
>> 4.20.
>>
>> Personally I think it's low risk and worthwhile to take for 4.20, and it
>> was technically completed in time - it just fell between the cracks.
> 
> I think the same it's low risk patch series, so we can take it for 4.20:
>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>

... I should not go to 4.20 as-is.

And before someone ask why it wasn't answered early. I can't comment for 
the other Arm maintainers, but I have been away for the past two months. 
So still catching up on my emails.

Cheers,

-- 
Julien Grall


Re: [PATCH v4 for-4.20(?) 0/4] Add/enable stack protector
Posted by Oleksii Kurochko 12 months ago
On 2/13/25 3:24 PM, Julien Grall wrote:
> Hi,
>
> On 13/02/2025 14:21, Oleksii Kurochko wrote:
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
>
> I beg to differ. For low level code, you really ought to have Arm 
> folks to confirm this is correct. In fact, I don't think patch #3 it 
> is. So ...
>
>>>
>>> And at this point at rc4, you'll need to persuade Oleksii to take it 
>>> for
>>> 4.20.
>>>
>>> Personally I think it's low risk and worthwhile to take for 4.20, 
>>> and it
>>> was technically completed in time - it just fell between the cracks.
>>
>> I think the same it's low risk patch series, so we can take it for 4.20:
>>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>
>
> ... I should not go to 4.20 as-is.
>
> And before someone ask why it wasn't answered early. I can't comment 
> for the other Arm maintainers, but I have been away for the past two 
> months. So still catching up on my emails.

Agree, I wrote that in follow-up reply to my initial reply.

So if the proper Ack will be received I still think we can consider to have it in 4.20.

~ Oleksii

>