Commit 107af8ec authored by Daniel Borkmann's avatar Daniel Borkmann
Browse files

Merge branch 'bpf-fix-null-arg-semantics' 



Gianluca Borello says:

====================
This set includes some fixes in semantics and usability issues that emerged
recently, and would be good to have them in net before the next release.

In particular, ARG_CONST_SIZE_OR_ZERO semantics was recently changed in
commit 9fd29c08 ("bpf: improve verifier ARG_CONST_SIZE_OR_ZERO
semantics") with the goal of letting the compiler generate simpler code
that the verifier can more easily accept.

To handle this change in semantics, a few checks in some helpers were
added, like in commit 9c019e2b ("bpf: change helper bpf_probe_read arg2
type to ARG_CONST_SIZE_OR_ZERO"), and those checks are less than ideal
because once they make it into a released kernel bpf programs can start
relying on them, preventing the possibility of being removed later on.

This patch tries to fix the issue by introducing a new argument type
ARG_PTR_TO_MEM_OR_NULL that can be used for helpers that can receive a
<NULL, 0> tuple. By doing so, we can fix the semantics of the other helpers
that don't need <NULL, 0> and can just handle <!NULL, 0>, allowing the code
to get rid of those checks.
====================

Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
parents f1a8b8e3 a60dd35d

include/linux/bpf.h

+1 −0
Original line number Diff line number Diff line
@@ -78,6 +78,7 @@ enum bpf_arg_type { 
	 * functions that access data on eBPF program stack

	 */

	ARG_PTR_TO_MEM,		/* pointer to valid memory (stack, packet, map value) */

	ARG_PTR_TO_MEM_OR_NULL, /* pointer to valid memory or NULL */

	ARG_PTR_TO_UNINIT_MEM,	/* pointer to memory does not need to be initialized,

				 * helper function must fill all bytes or clear

				 * them in error case.

kernel/bpf/verifier.c

+3 −1
Original line number Diff line number Diff line
@@ -1384,13 +1384,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno, 
		if (type != expected_type)

			goto err_type;

	} else if (arg_type == ARG_PTR_TO_MEM ||

		   arg_type == ARG_PTR_TO_MEM_OR_NULL ||

		   arg_type == ARG_PTR_TO_UNINIT_MEM) {

		expected_type = PTR_TO_STACK;

		/* One exception here. In case function allows for NULL to be

		 * passed in as argument, it's a SCALAR_VALUE type. Final test

		 * happens during stack boundary checking.

		 */

		if (register_is_null(*reg))

		if (register_is_null(*reg) &&

		    arg_type == ARG_PTR_TO_MEM_OR_NULL)

			/* final test in check_stack_boundary() */;

		else if (!type_is_pkt_pointer(type) &&

			 type != PTR_TO_MAP_VALUE &&

kernel/trace/bpf_trace.c

+4 −8
Original line number Diff line number Diff line
@@ -78,16 +78,12 @@ EXPORT_SYMBOL_GPL(trace_call_bpf); 


BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)

{

	int ret = 0;



	if (unlikely(size == 0))

		goto out;

	int ret;



	ret = probe_kernel_read(dst, unsafe_ptr, size);

	if (unlikely(ret < 0))

		memset(dst, 0, size);



 out:

	return ret;

}
@@ -407,7 +403,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto = { 
	.arg2_type	= ARG_CONST_MAP_PTR,

	.arg3_type	= ARG_ANYTHING,

	.arg4_type	= ARG_PTR_TO_MEM,

	.arg5_type	= ARG_CONST_SIZE,

	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,

};



static DEFINE_PER_CPU(struct pt_regs, bpf_pt_regs);
@@ -498,7 +494,7 @@ static const struct bpf_func_proto bpf_probe_read_str_proto = { 
	.gpl_only	= true,

	.ret_type	= RET_INTEGER,

	.arg1_type	= ARG_PTR_TO_UNINIT_MEM,

	.arg2_type	= ARG_CONST_SIZE,

	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,

	.arg3_type	= ARG_ANYTHING,

};
@@ -609,7 +605,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_tp = { 
	.arg2_type	= ARG_CONST_MAP_PTR,

	.arg3_type	= ARG_ANYTHING,

	.arg4_type	= ARG_PTR_TO_MEM,

	.arg5_type	= ARG_CONST_SIZE,

	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,

};



BPF_CALL_3(bpf_get_stackid_tp, void *, tp_buff, struct bpf_map *, map,

net/core/filter.c

+2 −2
Original line number Diff line number Diff line
@@ -1646,9 +1646,9 @@ static const struct bpf_func_proto bpf_csum_diff_proto = { 
	.gpl_only	= false,

	.pkt_access	= true,

	.ret_type	= RET_INTEGER,

	.arg1_type	= ARG_PTR_TO_MEM,

	.arg1_type	= ARG_PTR_TO_MEM_OR_NULL,

	.arg2_type	= ARG_CONST_SIZE_OR_ZERO,

	.arg3_type	= ARG_PTR_TO_MEM,

	.arg3_type	= ARG_PTR_TO_MEM_OR_NULL,

	.arg4_type	= ARG_CONST_SIZE_OR_ZERO,

	.arg5_type	= ARG_ANYTHING,

};

tools/testing/selftests/bpf/test_verifier.c

+106 −7
Original line number Diff line number Diff line
@@ -5631,7 +5631,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size = 0 allowed on NULL",

		"helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_IMM(BPF_REG_1, 0),

			BPF_MOV64_IMM(BPF_REG_2, 0),
@@ -5645,7 +5645,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size > 0 not allowed on NULL",

		"helper access to variable memory: size > 0 not allowed on NULL (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_IMM(BPF_REG_1, 0),

			BPF_MOV64_IMM(BPF_REG_2, 0),
@@ -5663,7 +5663,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size = 0 allowed on != NULL stack pointer",

		"helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
@@ -5680,7 +5680,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size = 0 allowed on != NULL map pointer",

		"helper access to variable memory: size = 0 allowed on != NULL map pointer (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5702,7 +5702,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer",

		"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5727,7 +5727,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size possible = 0 allowed on != NULL map pointer",

		"helper access to variable memory: size possible = 0 allowed on != NULL map pointer (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5750,7 +5750,7 @@ static struct bpf_test tests[] = { 
		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size possible = 0 allowed on != NULL packet pointer",

		"helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,

				    offsetof(struct __sk_buff, data)),
@@ -5771,6 +5771,105 @@ static struct bpf_test tests[] = { 
		.result = ACCEPT,

		.prog_type = BPF_PROG_TYPE_SCHED_CLS,

	},

	{

		"helper access to variable memory: size = 0 not allowed on NULL (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_IMM(BPF_REG_1, 0),

			BPF_MOV64_IMM(BPF_REG_2, 0),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.errstr = "R1 type=inv expected=fp",

		.result = REJECT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size > 0 not allowed on NULL (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_IMM(BPF_REG_1, 0),

			BPF_MOV64_IMM(BPF_REG_2, 1),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.errstr = "R1 type=inv expected=fp",

		.result = REJECT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size = 0 allowed on != NULL stack pointer (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),

			BPF_MOV64_IMM(BPF_REG_2, 0),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.result = ACCEPT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size = 0 allowed on != NULL map pointer (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),

			BPF_LD_MAP_FD(BPF_REG_1, 0),

			BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),

			BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),

			BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),

			BPF_MOV64_IMM(BPF_REG_2, 0),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.fixup_map1 = { 3 },

		.result = ACCEPT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),

			BPF_LD_MAP_FD(BPF_REG_1, 0),

			BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),

			BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),

			BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 0),

			BPF_JMP_IMM(BPF_JGT, BPF_REG_2, 8, 4),

			BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.fixup_map1 = { 3 },

		.result = ACCEPT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: size possible = 0 allowed on != NULL map pointer (!ARG_PTR_TO_MEM_OR_NULL)",

		.insns = {

			BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),

			BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),

			BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),

			BPF_LD_MAP_FD(BPF_REG_1, 0),

			BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),

			BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),

			BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),

			BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 0),

			BPF_JMP_IMM(BPF_JGT, BPF_REG_2, 8, 2),

			BPF_MOV64_IMM(BPF_REG_3, 0),

			BPF_EMIT_CALL(BPF_FUNC_probe_read),

			BPF_EXIT_INSN(),

		},

		.fixup_map1 = { 3 },

		.result = ACCEPT,

		.prog_type = BPF_PROG_TYPE_TRACEPOINT,

	},

	{

		"helper access to variable memory: 8 bytes leak",

		.insns = {

CRA Git | Maintained and supported by SUSTech CRA and CCSE