Re: [PATCH,V4 10/14] gas: synthesize CFI for hand-written asm

[Jan Beulich <jbeulich@suse.com>]

On 10.01.2024 07:10, Indu Bhagat wrote:
> On 1/9/24 01:30, Jan Beulich wrote:
>> On 08.01.2024 20:33, Indu Bhagat wrote:
>>> On 1/5/24 05:58, Jan Beulich wrote:
>>>> On 03.01.2024 08:15, Indu Bhagat wrote:
>>>>> +/* Check whether a '66H' prefix accompanies the instruction.
>>>>
>>>> With APX 16-bit operand size isn't necessarily represented by a 66h
>>>> prefix, but perhaps with an "embedded prefix" inside the EVEX one.
>>>> Therefore both the comment and even more so ...
>>>>
>>>>> +   The current users of this API are in the handlers for PUSH, POP
>>>>> +   instructions.  These instructions affect the stack pointer implicitly:  the
>>>>> +   operand size (16, 32, or 64 bits) determines the amount by which the stack
>>>>> +   pointer is decremented (2, 4 or 8).  When '66H' prefix is present, the
>>>>> +   instruction has a 16-bit operand.  */
>>>>> +
>>>>> +static bool
>>>>> +ginsn_prefix_66H_p (i386_insn insn)
>>>>
>>>> ... the function name would better not allude to just the legacy
>>>> encoding. Maybe ginsn_opsize_prefix_p()?
>>>>
>>>
>>> Isnt 66H_p more readable and easier to follow because that's what the
>>> function is currently checking ?  If more scenarios were being handled,
>>> ginsn_opsize_prefix_p () would fit better.
>>
>> Well, as said - with APX you can't get away with just 0x66 prefix checking.
>> That prefix is simply illegal to use with EVEX-encoded insns.
>>
> 
> I am using the following in ginsn_opsize_prefix_p ():
> 
> !(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66

That addresses one half of my earlier remarks. Note however that elsewhere
we never check i.prefix[DATA_PREFIX] against being 0x66; we only ever check
for it being zero or non-zero. I'd like this to remain consistent.

For EVEX-encoded APX insns this isn't going to be sufficient though. See
respective code in process_suffix():

	  /* The DATA PREFIX of EVEX promoted from legacy APX instructions
	     needs to be adjusted.  */
	  if (i.tm.opcode_space == SPACE_EVEXMAP4)
	    {
	      gas_assert (!i.tm.opcode_modifier.opcodeprefix);
	      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
	    }
	  else if (!add_prefix (prefix))
	    return 0;

So you'll need to also check for that combination, plus take care of
avoiding insns where PREFIX_0X66 alters operation, not operand size
(ADCX being an example).

>>>>> +static ginsnS *
>>>>> +x86_ginsn_move (const symbolS *insn_end_sym)
>>>>> +{
>>>>> +  ginsnS *ginsn;
>>>>> +  unsigned int dst_reg;
>>>>> +  unsigned int src_reg;
>>>>> +  offsetT src_disp = 0;
>>>>> +  offsetT dst_disp = 0;
>>>>> +  const reg_entry *dst = NULL;
>>>>> +  const reg_entry *src = NULL;
>>>>> +  uint16_t opcode = i.tm.base_opcode;
>>>>> +  enum ginsn_src_type src_type = GINSN_SRC_REG;
>>>>> +  enum ginsn_dst_type dst_type = GINSN_DST_REG;
>>>>> +
>>>>> +  if (opcode == 0x8b || opcode == 0x8a)
>>>>
>>>> Above when handling ALU insns you didn't care about byte ops - why do
>>>> you do so here (by checking for 0x8a, and 0x88 below)?
>>>
>>> Because there may be weird code that saves and restores 8-byte registers
>>> on stack.  For ALU insns, if the destination is REG_SP/REG_FP, we will
>>> detect them in the unhandled track.
>>
>> You talk about 8-byte registers when I asked about byte reg moves. If
>> there's a MOV targeting %spl or %bpl, is that really any different from,
>> say, an ADD doing so?
>>
> 
> ATM, Yes. SCFI has heuristics implemented, _some_ of which (can be 
> evolved) are opcode specific. E.g.,
>    - MOV %rsp, %rbp will make SCFI machinery check if this is making the 
> CFA switch to %rbp.  If the target was %bpl, since we track 8-byte 
> registers, it will still trigger the same code path. A bug as I ack below.
>    - ADD rsp + 0 = rbp will not trigger CFA switch to RBP. Should it ? 
> Perhaps yes? Its on my TODO list (with low priority) to evolve this bit.

I'm not sure about this. Either way such special cases may want documenting
explicitly.

>>>>> +    }
>>>>> +
>>>>> +  ginsn_set_where (ginsn);
>>>>> +
>>>>> +  return ginsn;
>>>>> +}
>>>>
>>>> Throughout this function (and perhaps others as well), how come you
>>>> don't consider operand size at all? It matters whether results are
>>>> 64-bit, 32-bit, or 16-bit, after all.
>>>
>>> Operation size matters: No, not for all instructions in context of SCFI.
>>> For instructions using stack (save / restore), the size is important.
>>> But for other instructions, operation size will not affect SCFI correctness.
>>
>> But aren't you wrongly treating an update of %rbp and an update of,
>> say, %bp the same then? The latter should end up as untracable, aiui.
>>
> 
> For ALU ops:
>    - ADD/SUB reg1, reg2
>      If reg2 is the same as REG_CFA, then even in 64-bit mode, this 
> causes untraceability. So there is untraceability irrespective of 
> operation size. On the other hand, if uninteresting, it will remain 
> unintersting irrespective of operation size.
>    - ADD/SUB imm, reg will never trigger untraceability, irrespective of 
> size. There is the element of ignoring the carry bit, but I think the 
> current diagnostics around "asymmetrical save/restore" and the planned 
> "unbalanced stack at return" should provide user with some safety net.

