Re: [PATCH] x86/cet: Remove writeable mapping of the BSPs shadow stack

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 17/03/2022 16:28, Jan Beulich wrote:
> On 17.03.2022 17:19, Andrew Cooper wrote:
>> On 17/03/2022 09:17, Jan Beulich wrote:
>>> On 16.03.2022 20:23, Andrew Cooper wrote:
>>>> On 16/03/2022 08:33, Jan Beulich wrote:
>>>>> On 15.03.2022 17:53, Andrew Cooper wrote:
>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>> @@ -215,8 +215,9 @@ SECTIONS
>>>>>>    } PHDR(text)
>>>>>>    DECL_SECTION(.init.data) {
>>>>>>  #endif
>>>>>> +       . = ALIGN(STACK_SIZE);
>>>>>> +       *(.init.bss.stack_aligned)
>>>>> No real need for the ALIGN() here - it's the contributions to the
>>>>> section which ought to come with proper alignment. Imo ALIGN()
>>>>> should only ever be there ahead of a symbol definition, as otherwise
>>>>> the symbol might not mark what it is intended to mark due to padding
>>>>> which might be inserted. See also 01fe4da6243b ("x86: force suitable
>>>>> alignment in sources rather than in linker script").
>>>>>
>>>>> Really we should consider using
>>>>>
>>>>>     *(SORT_BY_ALIGNMENT(.init.data .init.data.* .init.bss.*))
>>>>>
>>>>> While I can see your point against forcing sorting by alignment
>>>>> globally, this very argument doesn't apply here (at least until
>>>>> there appeared a way for the section attribute and -fdata-sections
>>>>> to actually interact, such that .init.* could also become per-
>>>>> function/object).
>>>>>
>>>>> Then again - this block of zeroes doesn't need to occupy space in
>>>>> the binary.
>>>> It already occupies space, because of mkelf32.
>>> Hmm, yes, and not just because of mkelf32: Since we munge everything
>>> in a single PT_LOAD segment in the linker script, all of .init.*
>>> necessarily has space allocated.
>>>
>>>>>  It could very well live in a @nobits .init.bss in the
>>>>> final ELF binary. But sadly the section isn't @nobits in the object
>>>>> file, and with that there would need to be a way to make the linker
>>>>> convert it to @nobits (and I'm unaware of such). What would work is
>>>>> naming the section .bss.init.stack_aligned (or e.g.
>>>>> .bss..init.stack_aligned to make it easier to separate it from
>>>>> .bss.* in the linker script) - that'll make gcc mark it @nobits.
>>>> Living in .bss would prevent it from being reclaimed.  We've got several
>>>> nasty bugs from shooting holes in the Xen image, and too many special
>>>> cases already.
>>> I didn't suggest to put it in .bss; the suggested name was merely so
>>> that gcc would mark the section @nobits and we could exclude the
>>> section from what makes in into .bss in the final image independent
>>> of .init.* vs .bss.* ordering in the linker script. But anyway - with
>>> the above this aspect is now moot anyway.
>> So can I take this as an ack with the .init typo fixed?
> R-b, yes, as long as the ALIGN(STACK_SIZE) is also dropped and the
> other ALIGN() is retained (the latter you did already indicate you
> would do).

Ah yes.  Thanks.

~Andrew