[v2,06/25] x86/xen: Add ANNOTATE_ENDBR to startup_xen()

Commit Message

Josh Poimboeuf April 18, 2022, 4:50 p.m. UTC

The startup_xen() kernel entry point is referenced by the ".note.Xen"
section, but is presumably not indirect-branched to.  Add ANNOTATE_ENDBR
to silence future objtool warnings.

Cc: Boris Ostrovsky <[e-mail]>
Cc: Juergen Gross <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: [e-mail]
Signed-off-by: Josh Poimboeuf <[e-mail]>
---
 arch/x86/xen/xen-head.S | 1 +
 1 file changed, 1 insertion(+)

Comments

Andrew Cooper April 19, 2022, 11:42 a.m. UTC | #1

On 18/04/2022 17:50, Josh Poimboeuf wrote:
> The startup_xen() kernel entry point is referenced by the ".note.Xen"
> section, but is presumably not indirect-branched to.

It's the real entrypoint of the VM.  It's "got to" by setting %rip
during vcpu setup.

We could in principle support starting a PV VM with CET active, but that
sounds like an enormous quantity of effort for very little gain.  CET
for Xen PV requires paravirt anyway (because the kernel runs in CPL!=0)
so decisions like this can wait until someone feels like doing the work.

>   Add ANNOTATE_ENDBR
> to silence future objtool warnings.
>
> Cc: Boris Ostrovsky <[e-mail]>
> Cc: Juergen Gross <[e-mail]>
> Cc: Stefano Stabellini <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Josh Poimboeuf <[e-mail]>

FWIW, Reviewed-by: Andrew Cooper <[e-mail]>, preferably
with the commit message tweaked to remove the uncertainty.

~Andrew

Peter Zijlstra April 19, 2022, 11:57 a.m. UTC | #2

On Tue, Apr 19, 2022 at 11:42:12AM +0000, Andrew Cooper wrote:
> On 18/04/2022 17:50, Josh Poimboeuf wrote:
> > The startup_xen() kernel entry point is referenced by the ".note.Xen"
> > section, but is presumably not indirect-branched to.
> 
> It's the real entrypoint of the VM.  It's "got to" by setting %rip
> during vcpu setup.
> 
> We could in principle support starting a PV VM with CET active, but that
> sounds like an enormous quantity of effort for very little gain.  CET
> for Xen PV requires paravirt anyway (because the kernel runs in CPL!=0)
> so decisions like this can wait until someone feels like doing the work.
> 
> >   Add ANNOTATE_ENDBR
> > to silence future objtool warnings.
> >
> > Cc: Boris Ostrovsky <[e-mail]>
> > Cc: Juergen Gross <[e-mail]>
> > Cc: Stefano Stabellini <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Josh Poimboeuf <[e-mail]>
> 
> FWIW, Reviewed-by: Andrew Cooper <[e-mail]>, preferably
> with the commit message tweaked to remove the uncertainty.

Something like so then?

---
Subject: x86/xen: Add ANNOTATE_ENDBR to startup_xen()
From: Josh Poimboeuf <[e-mail]>
Date: Mon, 18 Apr 2022 09:50:25 -0700

From: Josh Poimboeuf <[e-mail]>

The startup_xen() kernel entry point is referenced by the ".note.Xen"
section, and is the real entry point of the VM. It *will* be
indirectly branched to, *however* currently Xen doesn't support PV VM
with CET active.

Add ANNOTATE_ENDBR to silence future objtool warnings.

Fixes: ed53a0d97192 ("x86/alternative: Use .ibt_endbr_seal to seal indirect calls")
Signed-off-by: Josh Poimboeuf <[e-mail]>
Signed-off-by: Peter Zijlstra (Intel) <[e-mail]>
Reviewed-by: Andrew Cooper <[e-mail]>
Link: https://lkml.kernel.org/r/[e-mail]

Jürgen Groß April 19, 2022, 12:06 p.m. UTC | #3

