Re: [PATCH] x86/cet: Use dedicated NOP4 for cf_clobber

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>, Wei Liu <wl@xen.org>,
	"Bjoern Doebel" <doebel@amazon.de>, Michael Kurth <mku@amazon.de>,
	Martin Pohlack <mpohlack@amazon.de>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/cet: Use dedicated NOP4 for cf_clobber
Date: Tue, 8 Mar 2022 15:19:52 +0000	[thread overview]
Message-ID: <476a25f8-86eb-0df5-b481-fc4cd5ecbb18@citrix.com> (raw)
In-Reply-To: <18fb4115-94d8-16c2-e39b-1be895e254f4@suse.com>

On 08/03/2022 14:37, Jan Beulich wrote:
> On 08.03.2022 15:01, Andrew Cooper wrote:
>> For livepatching, we need to look at a potentially clobbered function and
>> determine whether it used to have an ENDBR64 instruction.
>>
>> Use a non-default 4-byte P6 long nop, not emitted by toolchains, and introduce
>> the was_endbr64() predicate.
> Did you consider using ENDBR32 for this purpose?

No, and no because that's very short sighted.  Even 4 non-nops would be
better than ENDBR32, because they wouldn't create actually-usable
codepaths in corner cases we care to exclude.

> I'm worried that
> the pattern you picked is still too close to what might be used
> (down the road) by a tool chain.

This is what Linux are doing too, but no - I'm not worried.  The
encoding isn't the only protection; toolchains also have no reason to
put a nop4 at the head of functions; nop5 is the common one to find.

> One neat thing about ENDBR32 would be that you wouldn't even
> need memcpy() - you'd merely flip the low main opcode bit.

Not relevant.  You're taking the SMC pipeline hit for any sized write,
and a single movl is far less cryptic.

>> Bjoern: For the livepatching code, I think you want:
>>
>>   if ( is_endbr64(...) || was_endbr64(...) )
>>       needed += ENDBR64_LEN;
> Looks like I didn't fully understand the problem then from your
> initial description. The adjustment here (and the one needed in
> Björn's patch) is to compensate for the advancing of the
> targets of altcalls past the ENDBR?

No.  Consider this scenario:

callee:
    endbr64
    ...

altcall:
    call *foo(%rip)

During boot, we rewrite altcall to be `call callee+4` and turn endbr64
into nops, so it now looks like:

callee:
    nop4
    ...

altcall:
    call callee+4

Then we want to livepatch callee to callee_new, so we get

callee_new:
    endbr64
    ...

in the livepatch itself.

Now, to actually patch, we need to memcpy(callee+4, "jmp callee_new", 5).

The livepatch logic calling is_endbr(callee) doesn't work because it's
now a nop4, which is why it needs a was_endbr64(callee) too.

>
>> --- a/xen/arch/x86/include/asm/endbr.h
>> +++ b/xen/arch/x86/include/asm/endbr.h
>> @@ -52,4 +52,16 @@ static inline void place_endbr64(void *ptr)
>>      *(uint32_t *)ptr = gen_endbr64();
>>  }
>>  
>> +/*
>> + * After clobbering ENDBR64, we may need to confirm that the site used to
>> + * contain an ENDBR64 instruction.  Use an encoding which isn't the default
>> + * P6_NOP4.
>> + */
>> +#define ENDBR64_POISON "\x66\x0f\x1f\x00" /* osp nopl (%rax) */
> In case this remains as is - did you mean "opsz" instead of "osp"?
> But this really is "nopw (%rax)" anyway.

Oh, osp is the nasm name.  I'll switch to nopw.

~Andrew