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

[Rongguang Wei <clementwei90@163.com>]

Willem de Bruijn wrote:
> 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()).
Thanks Willem.
In v9, the zero-initializes the caller's header in tun_copy_filter() before the
two fields are assigned:
	memset(fprog, 0, sizeof(*fprog));
so the padding that TUNGETFILTER copies is zero instead of uninitialized
stack. Everything else about the header is unchanged.