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: Thu, 17 Mar 2022 12:07:31 +0000	[thread overview]
Message-ID: <9555a8ef-6b44-9a39-78ca-f6bb4fc4528a@citrix.com> (raw)
In-Reply-To: <db1e9af9-5441-c035-1998-33be07f6e908@suse.com>

On 17/03/2022 10:43, Jan Beulich wrote:
> On 17.03.2022 11:02, 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 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`.
> Urgh. But as per my earlier comments I'm happier with ...
>
>> Therefore, use nopw (%rcx) so the ModRM byte becomes 1.
> ... a non-zero ModR/M byte anyway.
>
>> This then demonstrates another bug.  Under perl regexes, \1 thru \9 are
>> subpattern matches, and not octal escapes.  Switch the `grep -P` runes to use
>> hex escapes instead.
>>
>> 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 <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> Jan: As you had the buggy grep, can you please confirm that hex escapes work.
> (Build-)Tested-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> When working out the workaround, I actually did test with hex, but
> then switched to octal to make easily visible that the two patterns
> actually match. I also did wonder about octal and sub-pattern
> matching conflicting, but the grep I used definitely didn't have
> an issue there. Hence I assume grep behavior changed at some point;
> I wonder how they mean to have octal expressed now.

$ LC_ALL=C grep -aobP '\363\17\36\372|\146\17\37\1' text.bin
grep: reference to non-existent subpattern

> https://perldoc.perl.org/perlre specifically outlines how the
> conflict is dealt with - assuming you have observed grep to misbehave,
> I wonder whether they've accumulated a bug. (The doc also makes clear
> that such references aren't limited to single digit numbers; you may
> want to adjust your description in this regard.)

That part of the doc does say that the dynamic interpretation is only
for \10 and higher, so I don't think this is a bug.  \1 use to
exclusively mean the first capture group, not an octal character, and
this behaviour remains.

> Depending on how exactly your grep behaves, switching to always-three-
> digit octal escapes may be an alternative to retain the property of
> making obvious the similarity between the two pattern representations.

\01 and \001 do both work properly, but the non-ambiguous forms are \o1
or \o001.

Overall, I think it's better to stay with the hex escapes, because
they're also non-ambiguous.  The mix of octal and hex is irritating, but
the comments are very clear about what we're searching for.

And on that note, I realise we can also scan for endbr32 in exactly the
same way for no extra cost.  I'll fold that in too, seeing as the
discussion has come up before, and post a v3.

~Andrew