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

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

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> TUNATTACHFILTER copies only the sock_fprog header, so tun->fprog.filter
> stays a pointer into the address space of the process that issued the
> ioctl. tun_attach() reads it again whenever a queue is attached to the
> persistent device later on.
> 
> Rebuilding the filter from that pointer is not reliable. With the inverted
> check below, a failure of the attach was not fatal: -EFAULT for an unmapped
> address, -EINVAL for bytes that are not a valid classic BPF program, -EPERM
> for a locked filter and -ENOMEM all ended up in "err = 0" for a new tfile,
> so the queue was attached without a filter. And when the read succeeded,
> the early return meant the queue was never published at all.
> 
> Keep the program in the kernel instead: tun->fprog_kern holds the
> instructions, and each queue gets its own program built from it with the
> new sk_attach_filter_kern(), the kernel memory counterpart of
> sk_attach_filter(). The copy is freed when the filter is detached or
> replaced and with the device, and tun->fprog is left untouched so
> TUNGETFILTER keeps its uapi behaviour.
> 
> Also fix the inverted check, which made tun_attach() return early when
> sk_attach_filter_kern() succeeded instead of when it failed. Neither change
> works on its own: with only the kernel copy, every re-attach returns 0
> without publishing the queue; with only the check fixed, a re-attach that
> used to succeed without installing a filter would start to fail.
> 
> sk_attach_filter_kern() builds the program with bpf_prog_create(), which
> does not keep an original program, so SO_GET_FILTER returns -EACCES and
> sock_diag omits the filter for sockets that use it. tun sockets are not
> exposed as file descriptors, so this is not user visible.
> 
> The header and the instructions are checked in tun_copy_filter() before
> anything is replaced: a TUNATTACHFILTER that the classic verifier rejects
> now fails early and leaves the previously attached filter in place.  The
> instructions are verified with the new bpf_prog_validate_classic(), which
> runs bpf_check_basics_ok() and bpf_check_classic() on the copied program.
> That does not cover the s16 check in bpf_convert_filter() or the runtime
> selection in bpf_prog_select_runtime(), so a program can still be refused
> when it is built for a queue. Such a late failure does not replace the
> saved program: the copy is committed to tun->fprog and tun->fprog_kern
> only after tun_attach_filter() installed it on every queue. A queue that
> already took the copy is rolled back, which leaves it without a filter,
> because the program it ran before was already released; restoring it
> would need an extra reference per queue.
> 
> The inverted check was discovered by manual code inspection first.
> 
> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/

nit: customary instead is to have a changelog with links to the previous
     versions below the three-dash line.

> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

Reviewed-by: Willem de Bruijn <willemb@google.com>