Re: [PATCH v3 2/3] xen: common: add ability to enable stack protector

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2024 01:52, Andrew Cooper wrote:
> On 11/12/2024 8:16 am, Jan Beulich wrote:
>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -432,7 +432,11 @@ else
>>>  CFLAGS_UBSAN :=
>>>  endif
>>>  
>>> +ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>> +CFLAGS += -fstack-protector
>>> +else
>>>  CFLAGS += -fno-stack-protector
>>> +endif
>> Personally I'd prefer if we consistently used the list approach we use
>> in various places, whenever possible:
>>
>> CFLAGS-stack-protector-y := -fno-stack-protector
>> CFLAGS-stack-protector-$(CONFIG_STACK_PROTECTOR) := -fstack-protector
>> CFLAGS += $(CFLAGS-stack-protector-y)
> 
> No - please stop this antipattern.
> 
> It saves 2 lines of code and makes the logic complete unintelligible.
> 
> I have a very strong preference for this patch to happen as Volodymyr
> presented, and without the double := replacing the more-legible ifeq.

Why "antipattern"? Surely there are cases where the list approach is
preferable. Surely there are cases where it ends up less legible, and
this may indeed be one such case. Yet then - where do you suggest to
draw the boundary?

>>> +void asmlinkage __stack_chk_fail(void)
>>> +{
>>> +    panic("Stack Protector integrity violation identified in %ps\n",
>>> +     __builtin_return_address(0));
>> Again.
>>
>> Is panic() really the right construct to use here, though?
>> __builtin_return_address() will merely identify the immediate caller. A
>> full stack trace (from BUG()) would likely be more useful in identifying
>> the offender.
> 
> Well - we have to be careful, because the backtrace from here is
> specifically misleading in this case.
> 
> When this trips, it's either the caller itself that broke, or some
> sibling call tree which is rubble under the active stack now.
> 
> BUG() also comes with 0 information.

Not quite, the more that you suggest an alternative way to ...

> So, maybe we want a dump_execution_state() (to get the backtrace), and
> then this panic() which states it was a Stack Protection violation,
> which hopefully is enough of a hint to people to look in the sibling
> call tree.

... get register state and stack trace out. Which I'd be entirely fine
with.

Jan