Re: [PATCH,V3 00/13] Synthesize CFI for hand-written asm

[Jan Beulich <jbeulich@suse.com>]

On 19.12.2023 22:02, Indu Bhagat wrote:
> On 12/18/23 00:47, Jan Beulich wrote:
>> While this is an unusual situation - new very general purpose insns aren't
>> introduced frequently -, I'd also like to see more formally addressed the
>> idea of ongoing support: From the original review I recall that you need
>> to minimally track insns altering GPRs, in order to avoid silently
>> generating bad CFI. Remember that I haven't looked at v3 yet, but as long
>> as that tracking is based on specific insns rather than a generalized
>> pattern, any ISA addition allowing GPRs to be altered would be at risk of
>> rendering the CFI generator code stale. Yet people, once they've started
>> to detect availability of this functionality, may validly expect that
>> their use of the functionality won't silently break behind their backs. In
>> this respect, did you consider constraining under what conditions the
>> generator code may actually come into play (at least for the time being)?
> 
> (Step 1) I propose that, for now, we add a check such that if any APX 
> insn is seen for --scfi invocation, we bail out.  IIUC, we could check 
> using the is_any_apx_rex2_encoding ().

That would cover only the REX2 subset of additions. The promoted-to-EVEX
insns would need covering as well.

> (Step 2a) We can remove this check once there is support for all APX 
> instructions for SCFI. I can add support for ginsns for APX instructions 
> once the APX work is pushed.
> 
> (Step 2b) Orthogonal to supporting the APX instruction set: For SCFI, it 
> is ideal to add a way to raise alert if new instructions are added in 
> the three categories (For APX, I see we have additions in #2, and #3):
> 
> 1. Control flow instructions
>     We can detect them by checking for insn.tm.opcode_modifier.jump.

We may certainly hope that any new control flow insns would also have
this attribute set.

This reminds me of another guard you may need, though: Use of .byte to
hand-craft insns.

> 2. Operations altering GPRs
>     We can detect them by checking for:
>     if (insn.operands && insn.reg_operands)
>       {
>         reg_op = i.op[insn.operands - 1].regs;
>         if (reg_op)
>           check if destination reg is REG_SP/REG_FP
>       }

The condition here isn't sufficient (the destination could also be a
memory operand), but something along these lines would certainly
address one of my fundamental concerns.

> 3. Operations with implicit update to stack pointer.
>     Currently we have no marker, but how about adding something like 
> unsigned int implicitstackop:1 in i386_opcode_modifier ? We will also 
> need to ensure this property is correctly conveyed for all existing 
> instructions in i386-opc.tbl.  When new instructions are added, the SCFI 
> machinery will be able to warn the user of missing functionality (and 
> not generate wrong CFI).

While I'm generally hesitant to see new attributes added, if one's
needed for a clear purpose (like looks to be the case here), that's
certainly fine. Much will depend on people (at least one of submitter
or reviewer) remembering to (ask to) add such an attribute whenever
needed.

> After this support for #3 is added to the backend, we will be in good 
> stead for future ISA additions wrt SCFI.
> 
> I can _try_ to accommodate 2b before the 2.42 release is cut. However, I 
> think its best to plan for both 2a and 2b for 2.43 release (given the 
> 2.42 is around the corner), if there is agreement.

If 2b was properly in place, I could probably be convinced to agree with
your work going in ahead of the APX stuff. It's in particular unclear to
me in how far APX is really going to make 2.42, considering how much of
a change it is, and how hard is has been so far to review the changes. It
would nevertheless feel better to me if your work was given some more
time, to go in only after 2.42 was branched off. I certainly can't
promise I will be able get around to reviewing either SCFI or APX again
before the end of the year (more precisely: before the holidays).

Jan