Re: [PATCH 00/65] x86: Support for CET Indirect Branch Tracking

[Andrew Cooper <amc96@xxxxxxxx>]

On 26/11/2021 12:48, Jan Beulich wrote:
> On 26.11.2021 13:33, Andrew Cooper wrote:
>> CET Indirect Branch Tracking is a hardware feature designed to protect 
>> against
>> forward-edge control flow hijacking (Call/Jump oriented programming), and is 
>> a
>> companion feature to CET Shadow Stacks added in Xen 4.14.
>>
>> This series depends on lots of previously posted patches.  See
>> xenbits/xen-cet-ibt for the full branch with all dependencies.
>>
>> Patch 1 introduces some compile time infrastructure.
>>
>> Patches 2 thru 56 annotate all function pointer targets in the common and x86
>> hypervisor code.  Patches are split by API and in no particular order, and
>> largely mechanical.  As such, I'm limiting review mainly to The Rest.  While
>> doing this work does depend on an experimental GCC change (patch 56), the
>> result does actually work properly with GCC 9 onwards.
> I wonder what this means. Are you talking about a gcc 9 with the experimental
> change backported?

No - plain GCC 9 as released (give or take the bug with retpoline which
was fixed in 9.4).  See patch 1.

This entire series, on GCC 9.4 or 10, will compile and function
correctly with CET-IBT active in hardware.

> Or are you saying that things build fine there (but don't
> work as far as IBT is concerned) in the absence of the experimental change?
> In which case what about older gcc?

The only thing the experimental change does is provide more
typechecking, so the compiler can identify when there is a call to a
non-ENDBR'd function.  See patch 56.

There is no possible way I could have done this work without the
experimental change, because there are far too many function pointers to
have found blind.

The typechecking isn't perfect, but it's pretty good.  In the short
term, we're going to have to be careful with new code, and I ought to
put something in Gitlab CI.  In the longer term, I hope for something
suitable to get into GCC 12.

That said, there are also a huge number of errors new in GCC 12 to do
with array bounds checks, and I'm not sure sprinkling more gcc11_wrap()
is going to work this time.

>> Various note accumulated through the work:
>>   * I have already posted patches fixing some of the most egregious (ab)uses 
>> of
>>     function pointers.  There are plenty of other areas which could do with
>>     cleanup.
>>   * With everything turned on, we get 1688 runtime endbr64's, and 233 init
>>     time.  The number of runtime endbr64's is expected to reduce with
>>     Juergen's hypercall series (see later), and in common deployment cases
>>     where not everything is compiled in by default.
>>   * I have not checked for misaligned endbr64's, and I'm not sure there is
>>     anything useful we could do upon discovering that there were any.
>>     Naively, there is a 1 in 2^32 chance (endbr64 being 4 bytes long), but
>>     this doesn't account for the structure of x86 code, which is most
>>     certainly not a uniform random distribution of bytes.
> Do you really mean "misaligned" here? The 2nd sentence rather might suggest
> that you mean byte sequences resembling ENDBR, despite actually being part
> of other insns. If so, checking might not allow to prove anything, as e.g.
> displacements change with about every build.

I do mean "any sequence of bytes resembling ENDBR", because that is
ultimately how the CPU instruction decode will behave.

And yes - you certainly can hide it in a 4-byte disp/imm, but it's an
incredibly rare imm32 to find (except for tasks such as in patch 64). 
You can also hide it in an disp/imm8 followed by a specific nopl, but
I'm not sure if we'd ever emit 0F 1E FA as a nopl by default.

~Andrew