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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

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

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

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

>
>> @@ -175,10 +175,16 @@ compatibility_mode_far:
>>          .long 0x00000000             /* set in call_32_bit above */
>>          .word 0x0010
>>  
>> +        .type compatibility_mode_far, @object
>> +        .size compatibility_mode_far, . - compatibility_mode_far
>> +
>>  compat_mode_gdt_desc:
>>          .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
>>          .quad 0x0000000000000000     /* set in call_32_bit above */
>>  
>> +        .type compat_mode_gdt_desc, @object
>> +        .size compat_mode_gdt_desc, . - compat_mode_gdt_desc
> Side note: We really ought to gain something like OBJECT(name) to avoid
> c'n'p mistakes not updating correctly all three symbol name instances.

I've got an intern working on it.

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

The sole user is the memcpy() size calculation moving the trampoline to
its destination page in the kexec reserved area.

~Andrew