Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK

Peter Zijlstra
2022-09-07T08:03:43Z

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.

> */
> + 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.

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.