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

[Nicolai Stange <nstange@xxxxxxx>]

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.

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.

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

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)