Re: [PATCH net v7 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 no verifier would accept now
> fails early and leaves the previously attached filter in place.  The
> instructions are verified with the new bpf_prog_validate_classic(), which
> runs the same checks as bpf_prog_create() on the copied program.  Before
> this series the check only happened per queue in tun_attach_filter(),
> which never ran when no queue was attached, so an invalid program could be
> stored; with the corrected error check, every later attach of such a
> program would then fail with -EINVAL.
> 
> The inverted check was discovered by manual code inspection first, and the
> review of v1 reported the other things.
> 
> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

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