Re: [PATCH v6 bpf-next 16/17] selftests/bpf: add new verifier_gotox test

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/10/21 03:42PM, Eduard Zingerman wrote:
> On Sun, 2025-10-19 at 20:21 +0000, Anton Protopopov wrote:
> > Add a set of tests to validate core gotox functionality
> > without need to rely on compilers.
> > 
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> 
> Thank you for adding these.
> Could you please also add a test cases that checks the following errors:
> - "jump table for insn %d points outside of the subprog [%u,%u]"
> - "the sum of R%u umin_value %llu and off %u is too big\n"
> - "register R%d doesn't point to any offset in map id=%d\n"

Yeah, sorry, these actually were on my list, but I've postponed them
for the next version. Will add. (I also need to add a few selftests
on the offset when loading from map.)

> 
> Might be the case that some of these can't be triggered because of the
> check_mem_access() call.
> 
> [...]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_gotox.c b/tools/testing/selftests/bpf/progs/verifier_gotox.c
> > new file mode 100644
> > index 000000000000..1a92e4d321e8
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/verifier_gotox.c
> 
> [...]
> 
> > +/*
> > + * Gotox is forbidden when there is no jump table loaded
> > + * which points to the sub-function where the gotox is used
> > + */
> > +SEC("socket")
> > +__failure __msg("no jump tables found for subprog starting at 0")
>                                                               ^^^^
> 				Nit: one day we need to figure out a way to
> 				     report subprogram names, when reporting
> 				     check_cfg() errors.

But those are not always present, right?

> 
> > +__naked void jump_table_no_jump_table(void)
> > +{
> > +	asm volatile ("						\
> > +	.8byte %[gotox_r0];					\
> > +	r0 = 1;							\
> > +	exit;							\
> > +"	:							\
> > +	: __imm_insn(gotox_r0, BPF_RAW_INSN(BPF_JMP | BPF_JA | BPF_X, BPF_REG_0, 0, 0 , 0))
> > +	: __clobber_all);
> > +}
> > +
> > +/*
> > + * Incorrect type of the target register, only PTR_TO_INSN allowed
> > + */
> > +SEC("socket")
> > +__failure __msg("R1 has type 1, expected PTR_TO_INSN")
>                            ^^^^^^
> 	      log.c:reg_type_str() should help here.

Yes, thanks, this was changed to address your comment
in the other patch.

> > +__naked void jump_table_incorrect_dst_reg_type(void)
> > +{
> > +	asm volatile ("						\
> > +	.pushsection .jumptables,\"\",@progbits;		\
> > +jt0_%=:								\
> > +	.quad ret0_%=;						\
> > +	.quad ret1_%=;						\
> > +	.size jt0_%=, 16;					\
> > +	.global jt0_%=;						\
> > +	.popsection;						\
> > +								\
> > +	r0 = jt0_%= ll;						\
> > +	r0 += 8;						\
> > +	r0 = *(u64 *)(r0 + 0);					\
> > +	r1 = 42;						\
> > +	.8byte %[gotox_r1];					\
> > +	ret0_%=:						\
> > +	r0 = 0;							\
> > +	exit;							\
> > +	ret1_%=:						\
> > +	r0 = 1;							\
> > +	exit;							\
> > +"	:							\
> > +	: __imm_insn(gotox_r1, BPF_RAW_INSN(BPF_JMP | BPF_JA | BPF_X, BPF_REG_1, 0, 0 , 0))
> > +	: __clobber_all);
> > +}
> > +
> > +#define DEFINE_INVALID_SIZE_PROG(READ_SIZE, OUTCOME)			\
> 
> Nit: this can be merged with DEFINE_SIMPLE_JUMP_TABLE_PROG.

Didn't want to overload the macro too much so the prog stays
readable. (Here are two different regs are used.) I will check
how it looks like if I merge them, and merge, if it looks ok-ish.

> > +									\
> > +	SEC("socket")							\
> > +	OUTCOME								\
> > +	__naked void jump_table_invalid_read_size_ ## READ_SIZE(void)	\
> > +	{								\
> > +		asm volatile ("						\
> > +		.pushsection .jumptables,\"\",@progbits;		\
> > +	jt0_%=:								\
> > +		.quad ret0_%=;						\
> > +		.quad ret1_%=;						\
> > +		.size jt0_%=, 16;					\
> > +		.global jt0_%=;						\
> > +		.popsection;						\
> > +									\
> > +		r0 = jt0_%= ll;						\
> > +		r0 += 8;						\
> > +		r0 = *(" #READ_SIZE " *)(r0 + 0);			\
> > +		.8byte %[gotox_r0];					\
> > +		ret0_%=:						\
> > +		r0 = 0;							\
> > +		exit;							\
> > +		ret1_%=:						\
> > +		r0 = 1;							\
> > +		exit;							\
> > +	"	:							\
> > +		: __imm_insn(gotox_r0, BPF_RAW_INSN(BPF_JMP | BPF_JA | BPF_X, BPF_REG_0, 0, 0 , 0)) \
> > +		: __clobber_all);					\
> > +	}
> > +
> > +DEFINE_INVALID_SIZE_PROG(u32, __failure __msg("Invalid read of 4 bytes from insn_array"))
> > +DEFINE_INVALID_SIZE_PROG(u16, __failure __msg("Invalid read of 2 bytes from insn_array"))
> > +DEFINE_INVALID_SIZE_PROG(u8,  __failure __msg("Invalid read of 1 bytes from insn_array"))
> 
> [...]