I don't see how the carry flag would matter here. What does matter is the
size of the register: Anything not 64-bit will need to trigger
untraceability imo.

>    - Other ALU ops should all trigger untracebility alike, irrespective 
> of operation size.
> Hence, my statement that ignoring operation size is fine here for SCFI.

As per above, I disagree.

> For MOV ops:
>    - 8-bit/16-bit MOV should trigger untracebility. I ack this as bug to 
> be fixed. Thanks to your careful review. ATM, I plan to deal with this 
> after the series goes in, unless you have strong opinion about this.

32-bit MOV should, too. And yes, I'm okay-ish with such being dealt with
later, as long as the open work item is easily recognizable (and hence
it'll be easy to check that all such work items are gone at the point
where the feature is promoted from experimental).

>>>>> +/* Generate one or more generic GAS instructions, a.k.a, ginsns for the current
>>>>> +   machine instruction.
>>>>> +
>>>>> +   Returns the head of linked list of ginsn(s) added, if success; Returns NULL
>>>>> +   if failure.
>>>>> +
>>>>> +   The input ginsn_gen_mode GMODE determines the set of minimal necessary
>>>>> +   ginsns necessary for correctness of any passes applicable for that mode.
>>>>> +   For supporting the GINSN_GEN_SCFI generation mode, following is the list of
>>>>> +   machine instructions that must be translated into the corresponding ginsns
>>>>> +   to ensure correctness of SCFI:
>>>>> +     - All instructions affecting the two registers that could potentially
>>>>> +       be used as the base register for CFA tracking.  For SCFI, the base
>>>>> +       register for CFA tracking is limited to REG_SP and REG_FP only for
>>>>> +       now.
>>>>> +     - All change of flow instructions: conditional and unconditional branches,
>>>>> +       call and return from functions.
>>>>> +     - All instructions that can potentially be a register save / restore
>>>>> +       operation.
>>>>
>>>> This could do with being more fine grained, as "potentially" is pretty vague,
>>>> and (as per earlier version review comments) my take on this is a much wider
>>>> set than yours.
>>>
>>> I would like to understand more on this comment, especially the "my take
>>> on this is a much wider set than yours".  I see its being hinted at in
>>> different flavors in the current review.
>>>
>>> I see some issues pointed out in this review (addressing modes of mov
>>> etc, safe to skip opcodes for TEST, CMP) etc., but it seems that your
>>> concerns are wider than this.
>>
>> I earlier version review I mentioned that even vector or mask registers
>> could in principle be use to hold preserved GPR values. I seem to recall
>> that you said you wouldn't want to deal with such. Hence my use of
>> "wider set": Just to give an example, "kmovq %rbp, %k0" plus later
>> "kmovq %k0, %rbp" is a pair of "instructions that can potentially be a
>> register save / restore operation".
>>
> 
> Hmm. I will need to understand them on a case to case basis.  For the 
> case of "kmovq %rbp, %k0" / "kmovq %k0, %rbp" how can this be used as 
> save/restore to/from stack ?

Maybe I'm still not having a clear enough picture of what forms of insns
you want to fully track. Said insn forms don't access the stack. But they
could in principle be used to preserve a certain register. Such preserving
of registers is part of what needs encoding in CFI, isn't it?

>>>>> +      ginsn = ginsn_new_load (insn_end_sym, false,
>>>>> +			      GINSN_SRC_INDIRECT, REG_SP, 0,
>>>>> +			      GINSN_DST_REG, dw2_regnum);
>>>>> +      ginsn_set_where (ginsn);
>>>>> +      ginsn_next = ginsn_new_add (insn_end_sym, false,
>>>>> +				  GINSN_SRC_REG, REG_SP, 0,
>>>>> +				  GINSN_SRC_IMM, 0, stack_opnd_size,
>>>>> +				  GINSN_DST_REG, REG_SP, 0);
>>>>> +      ginsn_set_where (ginsn_next);
>>>>> +      gas_assert (!ginsn_link_next (ginsn, ginsn_next));
>>>>> +      break;
>>>>> +
>>>>> +    case 0xff:
>>>>> +      if (i.tm.opcode_space != SPACE_BASE)
>>>>> +	break;
>>>>> +      /* push from mem.  */
>>>>> +      if (i.tm.extension_opcode == 6)
>>>>> +	{
>>>>> +	  /* In 64-bit mode, presence of 66H prefix indicates 16-bit op.  */
>>>>> +	  if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i))
>>>>> +	    stack_opnd_size = 2;
>>>>> +	  ginsn = ginsn_new_sub (insn_end_sym, false,
>>>>> +				 GINSN_SRC_REG, REG_SP, 0,
>>>>> +				 GINSN_SRC_IMM, 0, stack_opnd_size,
>>>>> +				 GINSN_DST_REG, REG_SP, 0);
>>>>> +	  ginsn_set_where (ginsn);
>>>>> +	  /* These instructions have no imm, only indirect access.  */
>>>>> +	  gas_assert (i.base_reg);
>>>>> +	  dw2_regnum = ginsn_dw2_regnum (i.base_reg);
>>>>> +	  ginsn_next = ginsn_new_store (insn_end_sym, false,
>>>>> +					GINSN_SRC_INDIRECT, dw2_regnum,
>>>>> +					GINSN_DST_INDIRECT, REG_SP, 0);
>>>>> +	  ginsn_set_where (ginsn_next);
>>>>> +	  gas_assert (!ginsn_link_next (ginsn, ginsn_next));
>>>>> +	}
>>>>> +      else if (i.tm.extension_opcode == 4)
>>>>> +	{
>>>>> +	  /* jmp r/m.  E.g., notrack jmp *%rax.  */
>>>>> +	  if (i.reg_operands)
>>>>> +	    {
>>>>> +	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
>>>>> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
>>>>> +				      GINSN_SRC_REG, dw2_regnum, NULL);
>>>>> +	      ginsn_set_where (ginsn);
>>>>> +	    }
>>>>> +	  else if (i.mem_operands && i.index_reg)
>>>>> +	    {
>>>>> +	      /* jmp    *0x0(,%rax,8).  */
>>>>> +	      dw2_regnum = ginsn_dw2_regnum (i.index_reg);
>>>>> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
>>>>> +				      GINSN_SRC_REG, dw2_regnum, NULL);
>>>>> +	      ginsn_set_where (ginsn);
>>>>
>>>> What if both base and index are in use? Like for PUSH/POP, all addressing
>>>> forms are permitted here and ...
>>>>
>>>>> +	    }
>>>>> +	  else if (i.mem_operands && i.base_reg)
>>>>> +	    {
>>>>> +	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
>>>>> +	      ginsn = ginsn_new_jump (insn_end_sym, true,
>>>>> +				      GINSN_SRC_REG, dw2_regnum, NULL);
>>>>> +	      ginsn_set_where (ginsn);
>>>>> +	    }
>>>>> +	}
>>>>> +      else if (i.tm.extension_opcode == 2)
>>>>> +	{
>>>>> +	  /* 0xFF /2 (call).  */
>>>>> +	  if (i.reg_operands)
>>>>> +	    {
>>>>> +	      dw2_regnum = ginsn_dw2_regnum (i.op[0].regs);
>>>>> +	      ginsn = ginsn_new_call (insn_end_sym, true,
>>>>> +				      GINSN_SRC_REG, dw2_regnum, NULL);
>>>>> +	      ginsn_set_where (ginsn);
>>>>> +	    }
>>>>> +	  else if (i.mem_operands && i.base_reg)
>>>>> +	    {
>>>>> +	      dw2_regnum = ginsn_dw2_regnum (i.base_reg);
>>>>> +	      ginsn = ginsn_new_call (insn_end_sym, true,
>>>>> +				      GINSN_SRC_REG, dw2_regnum, NULL);
>>>>> +	      ginsn_set_where (ginsn);
>>>>> +	    }
>>>>
>>>> ... here.
>>>
>>> For the indirect jump and call instructions, the target destination is
>>> not necessary to be known.  Indirect jumps will cause SCFI to error out
>>> as control flow cannot be constructed.
>>>
>>> Call instructions are assumed to transfer control out of function.
>>>
>>> In other words, more information in these cases will not be of use to SCFI.
>>
>> But then aren't you already doing too much work here? With what you say,
>> you don't care about the kind of operand at all, merely the fact it's a
>> CALL might be interesting. Albeit even there I'd then wonder why - if
>> the function called isn't of interest, how is CALL different from just
>> NOP? The only CALL you'd need to be concerned about would be the direct
>> form with a displacement of 0, as indicated elsewhere. But of course
>> tricky code might also use variants of that; see e.g. the retpoline code
>> that was introduced into kernels a couple of years back. Recognizing
>> and bailing on such tricky code may be desirable, if not necessary.
>>
> 
> Tracking operands for CALL instructions does not provide value ATM.  We 
> do not even split a BB if there is a CALL instruction (the assumption is 
> that CALL insn transfers control out of function).
> 
> You're right that we need to treat some CALLs differently (because of 
> its affect of control flow and SCFI correctness).
> 
> RE: doing more work. Sure, but the vision for ginsn was to allow a 
> representation where other analyses may be added. The representation of 
> ginsn will need revisions to get there, but keeping an explicit 
> GINSN_TYPE_CALL seems good.

That's okay, but with as little dead (for now) code as possible. If the
operand isn't interesting, why deal with the various operand forms right
now?

>>>>> +	}
>>>>> +      break;
>>>>> +
>>>>> +    case 0xc2:
>>>>> +    case 0xc3:
>>>>> +      if (i.tm.opcode_space != SPACE_BASE)
>>>>> +	break;
>>>>> +      /* Near ret.  */
>>>>> +      ginsn = ginsn_new_return (insn_end_sym, true);
>>>>> +      ginsn_set_where (ginsn);
>>>>> +      break;
>>>>
>>>> No tracking of the stack pointer adjustment?
>>>
>>> No stack unwind information for a function is relevant after the
>>> function has returned.  So, tracking of stack pointer adjustment by
>>> return is not necessary.
>>
>> What information does the "return" insn then carry, beyond it being
>> an unconditional branch (which you have a different insn for)?
>>
> 
> "return" does not carry any more information than just the 
> GINSN_TYPE_RETURN as ginsn->type.
> 
> So then why support both "return" and an unconditional branch: The 
> intention is to carry the semantic difference between ret and 
> unconditional jump.  Unconditional jumps may be to a label within 
> function, and in those cases, we use it for some validation and BB 
> linking when creating CFG. Return, OTOH, always indicates exit from 
> function.
> 
> For SCFI purposes, above is the one use.  Future analyses may find other 
> use-cases for an explicit return ginsn.  But IMO, keeping 
> GINSN_TYPE_RETURN as an explicit insn makes the overall offering cleaner.

Okay. And here you don't bother decoding operands. Hence why I'm
asking the same to be the case for (e.g.) CALL.

>>>>> @@ -5830,6 +6804,14 @@ md_assemble (char *line)
>>>>>      /* We are ready to output the insn.  */
>>>>>      output_insn (last_insn);
>>>>>    
>>>>> +  /* PS: SCFI is enabled only for AMD64 ABI.  The ABI check has been
>>>>> +     performed in i386_target_format.  */
>>>>
>>>> See my earlier comment - it's yet more restrictive (as in not covering
>>>> e.g. the Windows ABI, which importantly is also used in EFI).
>>>
>>> Not clear, can you please elaborate ?
>>
>> Hmm, it's not clear to me what's not clear in my earlier comment. The
>> set of preserved registers, for example, differs between the SysV and
>> the Windows ABIs (see x86_scfi_callee_saved_p()). Then again, thinking
>> of it, talking of anything ABI-ish in assembly code is questionable.
>> An assembly function calling another assembly function may use an
>> entirely custom "ABI". You just cannot guess upon preserved registers.
>>
> 
> I think the confusion is stemming from my usage of AMD64 colloquially to 
> refer to System V ABI for x86_64. I think "System V AMD64 ABI" is the 
> correct term. I will use that.
> 
> And yes, GAS can only work out SCFI if there is ABI adherence.  If input 
> asm does not adhere to the supported ABIs, and the user invokes SCFI, 
> then the user is on their own.  GAS will not (rather can not) warn about 
> ABI incompliance.

I don't recall documentation stating this explicitly. This is a pretty
important aspect for users to consider, after all.

Jan