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

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

[Greg KH]

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.

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
		ANNOTATE_UNRET_SAFE
		RET
	.endr

So should the "ret" remain or be turned into "RET" in mainline right
now?

thanks,

greg k-h

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

[Boris Ostrovsky]

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


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.


-boris

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

[Greg KH]

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.
> > 
> > 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
> > 		ANNOTATE_UNRET_SAFE
> > 		RET
> > 	.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.
> 
> 
> 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"?

I don't run any Xen systems, so I can't test any of this myself.

thanks,

greg k-h

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

[Boris Ostrovsky]

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

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

[Nicolai Stange]

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.

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

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

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

[Boris Ostrovsky]

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

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 5518 bytes --]

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

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Boris Ostrovsky]

On 7/17/22 1:20 AM, Juergen Gross wrote:
>
> 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?
>

This is all about objtool, that's why 'ret' was added there originally by f4b4bc10b0b8 ("x86/xen: Support objtool vmlinux.o validation in xen-head.S").


Before that it was all 'nop' which is similar to what you are suggesting ('int3' or 'ud2' would of course be better)


-boris