Re: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxx
>
> > > 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