netdev - Re: [PATCH net] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240620221223.66096-1-kuniyu@amazon.com>
Date: Thu, 20 Jun 2024 15:12:23 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <mhal@...x.co>
CC: <bpf@...r.kernel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<jakub@...udflare.com>, <john.fastabend@...il.com>, <kuba@...nel.org>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH net] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

Sorry for not mentioning this before, but could you replace "net" with
"bpf" in Subject and rebase the patch on bpf.git so that we can trigger
the patchwork's CI ?

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git


From: Michal Luczaj <mhal@...x.co>
Date: Thu, 20 Jun 2024 22:20:05 +0200
> AF_UNIX socket tracks the most recent OOB packet (in its receive queue)
> with an `oob_skb` pointer. BPF redirecting does not account for that: when
> an OOB packet is moved between sockets, `oob_skb` is left outdated. This
> results in a single skb that may be accessed from two different sockets.
> 
> Take the easy way out: silently drop MSG_OOB data targeting any socket that
> is in a sockmap or a sockhash. Note that such silent drop is akin to the
> fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS).
> 
> For symmetry, forbid MSG_OOB in unix_bpf_recvmsg().
> 
> Suggested-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Michal Luczaj <mhal@...x.co>
> ---
>  net/unix/af_unix.c  | 30 +++++++++++++++++++++++++++++-
>  net/unix/unix_bpf.c |  3 +++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 5e695a9a609c..3a55d075f199 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2653,10 +2653,38 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>  
>  static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>  {
> +	struct unix_sock *u = unix_sk(sk);
> +	struct sk_buff *skb;
> +	int err;
> +
>  	if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED))
>  		return -ENOTCONN;
>  
> -	return unix_read_skb(sk, recv_actor);
> +	mutex_lock(&u->iolock);
> +	skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err);

	mutex_unlock(&u->iolock);

I think we can drop mutex here as the skb is already unlinked
and no receiver can touch it.

and the below part can be like the following not to slow down
the common case:

	if (!skb)
		return err;

> +
> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> +	if (skb) {

	if (unlikely(skb == READ_ONCE(u->oob_skb))) {


> +		bool drop = false;
> +
> +		spin_lock(&sk->sk_receive_queue.lock);
> +		if (skb == u->oob_skb) {

		if (likely(skb == u->oob_skb)) {

> +			WRITE_ONCE(u->oob_skb, NULL);
> +			drop = true;
> +		}
> +		spin_unlock(&sk->sk_receive_queue.lock);
> +
> +		if (drop) {
> +			WARN_ON_ONCE(skb_unref(skb));
> +			kfree_skb(skb);
> +			skb = NULL;
> +			err = -EAGAIN;
			return -EAGAIN;

> +		}
> +	}
> +#endif

	return recv_actor(sk, skb);

Thanks!

> +
> +	mutex_unlock(&u->iolock);
> +	return skb ? recv_actor(sk, skb) : err;
>  }
>  
>  static int unix_stream_read_generic(struct unix_stream_read_state *state,
> diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> index bd84785bf8d6..bca2d86ba97d 100644
> --- a/net/unix/unix_bpf.c
> +++ b/net/unix/unix_bpf.c
> @@ -54,6 +54,9 @@ static int unix_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>  	struct sk_psock *psock;
>  	int copied;
>  
> +	if (flags & MSG_OOB)
> +		return -EOPNOTSUPP;
> +
>  	if (!len)
>  		return 0;
>  
> -- 
> 2.45.1