Skip to content

Commit 107af8e

Browse files
borkmannborkmann
committed
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 9fd29c0 ("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 9c019e2 ("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: Daniel Borkmann <daniel@iogearbox.net>
2 parents f1a8b8e + a60dd35 commit 107af8e

File tree

5 files changed

+116
-18
lines changed

5 files changed

+116
-18
lines changed

‎include/linux/bpf.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ enum bpf_arg_type {
7878
* functions that access data on eBPF program stack
7979
*/
8080
ARG_PTR_TO_MEM, /* pointer to valid memory (stack, packet, map value) */
81+
ARG_PTR_TO_MEM_OR_NULL, /* pointer to valid memory or NULL */
8182
ARG_PTR_TO_UNINIT_MEM, /* pointer to memory does not need to be initialized,
8283
* helper function must fill all bytes or clear
8384
* them in error case.

‎kernel/bpf/verifier.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1384,13 +1384,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
13841384
if (type != expected_type)
13851385
goto err_type;
13861386
} else if (arg_type == ARG_PTR_TO_MEM ||
1387+
arg_type == ARG_PTR_TO_MEM_OR_NULL ||
13871388
arg_type == ARG_PTR_TO_UNINIT_MEM) {
13881389
expected_type = PTR_TO_STACK;
13891390
/* One exception here. In case function allows for NULL to be
13901391
* passed in as argument, it's a SCALAR_VALUE type. Final test
13911392
* happens during stack boundary checking.
13921393
*/
1393-
if (register_is_null(*reg))
1394+
if (register_is_null(*reg) &&
1395+
arg_type == ARG_PTR_TO_MEM_OR_NULL)
13941396
/* final test in check_stack_boundary() */;
13951397
else if (!type_is_pkt_pointer(type) &&
13961398
type != PTR_TO_MAP_VALUE &&

‎kernel/trace/bpf_trace.c

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,12 @@ EXPORT_SYMBOL_GPL(trace_call_bpf);
7878

7979
BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
8080
{
81-
int ret = 0;
82-
83-
if (unlikely(size == 0))
84-
goto out;
81+
int ret;
8582

8683
ret = probe_kernel_read(dst, unsafe_ptr, size);
8784
if (unlikely(ret < 0))
8885
memset(dst, 0, size);
8986

90-
out:
9187
return ret;
9288
}
9389

@@ -407,7 +403,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto = {
407403
.arg2_type = ARG_CONST_MAP_PTR,
408404
.arg3_type = ARG_ANYTHING,
409405
.arg4_type = ARG_PTR_TO_MEM,
410-
.arg5_type = ARG_CONST_SIZE,
406+
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
411407
};
412408

413409
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 = {
498494
.gpl_only = true,
499495
.ret_type = RET_INTEGER,
500496
.arg1_type = ARG_PTR_TO_UNINIT_MEM,
501-
.arg2_type = ARG_CONST_SIZE,
497+
.arg2_type = ARG_CONST_SIZE_OR_ZERO,
502498
.arg3_type = ARG_ANYTHING,
503499
};
504500

@@ -609,7 +605,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_tp = {
609605
.arg2_type = ARG_CONST_MAP_PTR,
610606
.arg3_type = ARG_ANYTHING,
611607
.arg4_type = ARG_PTR_TO_MEM,
612-
.arg5_type = ARG_CONST_SIZE,
608+
.arg5_type = ARG_CONST_SIZE_OR_ZERO,
613609
};
614610

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

‎net/core/filter.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1646,9 +1646,9 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
16461646
.gpl_only = false,
16471647
.pkt_access = true,
16481648
.ret_type = RET_INTEGER,
1649-
.arg1_type = ARG_PTR_TO_MEM,
1649+
.arg1_type = ARG_PTR_TO_MEM_OR_NULL,
16501650
.arg2_type = ARG_CONST_SIZE_OR_ZERO,
1651-
.arg3_type = ARG_PTR_TO_MEM,
1651+
.arg3_type = ARG_PTR_TO_MEM_OR_NULL,
16521652
.arg4_type = ARG_CONST_SIZE_OR_ZERO,
16531653
.arg5_type = ARG_ANYTHING,
16541654
};

‎tools/testing/selftests/bpf/test_verifier.c

Lines changed: 106 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5631,7 +5631,7 @@ static struct bpf_test tests[] = {
56315631
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
56325632
},
56335633
{
5634-
"helper access to variable memory: size = 0 allowed on NULL",
5634+
"helper access to variable memory: size = 0 allowed on NULL (ARG_PTR_TO_MEM_OR_NULL)",
56355635
.insns = {
56365636
BPF_MOV64_IMM(BPF_REG_1, 0),
56375637
BPF_MOV64_IMM(BPF_REG_2, 0),
@@ -5645,7 +5645,7 @@ static struct bpf_test tests[] = {
56455645
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
56465646
},
56475647
{
5648-
"helper access to variable memory: size > 0 not allowed on NULL",
5648+
"helper access to variable memory: size > 0 not allowed on NULL (ARG_PTR_TO_MEM_OR_NULL)",
56495649
.insns = {
56505650
BPF_MOV64_IMM(BPF_REG_1, 0),
56515651
BPF_MOV64_IMM(BPF_REG_2, 0),
@@ -5663,7 +5663,7 @@ static struct bpf_test tests[] = {
56635663
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
56645664
},
56655665
{
5666-
"helper access to variable memory: size = 0 allowed on != NULL stack pointer",
5666+
"helper access to variable memory: size = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL)",
56675667
.insns = {
56685668
BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
56695669
BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
@@ -5680,7 +5680,7 @@ static struct bpf_test tests[] = {
56805680
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
56815681
},
56825682
{
5683-
"helper access to variable memory: size = 0 allowed on != NULL map pointer",
5683+
"helper access to variable memory: size = 0 allowed on != NULL map pointer (ARG_PTR_TO_MEM_OR_NULL)",
56845684
.insns = {
56855685
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
56865686
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5702,7 +5702,7 @@ static struct bpf_test tests[] = {
57025702
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
57035703
},
57045704
{
5705-
"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer",
5705+
"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer (ARG_PTR_TO_MEM_OR_NULL)",
57065706
.insns = {
57075707
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
57085708
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5727,7 +5727,7 @@ static struct bpf_test tests[] = {
57275727
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
57285728
},
57295729
{
5730-
"helper access to variable memory: size possible = 0 allowed on != NULL map pointer",
5730+
"helper access to variable memory: size possible = 0 allowed on != NULL map pointer (ARG_PTR_TO_MEM_OR_NULL)",
57315731
.insns = {
57325732
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
57335733
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
@@ -5750,7 +5750,7 @@ static struct bpf_test tests[] = {
57505750
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
57515751
},
57525752
{
5753-
"helper access to variable memory: size possible = 0 allowed on != NULL packet pointer",
5753+
"helper access to variable memory: size possible = 0 allowed on != NULL packet pointer (ARG_PTR_TO_MEM_OR_NULL)",
57545754
.insns = {
57555755
BPF_LDX_MEM(BPF_W, BPF_REG_6, BPF_REG_1,
57565756
offsetof(struct __sk_buff, data)),
@@ -5771,6 +5771,105 @@ static struct bpf_test tests[] = {
57715771
.result = ACCEPT,
57725772
.prog_type = BPF_PROG_TYPE_SCHED_CLS,
57735773
},
5774+
{
5775+
"helper access to variable memory: size = 0 not allowed on NULL (!ARG_PTR_TO_MEM_OR_NULL)",
5776+
.insns = {
5777+
BPF_MOV64_IMM(BPF_REG_1, 0),
5778+
BPF_MOV64_IMM(BPF_REG_2, 0),
5779+
BPF_MOV64_IMM(BPF_REG_3, 0),
5780+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5781+
BPF_EXIT_INSN(),
5782+
},
5783+
.errstr = "R1 type=inv expected=fp",
5784+
.result = REJECT,
5785+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5786+
},
5787+
{
5788+
"helper access to variable memory: size > 0 not allowed on NULL (!ARG_PTR_TO_MEM_OR_NULL)",
5789+
.insns = {
5790+
BPF_MOV64_IMM(BPF_REG_1, 0),
5791+
BPF_MOV64_IMM(BPF_REG_2, 1),
5792+
BPF_MOV64_IMM(BPF_REG_3, 0),
5793+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5794+
BPF_EXIT_INSN(),
5795+
},
5796+
.errstr = "R1 type=inv expected=fp",
5797+
.result = REJECT,
5798+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5799+
},
5800+
{
5801+
"helper access to variable memory: size = 0 allowed on != NULL stack pointer (!ARG_PTR_TO_MEM_OR_NULL)",
5802+
.insns = {
5803+
BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
5804+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
5805+
BPF_MOV64_IMM(BPF_REG_2, 0),
5806+
BPF_MOV64_IMM(BPF_REG_3, 0),
5807+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5808+
BPF_EXIT_INSN(),
5809+
},
5810+
.result = ACCEPT,
5811+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5812+
},
5813+
{
5814+
"helper access to variable memory: size = 0 allowed on != NULL map pointer (!ARG_PTR_TO_MEM_OR_NULL)",
5815+
.insns = {
5816+
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
5817+
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
5818+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
5819+
BPF_LD_MAP_FD(BPF_REG_1, 0),
5820+
BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
5821+
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4),
5822+
BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
5823+
BPF_MOV64_IMM(BPF_REG_2, 0),
5824+
BPF_MOV64_IMM(BPF_REG_3, 0),
5825+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5826+
BPF_EXIT_INSN(),
5827+
},
5828+
.fixup_map1 = { 3 },
5829+
.result = ACCEPT,
5830+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5831+
},
5832+
{
5833+
"helper access to variable memory: size possible = 0 allowed on != NULL stack pointer (!ARG_PTR_TO_MEM_OR_NULL)",
5834+
.insns = {
5835+
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
5836+
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
5837+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
5838+
BPF_LD_MAP_FD(BPF_REG_1, 0),
5839+
BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
5840+
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
5841+
BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 0),
5842+
BPF_JMP_IMM(BPF_JGT, BPF_REG_2, 8, 4),
5843+
BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
5844+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
5845+
BPF_MOV64_IMM(BPF_REG_3, 0),
5846+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5847+
BPF_EXIT_INSN(),
5848+
},
5849+
.fixup_map1 = { 3 },
5850+
.result = ACCEPT,
5851+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5852+
},
5853+
{
5854+
"helper access to variable memory: size possible = 0 allowed on != NULL map pointer (!ARG_PTR_TO_MEM_OR_NULL)",
5855+
.insns = {
5856+
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
5857+
BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
5858+
BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
5859+
BPF_LD_MAP_FD(BPF_REG_1, 0),
5860+
BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
5861+
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),
5862+
BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
5863+
BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_0, 0),
5864+
BPF_JMP_IMM(BPF_JGT, BPF_REG_2, 8, 2),
5865+
BPF_MOV64_IMM(BPF_REG_3, 0),
5866+
BPF_EMIT_CALL(BPF_FUNC_probe_read),
5867+
BPF_EXIT_INSN(),
5868+
},
5869+
.fixup_map1 = { 3 },
5870+
.result = ACCEPT,
5871+
.prog_type = BPF_PROG_TYPE_TRACEPOINT,
5872+
},
57745873
{
57755874
"helper access to variable memory: 8 bytes leak",
57765875
.insns = {

0 commit comments

Comments
 (0)