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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2022 16:19, Andrew Cooper wrote:
> 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.

Well - I thought of ENDBR32 because elsewhere you said we don't
need to be worried about any byte sequence resembling that insn,
since for it to become "usable" an attacker would first need to
have managed to manufacture a 32-bit ring0 code segment. Now you
say effectively the opposite.

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

Well, okay - let's hope for the best then.

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

Sounds like exactly what I was thinking of. Perhaps my description
wasn't sufficiently clear / unambiguous then.

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

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan