netdev - [PATCH v1] Remove zero length skb's when enqueuing new OOB

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-next>] [day] [month] [year] [list]

Message-Id: <20240910002854.264192-1-Rao.Shoaib@oracle.com>
Date: Mon,  9 Sep 2024 17:28:54 -0700
From: Rao Shoaib <Rao.Shoaib@...cle.com>
To: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com
Cc: kuniyu@...zon.com, netdev@...r.kernel.org,
        Rao Shoaib <Rao.Shoaib@...cle.com>
Subject: [PATCH v1] Remove zero length skb's when enqueuing new OOB

13:03 Recent tests show that AF_UNIX socket code does not handle
the following sequence properly

Send OOB
Read OOB
Send OOB
Read (Without OOB flag)

The last read returns the OOB byte, which is incorrect.
A following read with OOB flag returns EFAULT, which is also incorrect.

In AF_UNIX, OOB byte is stored in a single skb, a pointer to the
skb is stored in the linux socket (oob_skb) and the skb is linked
in the socket's receive queue. Obviously, there are two refcnts on
the skb.

If the byte is read as an OOB, there will be no remaining data and
regular read frees the skb in managge_oob() and moves to the next skb.
The bug was that the next skb could be an OOB byte, but the code did
not check that which resulted in a regular read, receiving the OOB byte.

This patch adds code check the next skb obtained when a zero
length skb is freed.

The patch also adds code to check and remove an skb in front
of about to be added OOB if it is a zero length skb.

The cause of the last EFAULT was that the OOB byte had already been read
by the regular read but oob_skb was not cleared. This resulted in
__skb_datagram_iter() receiving a zero length skb to copy a byte from.
So EFAULT was returned.

Fixes: 314001f0bf92 ("af_unix: Add OOB support")
Signed-off-by: Rao Shoaib <Rao.Shoaib@...cle.com>
---
 net/unix/af_unix.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 0be0dcb07f7b..468d37ea986a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2185,6 +2185,11 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	return err;
 }
 
+static unsigned int unix_skb_len(const struct sk_buff *skb)
+{
+	return skb->len - UNIXCB(skb).consumed;
+}
+
 /* We use paged skbs for stream sockets, and limit occupancy to 32768
  * bytes, and a minimum of a full page.
  */
@@ -2195,7 +2200,7 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 		     struct scm_cookie *scm, bool fds_sent)
 {
 	struct unix_sock *ousk = unix_sk(other);
-	struct sk_buff *skb;
+	struct sk_buff *skb, *tail_skb;
 	int err = 0;
 
 	skb = sock_alloc_send_skb(sock->sk, 1, msg->msg_flags & MSG_DONTWAIT, &err);
@@ -2231,8 +2236,17 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 	scm_stat_add(other, skb);
 
 	spin_lock(&other->sk_receive_queue.lock);
+
 	if (ousk->oob_skb)
 		consume_skb(ousk->oob_skb);
+
+	tail_skb = skb_peek_tail(&other->sk_receive_queue);
+	if (tail_skb && !unix_skb_len((const struct sk_buff *)tail_skb)) {
+		/* Remove the zero length skb of the previous OOB */
+		__skb_unlink(tail_skb, &other->sk_receive_queue);
+		consume_skb(tail_skb);
+	}
+
 	WRITE_ONCE(ousk->oob_skb, skb);
 	__skb_queue_tail(&other->sk_receive_queue, skb);
 	spin_unlock(&other->sk_receive_queue.lock);
@@ -2600,11 +2614,6 @@ static long unix_stream_data_wait(struct sock *sk, long timeo,
 	return timeo;
 }
 
-static unsigned int unix_skb_len(const struct sk_buff *skb)
-{
-	return skb->len - UNIXCB(skb).consumed;
-}
-
 struct unix_stream_read_state {
 	int (*recv_actor)(struct sk_buff *, int, int,
 			  struct unix_stream_read_state *);
@@ -2667,6 +2676,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 {
 	struct unix_sock *u = unix_sk(sk);
 
+scan_again:
 	if (!unix_skb_len(skb)) {
 		struct sk_buff *unlinked_skb = NULL;
 
@@ -2685,6 +2695,8 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 		spin_unlock(&sk->sk_receive_queue.lock);
 
 		consume_skb(unlinked_skb);
+		if (skb)
+			goto scan_again;
 	} else {
 		struct sk_buff *unlinked_skb = NULL;
 
-- 
2.43.5