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

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

[Michal Luczaj]

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@amazon.com>
Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Michal Luczaj <mhal@rbox.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);
+
+#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
+	if (skb) {
+		bool drop = false;
+
+		spin_lock(&sk->sk_receive_queue.lock);
+		if (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;
+		}
+	}
+#endif
+
+	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

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

[Kuniyuki Iwashima]

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@rbox.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@amazon.com>
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Michal Luczaj <mhal@rbox.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

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

[Jakub Kicinski]

On Thu, 20 Jun 2024 15:12:23 -0700 Kuniyuki Iwashima wrote:
> 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

netdev runs the BPF CI, too, FWIW.

Open the patch in patchwork:
https://patchwork.kernel.org/project/netdevbpf/patch/20240620203009.2610301-1-mhal@rbox.co/
Click on contest in "checks".
Select Executor = "gh-bpf-ci".
Click on "outputs", you should get to:
https://github.com/kernel-patches/bpf/actions/runs/9607623089
If you click in context on the branch name it will take you to
the tested branch:
https://github.com/linux-netdev/testing/commits/net-next-2024-06-21--03-00
which had:
  af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash
applied, 5th from the top.

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

[Michal Luczaj]

On 6/21/24 00:12, Kuniyuki Iwashima wrote:
> 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 ?

No problem, will do.

>> ...
>>  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.

I guess you're right about the mutex. That said, double mea culpa, lack of
state lock makes things racy:

unix_stream_read_skb
  mutex_lock
  skb = skb_recv_datagram
  mutex_unlock
  spin_lock
  if (oob_skb == skb) {
				unix_release_sock
				  if (u->oob_skb) {
				    kfree_skb(u->oob_skb)
				    u->oob_skb = NULL
				  }
    oob_skb = NULL
    drop = true
  }
  spin_unlock
  if (drop) {
    skb_unref(skb)
    kfree_skb(skb)
  }

In v2 I'll do what unix_stream_read_generic() does: take state lock and
check for SOCK_DEAD.

> 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);

All right, thanks. So here's v2:
https://lore.kernel.org/netdev/20240622223324.3337956-1-mhal@rbox.co/