From: Indu Bhagat <indu.bhagat@oracle.com>
To: binutils@sourceware.org, Richard.Earnshaw@arm.com,
richard.sandiford@arm.com
Subject: Re: [PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI
Date: Thu, 11 Jul 2024 12:07:40 -0700 [thread overview]
Message-ID: <8bc9fd54-bceb-4440-84d4-46afa8de8119@oracle.com> (raw)
In-Reply-To: <mptr0c0ul1t.fsf@arm.com>
On 7/11/24 06:15, Richard Sandiford wrote:
> 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.
>
The shifter struct is out of the union in struct aarch64_opnd_info.
>>>> + 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.
>
Correct, we dont need the two register ADD/SUB for SCFI correctness. A
placeholder insn like GINSN_TYPE_OTHER emitted via the
aarch64_ginsn_unhandled () code path should also be sufficient for SCFI.
>>>> + 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.
>
Yes it is orthogonal recognizing Z loads and stores. I just meant that
once we recognize Z loads and stores, the utility of such a workflow
(i.e., do not error out on unrecognised load/stores conservatively as
they may not be necessary for SCFI correctness at all) is reduced as
less code blocks will trigger that.
> 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?
>
Roughly yes. I need to hook a few more things in (3) for the additional
warning.
next prev parent reply other threads:[~2024-07-11 19:07 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-01 2:53 [PATCH,V4 0/8] Add SCFI support for aarch64 Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 1/8] gas: scfi: make scfi_state_restore_reg function more precise Indu Bhagat
2024-07-12 15:03 ` [PATCH, V4 " Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 2/8] include: opcodes: aarch64: define new subclasses Indu Bhagat
2024-07-01 17:40 ` Richard Sandiford
2024-07-11 5:14 ` Indu Bhagat
2024-07-11 12:22 ` Richard Sandiford
2024-07-11 17:59 ` Indu Bhagat
2024-07-01 2:53 ` [PATCH,V4 3/8] opcodes: aarch64: flags to denote subclasses of ldst insns Indu Bhagat
2024-07-01 18:06 ` Richard Sandiford
2024-07-11 5:45 ` Indu Bhagat
2024-07-12 13:59 ` Indu Bhagat
2024-07-13 7:34 ` Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 4/8] opcodes: aarch64: flags to denote subclasses of arithmetic insns Indu Bhagat
2024-07-01 18:13 ` Richard Sandiford
2024-07-11 5:47 ` Indu Bhagat
2024-07-11 12:52 ` Richard Sandiford
2024-07-11 17:58 ` Indu Bhagat
2024-07-11 18:43 ` Richard Sandiford
2024-07-12 12:53 ` Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 5/8] opcodes: aarch64: flags to denote subclasses of uncond branches Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 6/8] opcodes: aarch64: enforce checks on subclass flags in aarch64-gen.c Indu Bhagat
2024-07-01 2:54 ` [PATCH,V4 7/8] gas: aarch64: add experimental support for SCFI Indu Bhagat
2024-07-01 19:49 ` Richard Sandiford
2024-07-11 6:30 ` Indu Bhagat
2024-07-11 13:15 ` Richard Sandiford
2024-07-11 19:07 ` Indu Bhagat [this message]
2024-07-11 20:10 ` Richard Sandiford
2024-07-01 2:54 ` [PATCH,V4 8/8] gas: aarch64: testsuite: add new tests " Indu Bhagat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8bc9fd54-bceb-4440-84d4-46afa8de8119@oracle.com \
--to=indu.bhagat@oracle.com \
--cc=Richard.Earnshaw@arm.com \
--cc=binutils@sourceware.org \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).