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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 08/03/2022 15:36, Jan Beulich wrote:
> 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.

We've got ~0 risk of having any embedded ENDBR32's because we never
refer to the opcode, and therefore adding 2x 0.7s delay to scan the
binary on build is a waste.  If the check were free, it would be a
different matter.

At any point, if we were to introduce references to ENDBR32, we'd want
to start scanning for embedded sequences.

But at no point do we want to intentionally remove our defence in depth
created by both having no CS32 code segment, and no ENDBR32 instructions.

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

Ah yes - I think I did misinterpret what you wrote.  I hope everything
is clear now.

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

Thanks.

~Andrew