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

[David Laight]

From: Sami Tolvanen
> Sent: 16 May 2022 17:39
> 
> On Mon, May 16, 2022 at 1:32 AM David Laight <David.Laight@xxxxxxxxxx> 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)