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