On Wed, 7 Sep 2022 10:02:40 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote:
>
> > static int can_probe(unsigned long paddr)
> > {
> > kprobe_opcode_t buf[MAX_INSN_SIZE];
> > + unsigned long addr, offset = 0;
> > + struct insn insn;
> >
> > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset))
> > return 0;
> >
> > + /* The first address must be instruction boundary. */
> > + if (!offset)
> > + return 1;
> >
> > + /* Decode instructions */
> > + for_each_insn(&insn, paddr - offset, paddr, buf) {
> > /*
> > + * CONFIG_RETHUNK or CONFIG_SLS or another debug feature
> > + * may install INT3.
>
> Note: they are not debug features.
Yes, sorry for confusion. CONFIG_RETHUNK/CONFIG_SLS are security
feature, and something like kgdb is debug feature, what I meant
here.
>
> > */
> > + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) {
> > + /* Find the next non-INT3 instruction address */
> > + addr = skip_padding_int3((unsigned long)insn.kaddr);
> > + if (!addr)
> > + return 0;
> > + /*
> > + * This can be a padding INT3 for CONFIG_RETHUNK or
> > + * CONFIG_SLS. If a branch jumps to the address next
> > + * to the INT3 sequence, this is just for padding,
> > + * then we can continue decoding.
> > + */
> > + for_each_insn(&insn, paddr - offset, addr, buf) {
> > + if (insn_get_branch_addr(&insn) == addr)
> > + goto found;
> > + }
> >
> > + /* This INT3 can not be decoded safely. */
> > return 0;
> > +found:
> > + /* Set loop cursor */
> > + insn.next_byte = (void *)addr;
> > + continue;
> > + }
> > }
> >
> > + return ((unsigned long)insn.next_byte == paddr);
> > }
>
> If I understand correctly, it'll fail on something like this:
>
> foo: insn
> insn
> insn
> jmp 2f
> int3
>
> 1: insn
> insn
> 2: insn
> jcc 1b
>
> ret
> int3
>
> Which isn't weird code by any means. And soon to be generated by
> compilers.
Hmm, yeah, I thought that was rare case.
>
>
> Maybe something like:
>
> struct queue {
> int head, tail;
> unsigned long val[16]; /* insufficient; probably should allocate something */
> };
>
> void push(struct queue *q, unsigned long val)
> {
> /* break loops, been here already */
> for (int i = 0; i < q->head; i++) {
> if (q->val[i] == val)
> return;
> }
>
> q->val[q->head++] = val;
>
> WARN_ON(q->head > ARRAY_SIZE(q->val)
> }
>
> unsigned long pop(struct queue *q)
> {
> if (q->tail == q->head)
> return 0;
>
> return q->val[q->tail++];
> }
>
> bool dead_end_insn(struct instruction *insn)
> {
> switch (insn->opcode.bytes[0]) {
> case INT3_INSN_OPCODE:
> case JMP8_INSN_OPCODE:
> case JMP32_INSN_OPCODE:
> return true; /* no arch execution after these */
>
> case 0xff:
> /* jmp *%reg; jmpf */
> if (modrm_reg == 4 || modrm_reg == 5)
> return true;
> break;
>
> default:
> break;
> }
>
> return false;
> }
>
>
>
> struct queue q;
>
> start = paddr - offset;
> end = start + size;
> push(&q, paddr - offset);
>
> while (start = pop(&q)) {
> for_each_insn(&insn, start, end, buf) {
> if (insn.kaddr == paddr)
> return 1;
>
> target = insn_get_branch_addr(&insn);
> if (target)
> push(&q, target);
>
> if (dead_end_insn(&insn))
> break;
> }
> }
>
>
>
> It's a bit of a pain, but I think it should cover things.
Yeah, this looks good to me. What I just need is to add expanding
queue buffer. (can we use xarray for this purpose?)
Thank you!
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
