[PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI
Richard Sandiford
richard.sandiford@arm.com
Thu Jul 11 13:15:26 GMT 2024
Indu Bhagat <indu.bhagat@oracle.com> writes:
> On 7/1/24 12:49, Richard Sandiford wrote:
>> Indu Bhagat <indu.bhagat@oracle.com> writes:
>>> +{
>>> + enum aarch64_operand_class opnd_class;
>>> + unsigned int dw2reg_num = 0;
>>> +
>>> + opnd_class = aarch64_get_operand_class (opnd->type);
>>> +
>>> + switch (opnd_class)
>>> + {
>>> + case AARCH64_OPND_CLASS_FP_REG:
>>> + dw2reg_num = opnd->reg.regno + 64;
>>> + break;
>>> + case AARCH64_OPND_CLASS_SVE_REGLIST:
>>> + dw2reg_num = opnd->reglist.first_regno + 64;
>>> + break;
>>> + case AARCH64_OPND_CLASS_MODIFIED_REG:
>>> + case AARCH64_OPND_CLASS_INT_REG:
>>> + case AARCH64_OPND_CLASS_ADDRESS:
>>> + /* Use a dummy register value in case of WZR, else this will be an
>>> + incorrect dependency on REG_SP. */
>>> + if (!sp_allowed_p && opnd->reg.regno == REG_SP)
>>> + dw2reg_num = GINSN_DW2_REGNUM_R1_DUMMY;
>>> + else
>>> + /* For GPRs of our interest (callee-saved regs, SP, FP, LR),
>>> + DWARF register number is the same as AArch64 register number. */
>>> + dw2reg_num = opnd->reg.regno;
>>> + break;
>>
>> I think the AARCH64_OPND_CLASS_ADDRESS case should look at opnd->addr
>> instead. AARCH64_OPND_CLASS_MODIFIED_REG should look at opnd->shifter.
>>
>
> Made the correction for AARCH64_OPND_CLASS_ADDRESS.
>
> But for AARCH64_OPND_CLASS_MODIFIED_REG, the register information is
> still correct in opnd->reg.regno, IIUC. It seems to me that the only
> information available in the opnd->shifter is how the register is
> modified by additional work (shift, multiply etc.); This information is
> not used by SCFI:
> - an add/sub with two source register and destination REG_SP/ REG_FP
> makes REG_SP/ REG_FP untraceable. So ignoring the shift amount etc does
> not hurt SCFI correctness.
> - Cant think of other operations where the shift amount will affect
> SCFI correctness..
>
> So I am not sure of the "AARCH64_OPND_CLASS_MODIFIED_REG should look at
> opnd->shifter." of the review comment.
It's more about type correctness. "shifter" is the data associated with
AARCH64_OPND_CLASS_MODIFIED_REG and "reg" is the data associated with
AARCH64_OPND_CLASS_INT_REG etc. I think it's mostly a coincidence
that the "reg" and "shifter" alternatives of the union put the register
at the same byte offset from the start of the structure.
>>> + default:
>>> + as_bad ("Unexpected value in ginsn_dw2_regnum");
>>> + break;
>>> + }
>>> +
>>> + return dw2reg_num;
>>> +}
>>> +
>>> +/* Generate ginsn for addsub instructions with immediate opnd. */
>>> +
>>> +static ginsnS *
>>> +aarch64_ginsn_addsub_imm (const symbolS *insn_end_sym)
>>> +{
>>> + ginsnS *ginsn = NULL;
>>> + bool add_p, sub_p;
>>> + offsetT src_imm = 0;
>>> + unsigned int dst_reg, opnd_reg;
>>> + aarch64_opnd_info *dst, *opnd;
>>> + ginsnS *(*ginsn_func) (const symbolS *, bool,
>>> + enum ginsn_src_type, unsigned int, offsetT,
>>> + enum ginsn_src_type, unsigned int, offsetT,
>>> + enum ginsn_dst_type, unsigned int, offsetT);
>>> +
>>> + aarch64_inst *base = &inst.base;
>>> + const aarch64_opcode *opcode = base->opcode;
>>> +
>>> + add_p = aarch64_opcode_subclass_p (opcode, F_ARITH_ADD);
>>> + sub_p = aarch64_opcode_subclass_p (opcode, F_ARITH_SUB);
>>> + gas_assert (add_p || sub_p);
>>> + ginsn_func = add_p ? ginsn_new_add : ginsn_new_sub;
>>> +
>>> + gas_assert (aarch64_num_of_operands (opcode) == 3);
>>> + dst = &base->operands[0];
>>> + opnd = &base->operands[1];
>>> +
>>> + dst_reg = ginsn_dw2_regnum (dst, true);
>>> +
>>> + if (aarch64_gas_internal_fixup_p () && inst.reloc.exp.X_op == O_constant)
>>> + src_imm = inst.reloc.exp.X_add_number;
>>> + /* For any other relocation type, e.g., in add reg, reg, symbol, skip now
>>> + and handle via aarch64_ginsn_unhandled () code path. */
>>> + else if (inst.reloc.type != BFD_RELOC_UNUSED)
>>> + return ginsn;
>>> + /* FIXME - verify the understanding and remove assert. */
>>> + else
>>> + gas_assert (0);
>>> +
>>> + opnd_reg = ginsn_dw2_regnum (opnd, true);
>>> +
>>> + ginsn = ginsn_func (insn_end_sym, true,
>>> + GINSN_SRC_REG, opnd_reg, 0,
>>> + GINSN_SRC_IMM, 0, src_imm,
>>> + GINSN_DST_REG, dst_reg, 0);
>>> + ginsn_set_where (ginsn);
>>> +
>>> + return ginsn;
>>> +}
>>> +
>>> +/* Generate ginsn for addsub instructions with reg opnd. */
>>> +
>>> +static ginsnS *
>>> +aarch64_ginsn_addsub_reg (const symbolS *insn_end_sym)
>>> +{
>>> + ginsnS *ginsn = NULL;
>>> + bool add_p, sub_p;
>>> + unsigned int dst_reg, src1_reg, src2_reg;
>>> + aarch64_opnd_info *dst, *src1, *src2;
>>> + ginsnS *(*ginsn_func) (const symbolS *, bool,
>>> + enum ginsn_src_type, unsigned int, offsetT,
>>> + enum ginsn_src_type, unsigned int, offsetT,
>>> + enum ginsn_dst_type, unsigned int, offsetT);
>>> +
>>> + aarch64_inst *base = &inst.base;
>>> + const aarch64_opcode *opcode = base->opcode;
>>> +
>>> + add_p = aarch64_opcode_subclass_p (opcode, F_ARITH_ADD);
>>> + sub_p = aarch64_opcode_subclass_p (opcode, F_ARITH_SUB);
>>> + gas_assert (add_p || sub_p);
>>> + ginsn_func = add_p ? ginsn_new_add : ginsn_new_sub;
>>> +
>>> + gas_assert (aarch64_num_of_operands (opcode) == 3);
>>> + dst = &base->operands[0];
>>> + src1 = &base->operands[1];
>>> + src2 = &base->operands[2];
>>> +
>>> + dst_reg = ginsn_dw2_regnum (dst, true);
>>> + src1_reg = ginsn_dw2_regnum (src1, true);
>>> + src2_reg = ginsn_dw2_regnum (src2, false);
>>> +
>>> + ginsn = ginsn_func (insn_end_sym, true,
>>> + GINSN_SRC_REG, src1_reg, 0,
>>> + GINSN_SRC_REG, src2_reg, 0,
>>> + GINSN_DST_REG, dst_reg, 0);
>>> + ginsn_set_where (ginsn);
>>
>> It looks like this ignores any shift that is applied to src2_reg.
>>
>
> Yes. An add/sub operation with two source registers makes REG_SP/REG_FP
> untraceable (if dest is REG_SP/REG_FP), so any further information
> carried forward in the ginsn is of little use for SCFI.
>
> For a usecase apart from SCFI, yes, this may amount to loss of vital
> information. I will add a TBD_GINSN_INFO_LOSS comment.
If SCFI doesn't handle add/sub of two registers, then do we still
need this code for correctness? I would have expected SCFI to handle
unrecognised writes to SP in a conservative way, even if there is no
ginsn type defined for the instruction that sets SP.
>>> +
>>> + ginsn = ginsn_new_mov (insn_end_sym, false,
>>> + src_type, src_reg, src_imm,
>>> + GINSN_DST_REG, dst_reg, 0);
>>> + ginsn_set_where (ginsn);
>>> +
>>> + return ginsn;
>>> +}
>>> +
>>> +/* Check if an instruction is whitelisted.
>>> +
>>> + An instruction is a candidate for whitelisting if not generating ginsn for
>>> + it, does not affect SCFI correctness.
>>> +
>>> + TBD_GINSN_GEN_NOT_SCFI. This function assumes GINSN_GEN_SCFI is in effect.
>>> + When other ginsn_gen_mode are added, this will need fixing. */
>>> +
>>> +static bool
>>> +aarch64_ginsn_safe_to_skip_p (void)
>>> +{
>>> + bool skip_p = false;
>>> + aarch64_opnd_info *opnd = NULL;
>>> + unsigned int dw2_regnum;
>>> + unsigned int opnd_reg;
>>> + int num_opnds = 0;
>>> +
>>> + aarch64_inst *base = &inst.base;
>>> + const aarch64_opcode *opcode = base->opcode;
>>> +
>>> + /* ATM, whitelisting operations with no operands does not seem to be
>>> + necessary. */
>>> + num_opnds = aarch64_num_of_operands (opcode);
>>> + if (!num_opnds)
>>> + return skip_p;
>>
>> Things like ERET show that this would be dangerous, so it might be better
>> to make the false return explicit (and adjust the comment).
>>
>
> I am not sure I understand the "make false return explicit". I have
> adjusted the code and comment:
>
> /* ATM, whitelisting operations with no operands does not seem to be
> necessary. In fact, whitelisting insns like ERET will be
> dangerous for
> SCFI. So, return false now and bar any such insns from being
> whitelisted
> altogether. */
> num_opnds = aarch64_num_of_operands (opcode);
> if (!num_opnds)
> return false;
>
> Is this what you meant ?
Yes, thanks.
>>> + if (opnd->type == AARCH64_OPND_Rd_SP)
>>> + {
>>> + dw2_regnum = ginsn_dw2_regnum (opnd, true);
>>> + if (dw2_regnum == REG_SP)
>>> + skip_p = true;
>>> + }
>>> + break;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + return skip_p;
>>> +}
>>> +
>>> +#define AARCH64_GINSN_UNHANDLED_NONE 0
>>> +#define AARCH64_GINSN_UNHANDLED_DEST_REG 1
>>> +#define AARCH64_GINSN_UNHANDLED_CFG 2
>>> +#define AARCH64_GINSN_UNHANDLED_STACKOP 3
>>> +#define AARCH64_GINSN_UNHANDLED_UNEXPECTED 4
>>> +
>>> +/* Check the input insn for its impact on the correctness of the synthesized
>>> + CFI. Returns an error code to the caller. */
>>> +
>>> +static int
>>> +aarch64_ginsn_unhandled (void)
>>> +{
>>> + int err = AARCH64_GINSN_UNHANDLED_NONE;
>>> + aarch64_inst *base = &inst.base;
>>> + const aarch64_opcode *opcode = base->opcode;
>>> + aarch64_opnd_info *dest = &base->operands[0];
>>> + int num_opnds = aarch64_num_of_operands (opcode);
>>> + aarch64_opnd_info *addr;
>>> + unsigned int dw2_regnum;
>>> + unsigned int addr_reg;
>>> + aarch64_opnd_info *opnd = NULL;
>>> + unsigned int opnd_reg;
>>> + bool sp_allowed_p = false;
>>> +
>>> + /* All change of flow instructions are important for SCFI. */
>>> + if (opcode->iclass == condbranch
>>> + || opcode->iclass == compbranch
>>> + || opcode->iclass == testbranch
>>> + || opcode->iclass == branch_imm
>>> + || opcode->iclass == branch_reg)
>>> + err = AARCH64_GINSN_UNHANDLED_CFG;
>>> + /* Also, any memory instructions that may involve an update to the stack
>>> + pointer or save/restore of callee-saved registers must not be skipped.
>>> + Note that, some iclasses cannot be used to push or pop stack because of
>>> + disallowed writeback: ldst_unscaled, ldst_regoff, ldst_unpriv, ldstexcl,
>>> + loadlit, ldstnapair_offs. FIXME double-check.
>>> + Also, these iclasses do not seem to be amenable to being used for
>>> + save/restore ops either. FIXME double-check. */
>>> + else if (opcode->iclass == ldstpair_off
>>> + || opcode->iclass == ldstpair_indexed
>>> + || opcode->iclass == ldst_imm9
>>> + || opcode->iclass == ldst_imm10
>>> + || opcode->iclass == ldst_pos
>>> + /* STR Zn are especially complicated as they do not store in the same byte
>>> + order for big-endian: STR Qn stores as a 128-bit integer (MSB first),
>>> + whereas STR Zn stores as a stream of bytes (LSB first). FIXME Simply punt
>>> + on the big-endian and little-endian SVE PCS case for now. */
>>> + || opcode->iclass == sve_misc)
>>> + {
>>> + opnd = &base->operands[0];
>>> + addr = &base->operands[num_opnds - 1];
>>> + addr_reg = ginsn_dw2_regnum (addr, true);
>>> + opnd_reg = ginsn_dw2_regnum (opnd, false);
>>> + /* For all skipped memory operations, check if an update to REG_SP or
>>> + REG_FP is involved. */
>>> + if ((addr_reg == REG_SP || addr_reg == REG_FP)
>>> + && (((addr->addr.postind || addr->addr.preind) && addr->addr.writeback)
>>> + || aarch64_scfi_callee_saved_p (opnd_reg)))
>>> +
>>> + err = AARCH64_GINSN_UNHANDLED_STACKOP;
>>
>> The way I'd imagined this working is that we'd use two-directional data
>> flow to record which callee-saved registers are "protected" at which
>> instructions. That is, for saves we'd walk the cfg forward from the
>> entry point, recording which registers have been saved earlier in the walk
>> (and being conservatively correct for cycles). And for restores we'd
>> walk the cfg backwards from the exit points, recording which registers
>> are restored later in the walk. If a register is both "saved ealier" and
>> "restored later" for a given instruction, it wouldn't matter what the
>> instruction does with that register.
>>
>> (For infinite loops we would pretend that all registers are restored later.)
>>
>> It seems like instead, we'd generate a warning for:
>>
>> ...
>> str d8, [sp, #...]
>> ...
>> ld1d z8.b, p0/z, [sp, #...]
>> ...
>> ldr d8, [sp, #...]
>> ...
>> ret
>>
>> even though that should be ok (even without interpreting SVE instructions).
>>
>> On the other hand, any read or write of a callee-saved register is suspicious
>> if it isn't saved earlier or isn't restored later.
>>
>> In other words, it seems like this information should be context-dependent,
>> or used in a context-dependent way. Whether a particular register matters
>> depends on whether the register might still contain the caller's data.
>>
>
> We do something similar for non-memory ops:
> - In the aarch64_ginsn_unhandled () codepath, if the destination is of
> interest (REG_SP, REG_FP), synthesize a GINSN_TYPE_OTHER with the
> appropriate destination register if no ginsn has been generated.
> - The SCFI machinery checks if this instruction affects SCFI correctness.
> - If it does, only then the insn is issued a complaint for and brought
> to attention.
>
> What you suggest is take a similar approach for memory operations too.
> I think it makes sense. It needs some work now to accommodate this, but
> I think we can re-evaluate after some basic handling for Z registers is
> in place ? I plan to work on adding handling for Z registers after this
> series is merged. (I have renamed an old testcase involving Z registers
> to "scfi-unsupported-2.s" to keep track of the currently unsupported
> cases to be worked on in near future, so I dont lose track of this..).
Yeah, leaving it till later sounds ok. But I don't think this should
depend on recognising Z loads and stores. It should just depend on
processing Z registers in the operand lists, and recognising that the
low 64 bits of Z8-Z15 are the same as D8-D15.
In other words, I imagined the set-up would be:
(1) the operand list should tell us whether a register is referenced
(2) recognising 8-byte+ loads and stores, plus support arithmetic,
lets us recognise instructions that would function as a save or
restore (and at what offset)
(3) dataflow analysis based on (2) tells us, for a given instruction,
which registers might have caller data
(4) A reference to possible caller data triggers a warning
(references detected by (1), caller data by (3))
Is that roughly how it works?
Thanks,
Richard
More information about the Binutils
mailing list