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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 7 Sep 2022 10:02:40 +0200
Peter Zijlstra <peterz@infradead.org> 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@kernel.org>