bpf_for_each_map_elem() and bpf_timer_set_callback() helpers are relying on PTR_TO_FUNC infra in the verifier to validate addresses to subprograms and pass them into the helpers as function callbacks. In case of bpf_for_each_map_elem() the callback is invoked synchronously and the verifier treats it as a normal subprogram call by adding another bpf_func_state and new frame in __check_func_call(). bpf_timer_set_callback() doesn't invoke the callback directly. The subprogram will be called asynchronously from bpf_timer_cb(). Teach the verifier to validate such async callbacks as special kind of jump by pushing verifier state into stack and let pop_stack() process it. Special care needs to be taken during state pruning. The call insn doing bpf_timer_set_callback has to be a prune_point. Otherwise short timer callbacks might not have prune points in front of bpf_timer_set_callback() which means is_state_visited() will be called after this call insn is processed in __check_func_call(). Which means that another async_cb state will be pushed to be walked later and the verifier will eventually hit BPF_COMPLEXITY_LIMIT_JMP_SEQ limit. Since push_async_cb() looks like another push_stack() branch the infinite loop detection will trigger false positive. To recognize this case mark such states as in_async_callback_fn. To distinguish infinite loop in async callback vs the same callback called with different arguments for different map and timer add async_entry_cnt to bpf_func_state. Enforce return zero from async callbacks.
Showing the changes to kernel (2 of 3 files). Show all
Unified · Split
| @@ -1043,7 +1043,6 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer) · 1042 unmodified lines | |||
| 1043 | void *callback_fn; | 1043 | void *callback_fn; |
| 1044 | void *key; | 1044 | void *key; |
| 1045 | u32 idx; | 1045 | u32 idx; |
| 1046 | int ret; | ||
| 1047 | 1046 | ||
| 1048 | callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held()); | 1047 | callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held()); |
| 1049 | if (!callback_fn) | 1048 | if (!callback_fn) |
| @@ -1066,10 +1065,9 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer) · 16 unmodified lines | |||
| 1066 | key = value - round_up(map->key_size, 8); | 1065 | key = value - round_up(map->key_size, 8); |
| 1067 | } | 1066 | } |
| 1068 | 1067 | ||
| 1069 | ret = BPF_CAST_CALL(callback_fn)((u64)(long)map, | 1068 | BPF_CAST_CALL(callback_fn)((u64)(long)map, (u64)(long)key, |
| 1070 | (u64)(long)key, | 1069 | (u64)(long)value, 0, 0); |
| 1071 | (u64)(long)value, 0, 0); | 1070 | /* The verifier checked that return value is zero. */ |
| 1072 | WARN_ON(ret != 0); /* Next patch moves this check into the verifier */ | ||
| 1073 | 1071 | ||
| 1074 | this_cpu_write(hrtimer_running, NULL); | 1072 | this_cpu_write(hrtimer_running, NULL); |
| 1075 | out: | 1073 | out: |
| @@ -735,6 +735,10 @@ static void print_verifier_state(struct bpf_verifier_env *env, · 734 unmodified lines | |||
| 735 | if (state->refs[i].id) | 735 | if (state->refs[i].id) |
| 736 | verbose(env, ",%d", state->refs[i].id); | 736 | verbose(env, ",%d", state->refs[i].id); |
| 737 | } | 737 | } |
| 738 | if (state->in_callback_fn) | ||
| 739 | verbose(env, " cb"); | ||
| 740 | if (state->in_async_callback_fn) | ||
| 741 | verbose(env, " async_cb"); | ||
| 738 | verbose(env, "\n"); | 742 | verbose(env, "\n"); |
| 739 | } | 743 | } |
| 740 | 744 | ||
| @@ -1527,6 +1531,54 @@ static void init_func_state(struct bpf_verifier_env *env, · 786 unmodified lines | |||
| 1527 | init_reg_state(env, state); | 1531 | init_reg_state(env, state); |
| 1528 | } | 1532 | } |
| 1529 | 1533 | ||
| 1534 | /* Similar to push_stack(), but for async callbacks */ | ||
| 1535 | static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env, | ||
| 1536 | int insn_idx, int prev_insn_idx, | ||
| 1537 | int subprog) | ||
| 1538 | { | ||
| 1539 | struct bpf_verifier_stack_elem *elem; | ||
| 1540 | struct bpf_func_state *frame; | ||
| 1541 | |||
| 1542 | elem = kzalloc(sizeof(struct bpf_verifier_stack_elem), GFP_KERNEL); | ||
| 1543 | if (!elem) | ||
| 1544 | goto err; | ||
| 1545 | |||
| 1546 | elem->insn_idx = insn_idx; | ||
| 1547 | elem->prev_insn_idx = prev_insn_idx; | ||
| 1548 | elem->next = env->head; | ||
| 1549 | elem->log_pos = env->log.len_used; | ||
| 1550 | env->head = elem; | ||
| 1551 | env->stack_size++; | ||
| 1552 | if (env->stack_size > BPF_COMPLEXITY_LIMIT_JMP_SEQ) { | ||
| 1553 | verbose(env, | ||
| 1554 | "The sequence of %d jumps is too complex for async cb.\n", | ||
| 1555 | env->stack_size); | ||
| 1556 | goto err; | ||
| 1557 | } | ||
| 1558 | /* Unlike push_stack() do not copy_verifier_state(). | ||
| 1559 | * The caller state doesn't matter. | ||
| 1560 | * This is async callback. It starts in a fresh stack. | ||
| 1561 | * Initialize it similar to do_check_common(). | ||
| 1562 | */ | ||
| 1563 | elem->st.branches = 1; | ||
| 1564 | frame = kzalloc(sizeof(*frame), GFP_KERNEL); | ||
| 1565 | if (!frame) | ||
| 1566 | goto err; | ||
| 1567 | init_func_state(env, frame, | ||
| 1568 | BPF_MAIN_FUNC /* callsite */, | ||
| 1569 | 0 /* frameno within this callchain */, | ||
| 1570 | subprog /* subprog number within this prog */); | ||
| 1571 | elem->st.frame[0] = frame; | ||
| 1572 | return &elem->st; | ||
| 1573 | err: | ||
| 1574 | free_verifier_state(env->cur_state, true); | ||
| 1575 | env->cur_state = NULL; | ||
| 1576 | /* pop all elements and return */ | ||
| 1577 | while (!pop_stack(env, NULL, NULL, false)); | ||
| 1578 | return NULL; | ||
| 1579 | } | ||
| 1580 | |||
| 1581 | |||
| 1530 | enum reg_arg_type { | 1582 | enum reg_arg_type { |
| 1531 | SRC_OP, /* register is used as source operand */ | 1583 | SRC_OP, /* register is used as source operand */ |
| 1532 | DST_OP, /* register is used as destination operand */ | 1584 | DST_OP, /* register is used as destination operand */ |
| @@ -5704,6 +5756,30 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn · 4171 unmodified lines | |||
| 5704 | } | 5756 | } |
| 5705 | } | 5757 | } |
| 5706 | 5758 | ||
| 5759 | if (insn->code == (BPF_JMP | BPF_CALL) && | ||
| 5760 | insn->imm == BPF_FUNC_timer_set_callback) { | ||
| 5761 | struct bpf_verifier_state *async_cb; | ||
| 5762 | |||
| 5763 | /* there is no real recursion here. timer callbacks are async */ | ||
| 5764 | async_cb = push_async_cb(env, env->subprog_info[subprog].start, | ||
| 5765 | *insn_idx, subprog); | ||
| 5766 | if (!async_cb) | ||
| 5767 | return -EFAULT; | ||
| 5768 | callee = async_cb->frame[0]; | ||
| 5769 | callee->async_entry_cnt = caller->async_entry_cnt + 1; | ||
| 5770 | |||
| 5771 | /* Convert bpf_timer_set_callback() args into timer callback args */ | ||
| 5772 | err = set_callee_state_cb(env, caller, callee, *insn_idx); | ||
| 5773 | if (err) | ||
| 5774 | return err; | ||
| 5775 | |||
| 5776 | clear_caller_saved_regs(env, caller->regs); | ||
| 5777 | mark_reg_unknown(env, caller->regs, BPF_REG_0); | ||
| 5778 | caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; | ||
| 5779 | /* continue with next insn after call */ | ||
| 5780 | return 0; | ||
| 5781 | } | ||
| 5782 | |||
| 5707 | callee = kzalloc(sizeof(*callee), GFP_KERNEL); | 5783 | callee = kzalloc(sizeof(*callee), GFP_KERNEL); |
| 5708 | if (!callee) | 5784 | if (!callee) |
| 5709 | return -ENOMEM; | 5785 | return -ENOMEM; |
| @@ -5856,6 +5932,7 @@ static int set_timer_callback_state(struct bpf_verifier_env *env, · 146 unmodified lines | |||
| 5856 | /* unused */ | 5932 | /* unused */ |
| 5857 | __mark_reg_not_init(env, &callee->regs[BPF_REG_4]); | 5933 | __mark_reg_not_init(env, &callee->regs[BPF_REG_4]); |
| 5858 | __mark_reg_not_init(env, &callee->regs[BPF_REG_5]); | 5934 | __mark_reg_not_init(env, &callee->regs[BPF_REG_5]); |
| 5935 | callee->in_async_callback_fn = true; | ||
| 5859 | return 0; | 5936 | return 0; |
| 5860 | } | 5937 | } |
| 5861 | 5938 | ||
| @@ -9224,7 +9301,8 @@ static int check_return_code(struct bpf_verifier_env *env) · 3362 unmodified lines | |||
| 9224 | struct tnum range = tnum_range(0, 1); | 9301 | struct tnum range = tnum_range(0, 1); |
| 9225 | enum bpf_prog_type prog_type = resolve_prog_type(env->prog); | 9302 | enum bpf_prog_type prog_type = resolve_prog_type(env->prog); |
| 9226 | int err; | 9303 | int err; |
| 9227 | const bool is_subprog = env->cur_state->frame[0]->subprogno; | 9304 | struct bpf_func_state *frame = env->cur_state->frame[0]; |
| 9305 | const bool is_subprog = frame->subprogno; | ||
| 9228 | 9306 | ||
| 9229 | /* LSM and struct_ops func-ptr's return type could be "void" */ | 9307 | /* LSM and struct_ops func-ptr's return type could be "void" */ |
| 9230 | if (!is_subprog && | 9308 | if (!is_subprog && |
| @@ -9249,6 +9327,22 @@ static int check_return_code(struct bpf_verifier_env *env) · 18 unmodified lines | |||
| 9249 | } | 9327 | } |
| 9250 | 9328 | ||
| 9251 | reg = cur_regs(env) + BPF_REG_0; | 9329 | reg = cur_regs(env) + BPF_REG_0; |
| 9330 | |||
| 9331 | if (frame->in_async_callback_fn) { | ||
| 9332 | /* enforce return zero from async callbacks like timer */ | ||
| 9333 | if (reg->type != SCALAR_VALUE) { | ||
| 9334 | verbose(env, "In async callback the register R0 is not a known value (%s)\n", | ||
| 9335 | reg_type_str[reg->type]); | ||
| 9336 | return -EINVAL; | ||
| 9337 | } | ||
| 9338 | |||
| 9339 | if (!tnum_in(tnum_const(0), reg->var_off)) { | ||
| 9340 | verbose_invalid_scalar(env, reg, &range, "async callback", "R0"); | ||
| 9341 | return -EINVAL; | ||
| 9342 | } | ||
| 9343 | return 0; | ||
| 9344 | } | ||
| 9345 | |||
| 9252 | if (is_subprog) { | 9346 | if (is_subprog) { |
| 9253 | if (reg->type != SCALAR_VALUE) { | 9347 | if (reg->type != SCALAR_VALUE) { |
| 9254 | verbose(env, "At subprogram exit the register R0 is not a scalar value (%s)\n", | 9348 | verbose(env, "At subprogram exit the register R0 is not a scalar value (%s)\n", |
| @@ -9496,6 +9590,13 @@ static int visit_insn(int t, int insn_cnt, struct bpf_verifier_env *env) · 241 unmodified lines | |||
| 9496 | return DONE_EXPLORING; | 9590 | return DONE_EXPLORING; |
| 9497 | 9591 | ||
| 9498 | case BPF_CALL: | 9592 | case BPF_CALL: |
| 9593 | if (insns[t].imm == BPF_FUNC_timer_set_callback) | ||
| 9594 | /* Mark this call insn to trigger is_state_visited() check | ||
| 9595 | * before call itself is processed by __check_func_call(). | ||
| 9596 | * Otherwise new async state will be pushed for further | ||
| 9597 | * exploration. | ||
| 9598 | */ | ||
| 9599 | init_explored_state(env, t); | ||
| 9499 | return visit_func_call_insn(t, insn_cnt, insns, env, | 9600 | return visit_func_call_insn(t, insn_cnt, insns, env, |
| 9500 | insns[t].src_reg == BPF_PSEUDO_CALL); | 9601 | insns[t].src_reg == BPF_PSEUDO_CALL); |
| 9501 | 9602 | ||
| @@ -10503,9 +10604,25 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx) · 1001 unmodified lines | |||
| 10503 | states_cnt++; | 10604 | states_cnt++; |
| 10504 | if (sl->state.insn_idx != insn_idx) | 10605 | if (sl->state.insn_idx != insn_idx) |
| 10505 | goto next; | 10606 | goto next; |
| 10607 | |||
| 10506 | if (sl->state.branches) { | 10608 | if (sl->state.branches) { |
| 10507 | if (states_maybe_looping(&sl->state, cur) && | 10609 | struct bpf_func_state *frame = sl->state.frame[sl->state.curframe]; |
| 10508 | states_equal(env, &sl->state, cur)) { | 10610 | |
| 10611 | if (frame->in_async_callback_fn && | ||
| 10612 | frame->async_entry_cnt != cur->frame[cur->curframe]->async_entry_cnt) { | ||
| 10613 | /* Different async_entry_cnt means that the verifier is | ||
| 10614 | * processing another entry into async callback. | ||
| 10615 | * Seeing the same state is not an indication of infinite | ||
| 10616 | * loop or infinite recursion. | ||
| 10617 | * But finding the same state doesn't mean that it's safe | ||
| 10618 | * to stop processing the current state. The previous state | ||
| 10619 | * hasn't yet reached bpf_exit, since state.branches > 0. | ||
| 10620 | * Checking in_async_callback_fn alone is not enough either. | ||
| 10621 | * Since the verifier still needs to catch infinite loops | ||
| 10622 | * inside async callbacks. | ||
| 10623 | */ | ||
| 10624 | } else if (states_maybe_looping(&sl->state, cur) && | ||
| 10625 | states_equal(env, &sl->state, cur)) { | ||
| 10509 | verbose_linfo(env, insn_idx, "; "); | 10626 | verbose_linfo(env, insn_idx, "; "); |
| 10510 | verbose(env, "infinite loop detected at insn %d\n", insn_idx); | 10627 | verbose(env, "infinite loop detected at insn %d\n", insn_idx); |
| 10511 | return -EINVAL; | 10628 | return -EINVAL; |