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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan