Misc bug fixes - part 1

kernel/bpf/btf.c static const struct bpf_raw_tp_null_args raw_tp_null_args[] = { · line 6722

Quoted 11 lines

kernel/bpf/btf.c +3 -0

index da36d4b9d31ab..d6d243c262ea5 100644
@@ -6717,6 +6717,9 @@ static const struct bpf_raw_tp_null_args raw_tp_null_args[] = {
 	{ "rxrpc_resend", 0x10 },
 	{ "rxrpc_tq", 0x10 },
 	{ "rxrpc_client", 0x1 },
+	/* signal */
+	{ "signal_generate", 0x20 },
+	{ "signal_deliver", 0x20 },

                                   ^^^^

Is signal_deliver's 0x20 mask too strict for its actual call sites?

Looking at the two call sites for trace_signal_deliver():

  kernel/signal.c (do_coredump):
    trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO,
                         &sighand->action[SIGKILL-1]);

  kernel/signal.c (get_signal):
    trace_signal_deliver(signr, &ksig->info, ka);

SEND_SIG_PRIV is never passed to this tracepoint. SEND_SIG_NOINFO is zero (NULL), and &ksig->info is always a real pointer filled by dequeue_synchronous_signal() or dequeue_signal() just above the call.

SEND_SIG_PRIV == 1 only reaches trace_signal_generate() from kernel/signal.c:__send_signal_locked().

If that's the case, would 0x10 (PTR_MAYBE_NULL) be both sufficient and less restrictive for signal_deliver? With 0x10 the verifier would still force a NULL check to catch SEND_SIG_NOINFO, but after the check a tp_btf program could read info->si_code and other fields. With 0x20 the register becomes SCALAR_VALUE immediately and check_mem_access() rejects all dereferencing with "R1 invalid mem access 'scalar'".

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch prevents bpf_tail_call within a callback to fix the 'callback unexpected regs' verifier bug, but misses that BPF_LD_ABS and BPF_LD_IND trigger the exact same bug via the same prepare_func_exit() path. --

commit a25ccc5cdb72d9330414949c17ecac5156595510
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Reject tail calls directly from callback frames

A tail call from a non-zero frame is modeled as a return from that frame, synthesizing R0 and calling prepare_func_exit() for the taken branch. When the current frame is a synchronous callback, prepare_func_exit() enforces the callback return-value contract and marks R0 precise, triggering the "callback unexpected regs" verifier warning during precision backtracking. This patch rejects a tail call made directly by a callback before constructing the inconsistent return state.

Quoted 9 lines

kernel/bpf/verifier.c +1 -0

index 7d8ddb1bee00c..f540279ff4abb 100644
@@ -11228,6 +11228,17 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		if (env->cur_state->curframe) {
 			struct bpf_verifier_state *branch;
 
+			/*

kernel/bpf/verifier.c static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn · line 11241

Quoted 10 lines

kernel/bpf/verifier.c +10 -0

+			 * A taken tail call is modeled as a return from the current
+			 * frame. A callback frame cannot be left that way because
+			 * prepare_func_exit() would apply its return contract to the
+			 * unknown R0 synthesized below. Stack-depth validation rejects
+			 * this construct anyway.
+			 */
+			if (cur_func(env)->in_callback_fn) {
+				verbose(env, "cannot tail call within callback\n");
+				return -EINVAL;
+			}

[Severity: High]
This isn't a bug introduced by this patch, but doesn't BPF_LD_ABS inside a subprog trigger the exact same bug via the same prepare_func_exit() path?

Looking at check_ld_abs() in kernel/bpf/verifier.c:

    if (env->cur_state->curframe) {
        struct bpf_verifier_state *branch;

        mark_reg_scratched(env, BPF_REG_0);
        branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
        if (IS_ERR(branch))
            return PTR_ERR(branch);
        mark_reg_known_zero(env, regs, BPF_REG_0);
        err = prepare_func_exit(env, &env->insn_idx);
        if (err)
            return err;
        env->insn_idx--;
    }

If a CAP_BPF user loads a SCHED_CLS program (where BPF_LD_ABS is allowed), invokes bpf_loop(), passes PTR_TO_CTX through the stack to the callback_ctx argument, loads the context into R6 inside the callback, and executes BPF_LD_ABS, it appears check_ld_abs() will synthesize R0 and call prepare_func_exit().

Since the callback frame lacks instruction history for R0, won't precision backtracking fail and hit the exact same "callback unexpected regs" verifier warning? Does check_ld_abs() need a similar rejection for callbacks?

Quoted 3 lines

kernel/bpf/verifier.c +0 -0

 			mark_reg_scratched(env, BPF_REG_0);
 			branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
 			if (IS_ERR(branch))

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The restriction against calling lock operations inside rbtree callbacks can be bypassed by nesting callbacks or calling subprograms, because in_rbtree_lock_required_cb() only checks the immediate top frame. --

commit c5914f67e15078bc12af13429620d2a2a4de2b11
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Reject resilient lock operations in rbtree callbacks

This patch attempts to enforce the root's lock requirement for resilient locks in rbtree callbacks. It aims to prevent use-after-free conditions when nodes are removed and freed by other CPUs during the in-progress tree walk.

kernel/bpf/verifier.c static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me · line 13244

Quoted 9 lines

kernel/bpf/verifier.c +1 -0

index f540279ff4abb..32d31fa67036b 100644
@@ -13241,6 +13241,11 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
 		{
 			int flags = PROCESS_RES_LOCK;
 
+			if (in_rbtree_lock_required_cb(env)) {

                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does in_rbtree_lock_required_cb() fully protect against this bypass?

Looking at in_rbtree_lock_required_cb(), it only checks the immediate top frame:

kernel/bpf/verifier.c:in_rbtree_lock_required_cb() {
    ...
	callee = state->frame[state->curframe];

	if (!callee->in_callback_fn)
		return false;

	kfunc_btf_id = insn[callee->callsite].imm;
	return is_rbtree_lock_required_kfunc(kfunc_btf_id);
}

If a BPF program calls a custom rbtree comparison callback, and inside that callback it calls a static subprogram (or a nested callback like bpf_loop()), would state->curframe's in_callback_fn be false (or its kfunc_btf_id not match the rbtree add kfunc)?

Could a program use this nested frame to successfully unlock the resilient lock via a global map value (BPF_PSEUDO_MAP_VALUE), temporarily dropping the lock and allowing another CPU to concurrently remove and free the nodes being traversed by bpf_rbtree_add(), leading to a Use-After-Free?

Quoted 7 lines

kernel/bpf/verifier.c +4 -0

+				verbose(env, "can't res_spin_{lock,unlock} in rbtree cb\n");
+				return -EACCES;
+			}
+
 			if (reg->type != PTR_TO_MAP_VALUE && reg->type != (PTR_TO_BTF_ID | MEM_ALLOC)) {
 				verbose(env, "%s doesn't point to map value or allocated object\n",
 					reg_arg_name(env, argno));

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] bpf_btf_find_by_name_kind_proto lacks .might_sleep = true, allowing arbitrary FD injection via softirq. --

commit 1d16e876066d61c09bcc6698ca694cdf8336d52c
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Mark syscall helpers as sleepable

This commit adds the .might_sleep = true constraint to the bpf_sys_bpf and bpf_sys_close helper prototypes. This ensures the verifier rejects them from non-sleepable contexts, such as bpf_timer callbacks registered within a BPF_PROG_TYPE_SYSCALL program.

Quoted 15 lines

kernel/bpf/syscall.c +1 -0

index 6874ba1424af0..c7bc9ba9b331f 100644
@@ -6568,6 +6568,7 @@ EXPORT_SYMBOL_NS(kern_sys_bpf, "BPF_INTERNAL");
 static const struct bpf_func_proto bpf_sys_bpf_proto = {
 	.func		= bpf_sys_bpf,
 	.gpl_only	= false,
+	.might_sleep	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_ANYTHING,
 	.arg2_type	= ARG_PTR_TO_MEM | MEM_RDONLY,

kernel/bpf/syscall.c +0 -0

 	.arg3_type	= ARG_MEM_SIZE,
 };

kernel/bpf/syscall.c BPF_CALL_1(bpf_sys_close, u32, fd) · line 6600

Quoted 7 lines

kernel/bpf/syscall.c +1 -0

@@ -6593,6 +6594,7 @@ BPF_CALL_1(bpf_sys_close, u32, fd)
 static const struct bpf_func_proto bpf_sys_close_proto = {
 	.func		= bpf_sys_close,
 	.gpl_only	= false,
+	.might_sleep	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_ANYTHING,
 };

[Severity: High]
This isn't a bug introduced by this patch, but does bpf_btf_find_by_name_kind_proto also need to be marked as sleepable?

Looking at syscall_prog_func_proto() in kernel/bpf/syscall.c, bpf_btf_find_by_name_kind is also exposed to syscall programs:

    case BPF_FUNC_btf_find_by_name_kind:
        return &bpf_btf_find_by_name_kind_proto;

Because bpf_btf_find_by_name_kind_proto in kernel/bpf/btf.c lacks the .might_sleep = true constraint, the verifier will permit its use inside a non-sleepable bpf_timer callback registered by a syscall program.

When the timer fires in softirq context, bpf_btf_find_by_name_kind() calls __btf_new_fd(). This attempts to allocate a file descriptor with GFP_KERNEL and injects the new file descriptor into the current->files struct of whatever arbitrary process was interrupted by the timer.

Should bpf_btf_find_by_name_kind_proto be updated as well to prevent this cross-process file descriptor injection and scheduling-while-atomic issue?