Re: [PATCH net v7 1/2] tun: keep a kernel copy of the socket filter program

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (2) · 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).
> 
> - [High] bpf_prog_validate_classic() does not run "the same checks that
>   bpf_prog_create() performs", although its kernel-doc and the commit…
> - [Low] The kernel-doc for the new exported helper
>   bpf_prog_validate_classic() (net/core/filter.c) says it returns "0 if…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but a filter
>   replacement that passes tun_copy_filter() and then fails on some queue…
> - [Medium] This problem wasn't introduced by this patch, but
>   TUNDETACHFILTER leaves disabled queues with their old socket filter,…
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This commit keeps a kernel copy of the classic BPF program passed to
> TUNATTACHFILTER in tun->fprog_kern. Each queue's filter is now built from
> that copy with a new sk_attach_filter_kern() helper. It also fixes the
> inverted error check in tun_attach(), and it validates the program up
> front in tun_copy_filter() with a new bpf_prog_validate_classic() helper.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 242899f7fd071..e7d9f4480a89f 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
> > +	/* The instructions are only verified when a program is built from
> > +	 * them, which tun_attach_filter() does once per attached queue.
> > +	 * Check them here as well: with no queue attached that would not
> > +	 * happen at all, and, now that tun_attach() treats that error as
> > +	 * fatal, every later attach would fail with -EINVAL instead of
> > +	 * rejecting the program now.
> > +	 */
> > +	err = bpf_prog_validate_classic(insns, fprog.len);
> > +	if (err) {
> > +		kfree(insns);
> > +		return err;
> > +	}
> 
> [Severity: High]
> Does bpf_prog_validate_classic() really reject everything that
> bpf_prog_create() rejects? Its kernel-doc and the commit message both say
> it does:
> 
>   "The instructions are verified with the new bpf_prog_validate_classic(),
>   which runs the same checks as bpf_prog_create() on the copied program."
> 
> The helper only runs bpf_check_basics_ok() and bpf_check_classic().
> bpf_prog_create() also goes through this path:
> 
> bpf_prog_create()
>   bpf_prepare_filter()
>     bpf_migrate_filter()
>       bpf_convert_filter()
>     bpf_prog_select_runtime()
>       __bpf_prog_select_runtime()
> 
> bpf_convert_filter() rejects any jump whose converted offset does not fit
> in s16:
> 
> 	if (off < off_min || off > off_max)
> 		goto err;
> 
> __bpf_prog_select_runtime() can also return -ENOTSUPP when a JIT is
> required but did not run.
> 
> Take a 4096 instruction program: BPF_JMP|BPF_JA with k=4094 at pc 0, then
> 4094 BPF_LD|BPF_B|BPF_ABS loads of offset 0, then BPF_RET|BPF_K.
> bpf_check_classic() accepts it. convert_bpf_ld_abs() expands each load
> into 12 eBPF instructions, so the JA offset grows to about 49128, and
> bpf_convert_filter() fails with -EINVAL.
> 
> Suppose every queue on a multiqueue TAP has been disabled with
> IFF_DETACH_QUEUE. tun_copy_filter() stores this program, and
> tun_attach_filter() loops over zero queues and then does:
> 
> 	tun->filter_attached = true;
> 
> >From then on, every IFF_ATTACH_QUEUE, and every TUNSETIFF without
> IFF_NOFILTER, calls sk_attach_filter_kern() in tun_attach() and gets
> -EINVAL. With the corrected check, tun_attach() then takes the goto out
> path and the queue is never published.
> 
> Isn't this the "every later attach would fail with -EINVAL" case that the
> comment above says is avoided?
> 
> When queues are active, the same program fails at queue 0, and
> tun_attach_filter() calls tun_detach_filter(tun, 0). That frees fprog_kern
> and clears filter_attached, which doesn't seem to match this part of the
> commit message:
> 
>   "a TUNATTACHFILTER that no verifier would accept now fails early and
>   leaves the previously attached filter in place."
> 
> The sk_attach_filter_kern() kernel-doc in this same patch lists -EINVAL
> from bpf_prog_create() and -ENOTSUPP as possible results. That also
> suggests the two checks are not equivalent.
> 
> Would it be more reliable for tun_copy_filter() to build the program once
> with bpf_prog_create() and then destroy it, instead of calling
> bpf_prog_validate_classic()?

This sounds to me as the perfect being the enemy of the good.

Even the addition of calling bpf_prog_validate_classic is a nice to have.
This series fixes a real issue.

Yes, some issues are still only discovered later when the BPF program is
built. But as long that late discovery is also handled well, that is
acceptable.