Re: [RFC bpf-next 2/9] bpf: Add uprobe faultable user copy kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] bpf: missing CAP_SYS_ADMIN check in user_mem_kfunc_filter()
- [High] bpf: hardened usercopy BUG() in bpf_probe_write_user_faultable()
- [Medium] bpf: missing prog->sleepable check in user_mem_kfunc_filter()
- [Low] bpf: unused enum user_mem_kfunc_id in kernel/bpf/helpers.c
--

commit bbd6dece6abea0ce7dfa98ce448c7aae57de64d1
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Add uprobe faultable user copy kfunc

This patch adds the bpf_probe_write_user_faultable() sleepable kfunc to copy
data to user memory with page faults enabled. It registers the kfunc for
BPF_PROG_TYPE_KPROBE programs and filters access with
LOCKDOWN_BPF_WRITE_USER.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 648abe1ba63ae..4d9e9423826a9 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -3738,6 +3739,36 @@ __bpf_kfunc int bpf_copy_from_user_mm(void *dst, u32 dst__sz,
>  	return ret;
>  }
>  
> +/**
> + * bpf_probe_write_user_faultable() - Copy data to userspace with page faults
> + * enabled
> + * @unsafe_dst__ign: Destination address in the current task's address space.
> + * @src__srcptr:     Source buffer in BPF-accessible memory.
> + * @src__sz:         Number of bytes to copy.
> + *
> + * Copies data to userspace, allowing page faults to resolve the destination
> + * mapping. A failed copy may have written a prefix of the source buffer.
> + *
> + * Return: 0 on success, -EFAULT if the copy is incomplete, or -EPERM if the
> + * current task cannot safely access userspace.
> + */
> +__bpf_kfunc int bpf_probe_write_user_faultable(void __user *unsafe_dst__ign,
> +					      const void *src__srcptr,
> +					      u32 src__sz)
> +{
> +	if (unlikely(in_interrupt() ||
> +		     current->flags & (PF_KTHREAD | PF_EXITING)))
> +		return -EPERM;
> +	if (unlikely(!nmi_uaccess_okay()))
> +		return -EPERM;
> +	if (unlikely(!current->mm))
> +		return -EFAULT;
> +	if (copy_to_user(unsafe_dst__ign, src__srcptr, src__sz))
> +		return -EFAULT;

[Severity: High]
Can calling copy_to_user() directly on src__srcptr trigger a hardened
usercopy bug when a BPF program passes a trusted BTF object pointer?

In kernel/bpf/verifier.c, get_kfunc_arg_type() classifies src__srcptr paired
with src__sz as ARG_PTR_TO_MEM | MEM_RDONLY. During verification,
check_reg_type() permits mem_types, which includes:

kernel/bpf/verifier.c:
	PTR_TO_BTF_ID | PTR_TRUSTED,

and check_helper_mem_access() allows BPF_READ access on scalar fields of
trusted BTF objects (such as &task->pid or task->comm from
bpf_get_current_task_btf()).

At runtime, copy_to_user() calls check_copy_size(src__srcptr, src__sz, true)
-> __check_object_size() -> __check_heap_object() in mm/slub.c:

mm/slub.c:__check_heap_object() {
	...
	usercopy_abort("SLUB object", s->name, to_user, offset, n);
}

When src__srcptr points to a field in a dedicated slab cache outside its
whitelisted usercopy region (for example task_struct_cachep, mm_cachep,
files_cachep, or fs_cachep), wouldn't __check_heap_object() call
usercopy_abort() and panic the kernel when CONFIG_HARDENED_USERCOPY is
enabled?

> +
> +	return 0;
> +}
> +
>  /**
>   * bpf_copy_from_user_mm_str() - Copy a string from an address space
>   * @dst:             Destination address, in kernel space. This buffer must be
> @@ -5139,6 +5170,31 @@ static const struct btf_kfunc_id_set generic_kfunc_set = {
>  	.set   = &generic_btf_ids,
>  };
>  
> +BTF_KFUNCS_START(user_mem_btf_ids)
> +BTF_ID_FLAGS(func, bpf_probe_write_user_faultable, KF_SLEEPABLE | KF_PERFMON)
> +BTF_KFUNCS_END(user_mem_btf_ids)
> +
> +enum user_mem_kfunc_id {
> +	KF_bpf_probe_write_user_faultable,
> +};

[Severity: Low]
Is enum user_mem_kfunc_id left over from an earlier version of the patch?
Since user_mem_kfunc_filter() compares kfunc_id against
bpf_probe_write_user_faultable_btf_id[0], neither enum user_mem_kfunc_id nor
KF_bpf_probe_write_user_faultable is used anywhere in kernel/bpf/helpers.c.

> +
> +BTF_ID_LIST_SINGLE(bpf_probe_write_user_faultable_btf_id, func,
> +		   bpf_probe_write_user_faultable)
> +
> +static int user_mem_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> +	if (kfunc_id == bpf_probe_write_user_faultable_btf_id[0])
> +		return security_locked_down(LOCKDOWN_BPF_WRITE_USER);

[Severity: Critical]
Should user_mem_kfunc_filter() also verify that the program has
CAP_SYS_ADMIN via bpf_token_capable(prog->aux->token, CAP_SYS_ADMIN)?

In user_mem_btf_ids, bpf_probe_write_user_faultable is marked with
KF_PERFMON, which in check_kfunc_call() in kernel/bpf/verifier.c only checks
env->allow_ptr_leaks (satisfied by CAP_PERFMON). Meanwhile, for the existing
bpf_probe_write_user() helper, bpf_tracing_func_proto() in
kernel/trace/bpf_trace.c enforces CAP_SYS_ADMIN in addition to lockdown:

kernel/trace/bpf_trace.c:bpf_tracing_func_proto() {
	...
	if (!bpf_token_capable(prog->aux->token, CAP_SYS_ADMIN))
		return NULL;

	switch (func_id) {
	case BPF_FUNC_probe_write_user:
		return security_locked_down(LOCKDOWN_BPF_WRITE_USER) < 0 ?
		       NULL : &bpf_probe_write_user_proto;
	...
}

And include/uapi/linux/capability.h documents this boundary under CAP_BPF:

	* CAP_SYS_ADMIN is required to use bpf_probe_write_user.

Without a bpf_token_capable(prog->aux->token, CAP_SYS_ADMIN) check in
user_mem_kfunc_filter(), couldn't a process or container granted only
CAP_BPF and CAP_PERFMON attach a sleepable uprobe or uprobe_multi program to
a shared library and overwrite memory in privileged processes?

> +
> +	return 0;
> +}

[Severity: Medium]
Does user_mem_kfunc_filter() also need to check prog->sleepable to restrict
bpf_probe_write_user_faultable() to sleepable uprobes as described in the
commit message?

Because BPF_PROG_TYPE_KPROBE is shared by kernel kprobes and uprobes, the
uprobe-only restriction for sleepable programs is enforced at attach time by
checking prog->sleepable in __perf_event_set_bpf_prog() and
bpf_kprobe_multi_link_attach().

In check_kfunc_call() in kernel/bpf/verifier.c, KF_SLEEPABLE checks
in_sleepable_context(env) rather than prog->sleepable. When a non-sleepable
BPF_PROG_TYPE_KPROBE program (prog->sleepable == false) registers a callback
via bpf_task_work_schedule_signal(), bpf_task_work_schedule_resume(), or
bpf_wq_set_callback(), push_callback_call() marks the callback state as
sleepable via is_async_cb_sleepable():

kernel/bpf/verifier.c:is_async_cb_sleepable() {
	/* bpf_wq and bpf_task_work callbacks are always sleepable. */
	if (bpf_pseudo_kfunc_call(insn) && insn->off == 0 &&
	    (is_bpf_wq_set_callback_kfunc(insn->imm) || is_task_work_add_kfunc(insn->imm)))
		return true;
	return false;
}

Without checking !prog->sleepable in user_mem_kfunc_filter(), wouldn't a
non-sleepable kernel kprobe, kretprobe, or kprobe_multi program be able to
call bpf_probe_write_user_faultable() from a bpf_task_work callback?

> +
> +static const struct btf_kfunc_id_set user_mem_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &user_mem_btf_ids,
> +	.filter = user_mem_kfunc_filter,
> +};

[ ... ]

> @@ -5266,6 +5322,7 @@ static int __init kfunc_init(void)
>  	};
>  
>  	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &generic_kfunc_set);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_KPROBE, &user_mem_kfunc_set);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195759.119421-1-jolsa@kernel.org?part=2