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

2022-03-17 Thread Andrew Cooper
On 17/03/2022 14:21, Jan Beulich wrote:
> On 17.03.2022 15:06, 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 extend
>> check-endbr.sh to look for it.  The same logic can check for the absence of
>> any endbr32 instructions, so include a check for those too.
>>
>> The choice of nop has some complicated consequences.  nopw (%rax) has a ModRM
>> byte of 0, which the Bourne compatible shells unconditionally strip from
>> parameters, meaning that we can't pass it to `grep -aob`.
>>
>> Therefore, use nopw (%rcx) so the ModRM byte becomes 1.
>>
>> This then demonstrates another bug.  Under perl regexes, \1 thru \9 are
>> subpattern matches, and not octal escapes, while the behaviour of \10 and
>> higher depend on the number of capture groups.  Switch the `grep -P` runes to
>> use hex escapes instead, which are unambiguous
>>
>> The build time check then requires that the endbr64 poison have the same
>> treatment as endbr64 to avoid placing the byte pattern in immediate operands.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 

Thanks.

> with one nit (which likely I should have spotted before):

Unlikely, seeing as that was part that I rewrote between v2 and v3.

Will fix.

~Andrew


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

2022-03-17 Thread Jan Beulich
On 17.03.2022 15:06, 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 extend
> check-endbr.sh to look for it.  The same logic can check for the absence of
> any endbr32 instructions, so include a check for those too.
> 
> The choice of nop has some complicated consequences.  nopw (%rax) has a ModRM
> byte of 0, which the Bourne compatible shells unconditionally strip from
> parameters, meaning that we can't pass it to `grep -aob`.
> 
> Therefore, use nopw (%rcx) so the ModRM byte becomes 1.
> 
> This then demonstrates another bug.  Under perl regexes, \1 thru \9 are
> subpattern matches, and not octal escapes, while the behaviour of \10 and
> higher depend on the number of capture groups.  Switch the `grep -P` runes to
> use hex escapes instead, which are unambiguous
> 
> The build time check then requires that the endbr64 poison have the same
> treatment as endbr64 to avoid placing the byte pattern in immediate operands.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with one nit (which likely I should have spotted before):

> @@ -45,13 +45,15 @@ echo "X" | grep -aobP "\130" -q 2>/dev/null || 
> perl_re=false
>  ${OBJDUMP} -j .text $1 -d -w | grep 'endbr64 *$' | cut -f 1 -d ':' > 
> $VALID &
>  
>  #
> -# Second, look for any endbr64 byte sequence
> +# Second, look for all endbr64, endbr32 and nop poison byte sequences
>  # This has a couple of complications:
>  #
>  # 1) Grep binary search isn't VMA aware.  Copy .text out as binary, causing
>  #the grep offset to be from the start of .text.
>  #
>  # 2) dash's printf doesn't understand hex escapes, hence the use of octal.
> +#`grep -P` on the other hand can has various ambiguities with octal-like
> +#escapes, so use hex escapes instead which are unambiguous.

There looks to be a stray "can" in here.

Jan