Messages in this thread |  | | Date | Sat, 16 Jul 2022 18:47:05 -0400 | Subject | Re: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports | From | Boris Ostrovsky <> |
| |
On 7/16/22 12:35 PM, Nicolai Stange wrote: > Hi, > > I see a patch for this has been queued up for 5.10 already ([1]), I'm > just sharing my findings in support of this patch here -- it doesn't > merely exchange one warning for another, but fixes a real issue and > should perhaps get applied to other stable branches as well. > > TL;DR: for this particular warning, objtool would exit early and fail to > create any .orc_unwind* ELF sections for head_64.o, which are consumed > by the ORC unwinder at runtime. > > > Boris Ostrovsky <boris.ostrovsky@oracle.com> writes: > >> On 7/12/22 3:31 PM, Greg KH wrote: >>> On Tue, Jul 12, 2022 at 03:19:39PM -0400, Boris Ostrovsky wrote: >>>> >>>> On 7/12/22 12:38 PM, Greg KH wrote: >>>>> Hi all, >>>>> >>>>> I'm seeing the following build warning: >>>>> arch/x86/kernel/head_64.o: warning: objtool: xen_hypercall_mmu_update(): can't find starting instruction >>>>> in the 5.15.y and 5.10.y retbleed backports. > > The reason for this is that with RET being multibyte, it can cross those > "xen_hypecall_*" symbol boundaries, because ... > >>>>> >>>>> I don't know why just this one hypercall is being called out by objtool, >>>>> and this warning isn't in 5.18 and Linus's tree due to I think commit >>>>> 5b2fc51576ef ("x86/ibt,xen: Sprinkle the ENDBR") being there. >>>>> >>>>> But, is this a ret call that we "forgot" here? It's a "real" ret in >>>>> Linus's branch: >>>>> >>>>> .pushsection .noinstr.text, "ax" >>>>> .balign PAGE_SIZE >>>>> SYM_CODE_START(hypercall_page) >>>>> .rept (PAGE_SIZE / 32) >>>>> UNWIND_HINT_FUNC >>>>> ANNOTATE_NOENDBR >>>>> ANNOTATE_UNRET_SAFE >>>>> ret >>>>> /* >>>>> * Xen will write the hypercall page, and sort out ENDBR. >>>>> */ >>>>> .skip 31, 0xcc >>>>> .endr >>>>> >>>>> while 5.15.y and older has: >>>>> .pushsection .text >>>>> .balign PAGE_SIZE >>>>> SYM_CODE_START(hypercall_page) >>>>> .rept (PAGE_SIZE / 32) >>>>> UNWIND_HINT_FUNC >>>>> .skip 31, 0x90 > > ... the "31" is no longer correct, ... > >>>>> ANNOTATE_UNRET_SAFE >>>>> RET > > ... as with RET occupying more than one byte, the resulting hypercall > entry's total size won't add up to 32 anymore.
Right! I haven't thought about that part. I think this has been broken since 14b476e07fab ("x86: Prepare asm files for straight-line-speculation").
It still shouldn't matter as far as correct execution is concerned which is probably why noone complained.
> > Note that those xen_hypercall_* symbols' values are getting statically > calculated as 'hypercall page + n * 32' in the HYPERCALL() #define from > xen-head.S. So there's a mismatch and with RET == 'ret; int3', the > resulting .text effectively becomes > > 101e: 90 nop > 101f: c3 ret > > 0000000000001020 <xen_hypercall_mmu_update>: > 1020: cc int3 > 1021: 90 nop > 1022: 90 nop > > > This is probably already not what has been intended, but because 'ret' > and 'int3' both are single-byte encoded, objtool would still be able to > find at least some "starting instruction" at this point. > > But with RET == 'jmp __x86_return_thunk', it becomes > > 101e: 90 nop > 101f: e9 .byte 0xe9 > > 0000000000001020 <xen_hypercall_mmu_update>: > 1020: 00 00 add %al,(%rax) > 1022: 00 00 add %al,(%rax) > 1024: 90 nop > > Here the 'e9 00 00 00 00' jmp crosses the symbol boundary and objtool > errors out. >
Ah, thanks for explanation.
Then I think we need to replace
.skip 31, 0x90
with something like
#if defined(CONFIG_RETHUNK) && !defined(__DISABLE_EXPORTS) && !defined(BUILD_VDSO) #define SKIP_BYTES 27 /* RET is 'jmp __x86_return_thunk' (5 bytes) */ #else /* CONFIG_RETPOLINE */ #ifdef CONFIG_SLS #define SKIP_BYTES 30 /* RET is 'ret; int3' (2 bytes) */ #else #define SKIP_BYTES 31 /* RET is 'ret' (1 byte) */ #endif .skip SKIP_BYTES, 0x90
(I don't have patched 5.15 so I am going by what mainline looks like)
Or replace RET with ret. (Although at least with unpatched 5.15 the warning below is still generated)
-boris
>>>>> .endr >>>>> >>>>> So should the "ret" remain or be turned into "RET" in mainline right >>>>> now? >>>> >>>> >>>> It doesn't matter --- this is overwritten by the hypervisor during >>>> initialization when Xen fills in actual hypercall code. > > It does makes a difference though: even though objtool reports only a > warning, it still exits early in this particular case and won't create > any of the .orc_unwind* or .return_sites sections for head_64.o as it's > supposed to. > > The significance of not having .orc_unwind* for head_64.o is that the > reliable stacktracing implementation would mark the swapper tasks' > stacktraces as unreliable at runtime, because the ORC unwinder would > fail to recognize their final secondary_startup_64() from head_64.o as > being the end. Note that livepatching relies on reliable stacktraces > when transitioning tasks. > > >>>> >>>> >>>> So f4b4bc10b0b85ec66f1a9bf5dddf475e6695b6d2 added 'ret' to make objtool happy and then 14b476e07fab6 replaced 'ret' with RET as part of SLS fixes. The latter was not really necessary but harmless. >>>> >>>> >>>> So it can be 'ret', RET, or anything else that tools don't complain about. It will not be executed. >>> Cool, thanks. >>> But what about the objtool warning that I now see? Is that "real"? >> >> >> >> It's not real in the sense that the code there is not real, it will be overwritten. (Originally the whole page was 'nop's) >> >> >> I am getting a different error BTW: >> >> arch/x86/kernel/head_64.o: warning: objtool: .text+0x5: unreachable instruction >> > > I think this one is (mostly?) harmless, at least as as far as the > .orc_unwind* generation is concerned. Josh would know more. > > > Thanks, > > Nicolai > > [1] https://lore.kernel.org/r/Ys+8ZYxkDmSCcDWv@kroah.com > >> >> >>> I don't run any Xen systems, so I can't test any of this myself. >> >> >> You can't test any changes to that code --- it is rewritten when Xen guest is running. >> >> >> We probably do want to shut up objtool. Josh, any suggestions? >> >> >> -boris >> >
|  |