RE: [RFC PATCH v2 20/21] x86: Add support for CONFIG_CFI_CLANG

David Laight
2022-05-16T21:32:58Z

From: Sami Tolvanen
> Sent: 16 May 2022 17:39
>
> On Mon, May 16, 2022 at 1:32 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Sami Tolvanen
> > > Sent: 13 May 2022 21:22
> > >
> > > With CONFIG_CFI_CLANG, the compiler injects a type preamble
> > > immediately before each function and a check to validate the target
> > > function type before indirect calls:
> > >
> > > ; type preamble
> > > __cfi_function:
> > > int3
> > > int3
> > > mov <id>, %eax
> >
> > Interesting - since this code can't be executed there is no
> > point adding an instruction 'prefix' to the 32bit constant.
>
> The reason to embed the type into an instruction is to avoid the need
> to special case objtool's instruction decoder.
>
> > > int3
> > > int3
> > > function:
> > > ...
> > > ; indirect call check
> > > cmpl <id>, -6(%r11)
> > > je .Ltmp1
> > > ud2
> > > .Ltmp1:
> > > call __x86_indirect_thunk_r11
> > >
> > > Define the __CFI_TYPE helper macro for manual type annotations in
> > > assembly code, add error handling for the CFI ud2 traps, and allow
> > > CONFIG_CFI_CLANG to be selected on x86_64.
> > >
> > ...
> > > +
> > > + /*
> > > + * The compiler generates the following instruction sequence
> > > + * for indirect call checks:
> > > + *
> > > + * cmpl <id>, -6(%reg) ; 7 bytes
> >
> > If the <id> is between -128 and 127 then an 8bit constant
> > (sign extended) might be used.
> > Possibly the compiler forces the assembler to generate the
> > long form.
> >
> > There could also be a REX prefix.
> > That will break any code that tries to use %reg.
>
> The compiler always generates this specific instruction sequence.

Yes, but there are several ways to encode 'cmpl imm,-6(reg)'.
Firstly you can use '81 /7 imm32' or '83 /7 imm8' where imm8 is sign extended.
(the /7 1/7/index_reg for a signed 8 bit offset).
But that only works for the original 32bit registers.
For the 64bit r8 to r15 an extra REX prefix is required.
That makes the instruction 8 bytes (if it needs a full 32bit immediate).

So if the register is %r11 there is an extra REX byte.
If the <id> is a hash and happens to be between -128 and 127
then there are three less bytes.

So decoding from regs->ip - 0 isn't always going to give
you the start of the instruction.

>
> > > + * je .Ltmp1 ; 2 bytes
> > > + * ud2 ; <- addr
> > > + * .Ltmp1:
> > > + *
> > > + * Both the type and the target address can be decoded from the
> > > + * cmpl instruction.
> > > + */
> > > + if (copy_from_kernel_nofault(buffer, (void *)regs->ip - 9, MAX_INSN_SIZE))
> > > + return;
> > > + if (insn_decode_kernel(&insn, buffer))
> > > + return;
> > > + if (insn.opcode.value != 0x81 || X86_MODRM_REG(insn.modrm.value) != 7)
> > > + return;
> >
> > Since you are looking for a very specific opcode why bother
> > calling insn_decode_kernel() - just check for the required (masked)
> > byte values.
>
> Because I need to decode both the immediate value and the register
> from that instruction.
>
> > > +
> > > + *type = insn.immediate.value;
> > > +
> > > + offset = insn_get_modrm_rm_off(&insn, regs);
> >
> > Given the expected instruction, isn't that -6 ??
>
> No, this is the register offset.

Hmmm.... strange function name...
>
> > > + if (offset < 0)
> > > + return;
> > > +
> > > + *target = *(unsigned long *)((void *)regs + offset);
> >
> > WTF is that calculating??
>
> It's reading the register value from pt_regs.
>
> Sami

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)