[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: Build warnings in Xen 5.15.y and 5.10.y with retbleed backports
On 17.07.22 00:47, Boris Ostrovsky wrote: 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 instructionin 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) What about filling the complete hypercall page just with "int 3" or "ud2"? Any try to do a hypercall before the hypercall page has been initialized is a bug anyway. What good can come from calling a function which will return a basically random value instead of doing a privileged operation? Juergen Attachment:
OpenPGP_0xB0DE9DD628BF132F.asc Attachment:
OpenPGP_signature
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |