Re: [kvm-unit-tests PATCH v3 01/17] x86/run_in_user: Add an "end branch" marker on the user_mode destination

Sean Christopherson
2025-11-14T20:46:49Z

On Fri, Nov 14, 2025, Mathias Krause wrote:
> On 14.11.25 01:12, Sean Christopherson wrote:
> > Add an endbr64 at the user_mode "entry point" so that run_in_user() can be
> > used when CET's Indirect Branch Tracking is enabled.
>
> I don't think that's needed, as 'user_mode' is branched to via IRETQ and
> that isn't covered by IBT -- nor is any other RET instruction.
>
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > lib/x86/usermode.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
> > index c3ec0ad7..f4ba0af4 100644
> > --- a/lib/x86/usermode.c
> > +++ b/lib/x86/usermode.c
> > @@ -68,6 +68,9 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
> > "iretq\n"
> >
> > "user_mode:\n\t"
> > +#ifdef __x86_64__
> > + "endbr64\n\t"
> > +#endif
>
> The thing is, this ENDBR64 is actually masking a missing cleanup step in
> the IBT tests. The first failing IBT test will make the CET_U IBT
> tracker state get stuck in the WAIT_FOR_ENDBRANCH state. This means,
> every time we return to userland (and thereby implicitly switching to
> the CET_U state again), it wants to see an ENDBR64 first or it will
> directly trigger a #CP(3) again. This ENDBR64 will please that demand
> and make it transition to IDLE and allow executing the test. However,
> it's really the old test that should have fixed the tracker state and
> not a blanket ENDBR64 when entering usermode.
>
> Attached is a patch on top of [1] that does that but is, admitted, a
> little hacky and evolved. However, it shows that above ENDBR64 is, in
> fact, not needed.

You say hacky, I say clever and correct. :-)

> diff --git a/x86/cet.c b/x86/cet.c
> index 801d8da6e929..bcf1ca6d740a 100644
> --- a/x86/cet.c
> +++ b/x86/cet.c
> @@ -1,4 +1,3 @@
> -
> #include "libcflat.h"
> #include "x86/desc.h"
> #include "x86/processor.h"
> @@ -192,6 +191,10 @@ static uint64_t cet_ibt_emulation(void)
> #define CET_ENABLE_SHSTK BIT(0)
> #define CET_ENABLE_IBT BIT(2)
> #define CET_ENABLE_NOTRACK BIT(4)
> +#define CET_IBT_SUPPRESS BIT(10)
> +#define CET_IBT_TRACKER_STATE BIT(11)
> +#define IBT_TRACKER_IDLE 0
> +#define IBT_TRACKER_WAIT_FOR_ENDBRANCH BIT(11)

For this, I think it makes sense to diverge slightly from the SDM and just do

#define CET_IBT_TRACKER_WAIT_FOR_ENDBRANCH BIT(11)

because...

> static void test_shstk(void)
> {
> @@ -244,6 +247,22 @@ static void test_shstk(void)
> report(vector == GP_VECTOR, "MSR_IA32_PL3_SSP alignment test.");
> }
>
> +static void ibt_tracker_fixup(struct ex_regs *regs)
> +{
> + u64 cet_u = rdmsr(MSR_IA32_U_CET);
> +
> + /*
> + * Switch the IBT tracker state to IDLE to have a clean state for
> + * following tests.
> + */
> + if ((cet_u & CET_IBT_TRACKER_STATE) == IBT_TRACKER_WAIT_FOR_ENDBRANCH) {
> + cet_u &= ~IBT_TRACKER_WAIT_FOR_ENDBRANCH;

...this is quite weird/confusing. It relies on CET_IBT_TRACKER_STATE being a
single bit, and "(x & y) == z)" is very un-idiomatic for a single bit.

> + printf("CET: suppressing IBT WAIT_FOR_ENDBRANCH state at RIP: %lx\n",
> + regs->rip);
> + wrmsr(MSR_IA32_U_CET, cet_u);
> + }
> +}