Re: [PATCH net v8 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/179309578401.3134.11519975030981154705@kernel.org/
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> ---
>  drivers/net/tun.c      | 125 +++++++++++++++++++++++++++++++++++++----
>  include/linux/filter.h |   2 +
>  net/core/filter.c      |  67 ++++++++++++++++++++++
>  3 files changed, 182 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd071..13ce62aa0f142 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -197,6 +197,7 @@ struct tun_struct {
>  	int			sndbuf;
>  	struct tap_filter	txflt;
>  	struct sock_fprog	fprog;
> +	struct sock_fprog_kern	fprog_kern;
>  	/* protected by rtnl lock */
>  	bool			filter_attached;
>  	u32			msg_enable;
> @@ -722,12 +723,67 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>  }
>  
> +/* Copy the filter that @argp points at into the kernel, so that it can be
> + * installed again later, independent of the ioctl caller's address space.
> + * @fprog_kern receives the copy, which belongs to the caller until it is
> + * installed or freed, and @fprog receives the header, with the user pointer
> + * kept for TUNGETFILTER.
> + */
> +static int tun_copy_filter(struct sock_fprog __user *argp,
> +			   struct sock_fprog *fprog,
> +			   struct sock_fprog_kern *fprog_kern)
> +{
> +	struct sock_fprog fp;
> +	struct sock_filter *insns;
> +	int err;
> +
> +	if (copy_from_user(&fp, argp, sizeof(fp)))
> +		return -EFAULT;
> +
> +	if (!fp.len || fp.len > BPF_MAXINSNS)
> +		return -EINVAL;
> +
> +	insns = kmalloc_array(fp.len, sizeof(struct sock_filter),
> +			      GFP_KERNEL_ACCOUNT);
> +	if (!insns)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(insns, fp.filter,
> +			   fp.len * sizeof(struct sock_filter))) {
> +		kfree(insns);
> +		return -EFAULT;
> +	}
> +
> +	/* The instructions are only verified when a program is built from
> +	 * them, which happens when the filter is attached to a socket. Run
> +	 * the classic verifier here as well, so that a bad program is
> +	 * refused before it is saved and before any queue is touched.
> +	 * bpf_prog_create() can still refuse the program later, when it is
> +	 * converted to eBPF or when no runtime can be selected for it; the
> +	 * attach path returns that error, and the saved program is not
> +	 * replaced.
> +	 */
> +	err = bpf_prog_validate_classic(insns, fp.len);
> +	if (err) {
> +		kfree(insns);
> +		return err;
> +	}
> +
> +	fprog->len = fp.len;
> +	fprog->filter = fp.filter;
> +	fprog_kern->len = fp.len;
> +	fprog_kern->filter = insns;
> +
> +	return 0;
> +}
> +
>  static int tun_attach(struct tun_struct *tun, struct file *file,
>  		      bool skip_filter, bool napi, bool napi_frags,
>  		      bool publish_tun)
>  {
>  	struct tun_file *tfile = file->private_data;
>  	struct net_device *dev = tun->dev;
> +	bool rollback_filter = false;
>  	int err;
>  
>  	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
> @@ -752,10 +808,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	/* Re-attach the filter to persist device */
>  	if (!skip_filter && (tun->filter_attached == true)) {
>  		lock_sock(tfile->socket.sk);
> -		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (!err)
> +		if (err)
>  			goto out;
> +		rollback_filter = true;
>  	}
>  
>  	if (!tfile->detached &&
> @@ -817,6 +874,17 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>  	WRITE_ONCE(tun->numqueues, tun->numqueues + 1);
>  	tun_set_real_num_queues(tun);
>  out:
> +	if (err && rollback_filter) {
> +		/* The filter was installed before the rest of the attach
> +		 * ran, so drop it again if a later step failed. Any
> +		 * program it replaced is already gone: this leaves the
> +		 * queue without a filter rather than restoring the
> +		 * previous one.
> +		 */
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}
>  	return err;
>  }
>  
> @@ -2400,6 +2468,7 @@ static void tun_free_netdev(struct net_device *dev)
>  	security_tun_dev_free_security(tun->security);
>  	__tun_set_ebpf(tun, &tun->steering_prog, NULL);
>  	__tun_set_ebpf(tun, &tun->filter_prog, NULL);
> +	kfree(tun->fprog_kern.filter);
>  }
>  
>  static void tun_setup(struct net_device *dev)
> @@ -3064,10 +3133,14 @@ static void tun_detach_filter(struct tun_struct *tun, int n)
>  		release_sock(tfile->socket.sk);
>  	}
>  
> +	kfree(tun->fprog_kern.filter);
> +	tun->fprog_kern.filter = NULL;
> +	tun->fprog_kern.len = 0;
>  	tun->filter_attached = false;
>  }
>  
> -static int tun_attach_filter(struct tun_struct *tun)
> +static int tun_attach_filter(struct tun_struct *tun,
> +			     struct sock_fprog_kern *fprog_kern)
>  {
>  	int i, ret = 0;
>  	struct tun_file *tfile;
> @@ -3075,15 +3148,29 @@ static int tun_attach_filter(struct tun_struct *tun)
>  	for (i = 0; i < tun->numqueues; i++) {
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		lock_sock(tfile->socket.sk);
> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
> +		ret = sk_attach_filter_kern(fprog_kern, tfile->socket.sk);
>  		release_sock(tfile->socket.sk);
> -		if (ret) {
> -			tun_detach_filter(tun, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out_detach;
>  	}
>  
>  	tun->filter_attached = true;
> +	return 0;
> +
> +out_detach:
> +	/* Drop the copies already installed on queues 0..i-1, which leaves
> +	 * them without a filter: their previous sk_filter, if any, was
> +	 * already replaced and released. The caller does not replace the
> +	 * saved program when this fails, so tun->fprog, tun->fprog_kern and
> +	 * filter_attached keep their previous values.
> +	 */
> +	while (i--) {
> +		tfile = rtnl_dereference(tun->tfiles[i]);
> +		lock_sock(tfile->socket.sk);
> +		sk_detach_filter(tfile->socket.sk);
> +		release_sock(tfile->socket.sk);
> +	}
> +
>  	return ret;
>  }
>  
> @@ -3418,17 +3505,31 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>  		tun_set_sndbuf(tun);
>  		break;
>  
> -	case TUNATTACHFILTER:
> +	case TUNATTACHFILTER: {
> +		struct sock_fprog_kern fprog_kern;
> +		struct sock_fprog fprog;
> +
>  		/* Can be set only for TAPs */
>  		ret = -EINVAL;
>  		if ((tun->flags & TUN_TYPE_MASK) != IFF_TAP)
>  			break;
> -		ret = -EFAULT;
> -		if (copy_from_user(&tun->fprog, argp, sizeof(tun->fprog)))
> +
> +		ret = tun_copy_filter(argp, &fprog, &fprog_kern);
> +		if (ret)
> +			break;
> +
> +		ret = tun_attach_filter(tun, &fprog_kern);
> +		if (ret) {
> +			kfree(fprog_kern.filter);
>  			break;
> +		}
>  
> -		ret = tun_attach_filter(tun);
> +		/* Install the copy only after every queue accepted it. */
> +		kfree(tun->fprog_kern.filter);
> +		tun->fprog_kern = fprog_kern;
> +		tun->fprog = fprog;

    struct sock_fprog has padding between @len and @filter (6 bytes on
    64-bit). Because @fprog is uninitialized on the stack and
    tun_copy_filter() only writes its two fields, `tun->fprog = fprog` can
    copy uninitialized kernel stack bytes into tun->fprog, which
    TUNGETFILTER then leaks to userspace via copy_to_user(...,
    sizeof(tun->fprog)) when CONFIG_INIT_STACK_ALL_ZERO is not enabled
    (KMSAN kernel-infoleak).

    Zero-initialize @fprog (e.g. `struct sock_fprog fprog = {};` or
    memset() in tun_copy_filter()).