On 19.04.22 13:57, Peter Zijlstra wrote:
> On Tue, Apr 19, 2022 at 11:42:12AM +0000, Andrew Cooper wrote:
>> On 18/04/2022 17:50, Josh Poimboeuf wrote:
>>> The startup_xen() kernel entry point is referenced by the ".note.Xen"
>>> section, but is presumably not indirect-branched to.
>>
>> It's the real entrypoint of the VM.  It's "got to" by setting %rip
>> during vcpu setup.
>>
>> We could in principle support starting a PV VM with CET active, but that
>> sounds like an enormous quantity of effort for very little gain.  CET
>> for Xen PV requires paravirt anyway (because the kernel runs in CPL!=0)
>> so decisions like this can wait until someone feels like doing the work.
>>
>>>    Add ANNOTATE_ENDBR
>>> to silence future objtool warnings.
>>>
>>> Cc: Boris Ostrovsky <[e-mail]>
>>> Cc: Juergen Gross <[e-mail]>
>>> Cc: Stefano Stabellini <[e-mail]>
>>> Cc: [e-mail]
>>> Signed-off-by: Josh Poimboeuf <[e-mail]>
>>
>> FWIW, Reviewed-by: Andrew Cooper <[e-mail]>, preferably
>> with the commit message tweaked to remove the uncertainty.
> 
> Something like so then?
> 
> ---
> Subject: x86/xen: Add ANNOTATE_ENDBR to startup_xen()
> From: Josh Poimboeuf <[e-mail]>
> Date: Mon, 18 Apr 2022 09:50:25 -0700
> 
> From: Josh Poimboeuf <[e-mail]>
> 
> The startup_xen() kernel entry point is referenced by the ".note.Xen"
> section, and is the real entry point of the VM. It *will* be
> indirectly branched to, *however* currently Xen doesn't support PV VM
> with CET active.

Hmm, Xen will always use IRET to activate the guest.


Juergen

Andrew Cooper April 19, 2022, 12:12 p.m. UTC | #4

On 19/04/2022 12:57, Peter Zijlstra wrote:
> On Tue, Apr 19, 2022 at 11:42:12AM +0000, Andrew Cooper wrote:
>> On 18/04/2022 17:50, Josh Poimboeuf wrote:
>>> The startup_xen() kernel entry point is referenced by the ".note.Xen"
>>> section, but is presumably not indirect-branched to.
>> It's the real entrypoint of the VM.  It's "got to" by setting %rip
>> during vcpu setup.
>>
>> We could in principle support starting a PV VM with CET active, but that
>> sounds like an enormous quantity of effort for very little gain.  CET
>> for Xen PV requires paravirt anyway (because the kernel runs in CPL!=0)
>> so decisions like this can wait until someone feels like doing the work.
>>
>>>   Add ANNOTATE_ENDBR
>>> to silence future objtool warnings.
>>>
>>> Cc: Boris Ostrovsky <[e-mail]>
>>> Cc: Juergen Gross <[e-mail]>
>>> Cc: Stefano Stabellini <[e-mail]>
>>> Cc: [e-mail]
>>> Signed-off-by: Josh Poimboeuf <[e-mail]>
>> FWIW, Reviewed-by: Andrew Cooper <[e-mail]>, preferably
>> with the commit message tweaked to remove the uncertainty.
> Something like so then?
>
> ---
> Subject: x86/xen: Add ANNOTATE_ENDBR to startup_xen()
> From: Josh Poimboeuf <[e-mail]>
> Date: Mon, 18 Apr 2022 09:50:25 -0700
>
> From: Josh Poimboeuf <[e-mail]>
>
> The startup_xen() kernel entry point is referenced by the ".note.Xen"
> section, and is the real entry point of the VM. It *will* be
> indirectly branched to, *however* currently Xen doesn't support PV VM
> with CET active.

Technically it's always IRET'd to, but the point is that it's never
"branched to" by the execution context of the VM.

So it would be better to say that it's never indirectly branched to. 
That's what the IBT checks care about.

>
> Add ANNOTATE_ENDBR to silence future objtool warnings.

Only just spotted.  All text in the subject and commit message needs
s/ENDBR/NOENDBR/

~Andrew

Peter Zijlstra April 19, 2022, 1:10 p.m. UTC | #5

On Tue, Apr 19, 2022 at 01:12:14PM +0100, Andrew Cooper wrote:

