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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ed82e54e-ccc4-f514-7018-8410d0f5bb82@oracle.com>
Date:   Sat, 16 Jul 2022 18:47:05 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Nicolai Stange <nstange@...e.de>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, xen-devel@...ts.xenproject.org,
        linux-kernel@...r.kernel.org, jpoimboe@...hat.com,
        Ben Hutchings <ben@...adent.org.uk>
Subject: Re: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports



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