lkml.org 
[lkml]   [2022]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports
From


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

\
 
 \ /
  Last update: 2022-07-17 00:48    [W:0.210 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site