Re: [PATCH v2 2/4] net: filter: add sk_attach_filter_kern() function

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] sk_attach_filter_kern() compiles stored classic BPF
  instructions in whatever task happens to call it.
- [Low] sk_attach_filter_kern() (net/core/filter.c:1570-1590) builds its
  program with bpf_prog_create() and publishes it in sk->sk_filter…

--

net: filter: add sk_attach_filter_kern() function

This adds sk_attach_filter_kern(). It builds a classic BPF socket filter
from a kernel-resident sock_fprog_kern using bpf_prog_create(), then
attaches it with __sk_attach_prog(). The kernel can then reinstall a
stored filter without reading a user buffer or file descriptor again.

[Severity: Low]

bpf_prog_create() is documented as creating an unattached filter. It
drops the original program on purpose:

net/core/filter.c:bpf_prog_create() {
    ...
	/* Since unattached filters are not copied back to user
	 * space through sk_get_filter(), we do not need to hold
	 * a copy here, and can spare us the work.
	 */
	fp->orig_prog = NULL;
    ...
}

With this new export, that program ends up in sk->sk_filter. Can a
filter attached this way still be read back?

When orig_prog is NULL, sk_get_filter() returns -EACCES. Its comment
says this only happens for eBPF programs:

net/core/filter.c:sk_get_filter() {
    ...
	/* We're copying the filter that has been originally attached,
	 * so no conversion/decode needed anymore. eBPF programs that
	 * have no original program cannot be dumped through this.
	 */
	ret = -EACCES;
	fprog = filter->prog->orig_prog;
	if (!fprog)
		goto out;
    ...
}

In the same case, sock_diag_put_filterinfo() leaves out the filter
attribute without any error.

sk_attach_filter() goes through __get_filter()->bpf_prog_store_orig_filter()
and keeps the original program. So for classic filters, the two exported
attach APIs now behave differently for SO_GET_FILTER and sock_diag.

The tun consumer later in this series does not seem to be affected in a
way userspace can see. SO_GET_FILTER and sock_diag cannot reach its
sockets, and TUNGETFILTER returns tun->fprog.

Could this store orig_prog, the way bpf_prog_create_from_user() does
with save_orig? If not, could the limitation be documented and the
comments in bpf_prog_create() and sk_get_filter() updated?

[Severity: Medium]

Does this change whose credentials decide JIT hardening?

bpf_prog_create() calls bpf_prog_alloc(), and bpf_prog_alloc_no_stats()
sets:

	fp->blinding_requested = bpf_jit_blinding_enabled(fp);

When bpf_jit_harden is 1, bpf_jit_blinding_enabled() decides based on
current:

	if (bpf_jit_harden == 1 && bpf_token_capable(prog->aux->token, CAP_BPF))
		return false;

The instructions come from whoever stored them in the sock_fprog_kern.
Blinding, however, depends on whichever task calls
sk_attach_filter_kern() later.

With the tun consumer later in this series, this sequence looks possible:

1. An unprivileged owner of a persistent multi-queue TAP passes
   tun_not_capable() and issues TUNATTACHFILTER. The instructions are
   stored in tun->fprog_kern. The programs built for the queues attached
   at that point are blinded.

2. Later, a process with CAP_BPF or CAP_SYS_ADMIN attaches another queue
   using TUNSETIFF or TUNSETQUEUE.

3. tun_attach() calls sk_attach_filter_kern(&tun->fprog_kern, ...) with
   the privileged credentials. The unprivileged user's filter goes
   through bpf_prepare_filter()->bpf_migrate_filter()->
   bpf_prog_select_runtime() and is JIT-compiled without constant
   blinding.

Before this change, sk_attach_filter(&tun->fprog, ...) read the
instructions again from the attaching process's own address space. The
unprivileged user did not control the bytes compiled in the privileged
context.

With bpf_jit_harden=1, does this let constants chosen by the user into
JIT memory that is not blinded? Could the blinding decision be recorded
when the instructions are supplied, or be tied to where the program came
from rather than to current at attach time?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929093712.131096-1-clementwei90%40163.com