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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2.1 6.5/70] x86/kexec: Annotate embedded data with ELF metadata
Date: Thu, 17 Feb 2022 15:48:07 +0100	[thread overview]
Message-ID: <e35ca83f-12ad-0c28-075b-82ff08b2bf1d@suse.com> (raw)
In-Reply-To: <826c97dd-dda9-b6c2-37bf-451354fcfde2@citrix.com>

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@suse.com>

>>>   ffff82d040396108:       00 00                   add    %al,(%rax)
>>>   ffff82d04039610a:       00 00                   add    %al,(%rax)
>>>   ffff82d04039610c:       10 00                   adc    %al,(%rax)
>>>
>>>   ffff82d04039610e <compat_mode_gdt_desc>:
>>>   ffff82d04039610e:       17                      (bad)
>>>           ...
>>>
>>>   ffff82d040396118 <compat_mode_gdt>:
>>>           ...
>>>   ffff82d040396120:       ff                      (bad)
>>>   ffff82d040396121:       ff 00                   incl   (%rax)
>>>   ffff82d040396123:       00 00                   add    %al,(%rax)
>>>   ffff82d040396125:       93                      xchg   %eax,%ebx
>>>   ffff82d040396126:       cf                      iret
>>>   ffff82d040396127:       00 ff                   add    %bh,%bh
>>>   ffff82d040396129:       ff 00                   incl   (%rax)
>>>   ffff82d04039612b:       00 00                   add    %al,(%rax)
>>>   ffff82d04039612d:       9b                      fwait
>>>   ffff82d04039612e:       cf                      iret
>>>           ...
>>>
>>>   ffff82d040396130 <compat_mode_idt>:
>>>           ...
>>>
>>>   ffff82d0403961b6 <kexec_reloc_size>:
>>>   ffff82d0403961b6:       b6 01                   mov    $0x1,%dh
>>>           ...
>>>
>>> to:
>>>
>>>   ffff82d040396108 <compatibility_mode_far>:
>>>   ffff82d040396108:       00 00 00 00 10 00                               ......
>>>
>>>   ffff82d04039610e <compat_mode_gdt_desc>:
>>>   ffff82d04039610e:       17 00 00 00 00 00 00 00 00 00                   ..........
>>>
>>>   ffff82d040396118 <compat_mode_gdt>:
>>>           ...
>>>   ffff82d040396120:       ff ff 00 00 00 93 cf 00 ff ff 00 00 00 9b cf 00 ................
>>>
>>>   ffff82d040396130 <compat_mode_idt>:
>>>   ffff82d040396130:       00 00 00 00 00 00                               ......
>> With the .size directives added, can we rely on consistent (past,
>> present, and future) objcopy behavior for padding gaps?
> 
> Of course not.  We don't know how things will develop in the future. 
> The best we can do is hope that it doesn't change too much.
> 
> But on that note, the way this would go wrong is the binary scan finding
> an endbr that wasn't disassembled properly here, for whatever reason.

True; it'll "just" be a false positive build failure.

>>  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?

> 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?

Jan