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

[Andrew Cooper <amc96@xxxxxxxx>]

On 26/11/2021 13:22, Jan Beulich wrote:
> On 26.11.2021 14:13, Andrew Cooper wrote:
>> On 26/11/2021 12:48, Jan Beulich wrote:
>>> On 26.11.2021 13:33, Andrew Cooper wrote:
>>>>   * 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).

[Answering out of order]
>> 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.
> We don't, and the tool chain doesn't either. Only canonical NOPs (opcode
> 0x1F) are to be used there, as all others may gain a meaning beyond
> plain NOP.

Good.  Presuming that this continues to be true, the "endbr64 bridging
two instructions" looks like:

F3 0F 1E FA - real endbr64
0F 1E FA - Not emitted by toolchains
1E xx - push %ds which is #UD in 64bit
FA - cli

So local_irq_{save,disable}() need to be a little wary, but this is far
more constrained than I was anticipating.

> A disp alone won't do in general, as the top byte will only ever be 0x00
> or 0xFF (as long as our binary image doesn't go beyond 16Mb).

Tangent... I thought I'd lifted all the 16M restrictions when I rewrote
the pagetable handling, but the linker assert is still present so
clearly something is still hanging around.

For a call/jump disp32, 0xF30F1EFA is nearly -2G so we're not in any
danger of encountering that, given the 1G upper limit on .text/.data/etc.

However, disp32s on memory operands are effectively arbitrary, and there
are tricks like:

    incl  ASM_PERFC_exceptions * 4(%rcx, %rax, 4)

where the disp32 field isn't even a "usual" offset.

> But a
> ModR/M or SIB byte could start such a sequence, with only two or three
> of the (lower) disp bytes used to complete the pattern.

Luckily, a ModRM of F3 is a reg/reg encoding (ebx and esi), with no SIB
byte, so there is no ModRM=F3, SIB=0F case to worry about.

That leaves:

1) ModRM=F3 with 0F 1E FA in imm32, or
2) ModRM=F3 with 0F 1E in imm16 and a trailing CLI instruction, or
3) SIB=F3, an (%rbx, %rsi, 8)-ish operand with 0F 1E FA coming from imm,
disp or the following instruction.

These look to have rather more wiggle room, but still don't look as if
they'd be common to encounter.

Perhaps the two most worrying areas are imm64 constants, and the
div-by-constant reciprocal that tends to yield a large imm32 for use
with mul.


Given that Marek has kindly hacked us up a check which should find any
arbitrary violations, and on a small sample of builds, there are no
violations, I suggest that we clean it up and put it as a check in the
real build and enable it by default seeing as we're right at the start
of the 4.17 dev window.

If it is seen to trip (and it might well not), we can judge at that
point whether to rearrange the code to avoid it, or drop the check. 
Until then however, it gives us a very strong security statement.

~Andrew