> > Subject: x86/xen: Add ANNOTATE_ENDBR to startup_xen()
> > From: Josh Poimboeuf <[e-mail]>
> > Date: Mon, 18 Apr 2022 09:50:25 -0700
> >
> > From: Josh Poimboeuf <[e-mail]>
> >
> > The startup_xen() kernel entry point is referenced by the ".note.Xen"
> > section, and is the real entry point of the VM. It *will* be
> > indirectly branched to, *however* currently Xen doesn't support PV VM
> > with CET active.
> 
> Technically it's always IRET'd to, but the point is that it's never
> "branched to" by the execution context of the VM.
> 
> So it would be better to say that it's never indirectly branched to. 
> That's what the IBT checks care about.

Right, so I was thinking the IRET could set the NEED_ENDBR bit, but
yeah, that might be stretching the definition of an indirect-branch a
wee bit.

How about so then?

---
Subject: x86/xen: Add ANNOTATE_NOENDBR to startup_xen()
From: Josh Poimboeuf <[e-mail]>
Date: Mon, 18 Apr 2022 09:50:25 -0700

From: Josh Poimboeuf <[e-mail]>

The startup_xen() kernel entry point is referenced by the ".note.Xen"
section, and is the real entry point of the VM. Control transfer is
through IRET, which *could* set NEED_ENDBR, however Xen currently does
no such thing.

Add ANNOTATE_NOENDBR to silence future objtool warnings.

Fixes: ed53a0d97192 ("x86/alternative: Use .ibt_endbr_seal to seal indirect calls")
Signed-off-by: Josh Poimboeuf <[e-mail]>
Signed-off-by: Peter Zijlstra (Intel) <[e-mail]>
Reviewed-by: Andrew Cooper <[e-mail]>
Link: https://lkml.kernel.org/r/[e-mail]

Andrew Cooper April 19, 2022, 2:25 p.m. UTC | #6

On 19/04/2022 14:10, Peter Zijlstra wrote:
> On Tue, Apr 19, 2022 at 01:12:14PM +0100, Andrew Cooper wrote:
>
>>> Subject: x86/xen: Add ANNOTATE_ENDBR to startup_xen()
>>> From: Josh Poimboeuf <[e-mail]>
>>> Date: Mon, 18 Apr 2022 09:50:25 -0700
>>>
>>> From: Josh Poimboeuf <[e-mail]>
>>>
>>> The startup_xen() kernel entry point is referenced by the ".note.Xen"
>>> section, and is the real entry point of the VM. It *will* be
>>> indirectly branched to, *however* currently Xen doesn't support PV VM
>>> with CET active.
>> Technically it's always IRET'd to, but the point is that it's never
>> "branched to" by the execution context of the VM.
>>
>> So it would be better to say that it's never indirectly branched to. 
>> That's what the IBT checks care about.
> Right, so I was thinking the IRET could set the NEED_ENDBR bit, but
> yeah, that might be stretching the definition of an indirect-branch a
> wee bit.
>
> How about so then?
>
> ---
> Subject: x86/xen: Add ANNOTATE_NOENDBR to startup_xen()
> From: Josh Poimboeuf <[e-mail]>
> Date: Mon, 18 Apr 2022 09:50:25 -0700
>
> From: Josh Poimboeuf <[e-mail]>
>
> The startup_xen() kernel entry point is referenced by the ".note.Xen"
> section, and is the real entry point of the VM. Control transfer is
> through IRET, which *could* set NEED_ENDBR, however Xen currently does
> no such thing.
>
> Add ANNOTATE_NOENDBR to silence future objtool warnings.
>
> Fixes: ed53a0d97192 ("x86/alternative: Use .ibt_endbr_seal to seal indirect calls")
> Signed-off-by: Josh Poimboeuf <[e-mail]>
> Signed-off-by: Peter Zijlstra (Intel) <[e-mail]>
> Reviewed-by: Andrew Cooper <[e-mail]>
> Link: https://lkml.kernel.org/r/[e-mail]

LGTM.

~Andrew

Patch

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ac17196e2518..3a2cd93bf059 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -45,6 +45,7 @@  SYM_CODE_END(hypercall_page)
 	__INIT
 SYM_CODE_START(startup_xen)
 	UNWIND_HINT_EMPTY
+	ANNOTATE_NOENDBR
 	cld
 
 	/* Clear .bss */