Re: [PATCH v2.1 6.5/70] x86/kexec: Annotate embedded data with ELF metadata

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 17/02/2022 14:48, Jan Beulich wrote:
> On 17.02.2022 13:06, Andrew Cooper wrote:
>> On 17/02/2022 10:42, Jan Beulich wrote:
>>> On 17.02.2022 11:01, Andrew Cooper wrote:
>>>> Scanning for embedded endbranch instructions involves parsing the .text
>>>> disassembly.  Data in the kexec trampoline has no ELF metadata, so objdump
>>>> treats it as instructions and tries to disassemble.  Convert:
>>>>
>>>>   ffff82d040396108 <compatibility_mode_far>:
>>> What about the (possible) padding ahead of this? Should the .align
>>> there perhaps specify a filler character?
>> What about it?  It's just long nops like all other padding in .text
>>
>> ffff82d040396101:       ff d5                   call   *%ebp
>> ffff82d040396103:       0f 0b                   ud2    
>> ffff82d040396105:       0f 1f 00                nopl   (%eax)
>>
>> ffff82d040396108 <compatibility_mode_far>:
>> ffff82d040396108:       00 00 00 00 10
>> 00                                   ......
>>
>> And for this problem, we don't need to care about the behaviour of any
>> pre-CET version of binutils.
> I was about to ask, but yes - this is a good point.
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

>>>  It just so
>>> happens that there's no 4-byte gap between compat_mode_gdt_desc and
>>> compat_mode_gdt. Changing the .align ahead of compatibility_mode_far
>>> would eliminate the risk of padding appearing if the code further up
>>> changed.
>> Gaps will be formed of long nops because we're in .text, and they merge
>> with the previous data blob (see below).
>>
>>>>   ffff82d040396136 <reloc_stack>:
>>>>           ...
>>> Now this is particularly puzzling: Us setting %rsp to an unaligned
>>> address is clearly not ABI-conforming. Since you're fiddling with
>>> all of this already anyway, how about fixing this at the same time?
>>> Of course there would then appear padding ahead of the stack, unless
>>> the stack was moved up some.
>> Oh - I'd not even noticed that.  Luckily there is no ABI which matters,
>> because it's the call/push/pop's in this file alone.
> And the entity transitioned to is forbidden to make use of our stack?

There's no expectation/guarantee of a good stack, no.  Purgatory is a
very minimal environment before it sets something new up.

>> With an align 8, we get:
>>
>> ffff82d0403a7138 <compat_mode_idt>:
>> ffff82d0403a7138:       00 00 00 00 00 00 66
>> 90                             ......f.
>>
>> ffff82d0403a7140 <reloc_stack>:
>>         ...
>>
>> where the 66 90 in compat_mode_idt is the padding.  Recall c/s 9fd181540c7e6
>>
>>>> --- a/xen/arch/x86/xen.lds.S
>>>> +++ b/xen/arch/x86/xen.lds.S
>>>> @@ -87,6 +87,7 @@ SECTIONS
>>>>         *(.text.unlikely)
>>>>         *(.fixup)
>>>>         *(.text.kexec)
>>>> +       kexec_reloc_end = .;
>>> Does this maybe want aligning on a 4- or even 8-byte boundary? If
>>> so, imo preferably not here, but by adding a trailing .align in the
>>> .S file.
>> There's no special need for it to be aligned, and it is anyway as the
>> stack is the last object in it.
> You mean it anyway would be, if the stack was aligned? Or am I to imply
> that you've amended the patch to add alignment there?

I have aligned reloc_stack stack because that's a no-brainer.

With that suitably aligned, kexec_reloc_end becomes aligned naturally
(because reloc_stack is the final object), and I don't think there's
much point putting anything explicit in the linker script.

It doesn't matter if subsequent things follow immediately, because this
trampoline is copied into the kexec region before being used.  In
practice, the thing immediately following it is .init.text.

~Andrew