| Message ID | 20240910002854.264192-1-Rao.Shoaib@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] Remove zero length skb's when enqueuing new OOB | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Sep 09, 2024 at 05:28:54PM -0700, Rao Shoaib wrote: > 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@oracle.com> Hi Rao, This is not a proper review, I will leave that to Iwashima-san and others. But I would like to note that as a fix for net it needs to be annotated as such. Subject: [PATCH net v1] ... Unfortunately while the patch applies to net it does not apply to net-next. But without the above annotation the CI did not know to apply the patch to net. So the CI can't process this patch. I suggest posting a v2, targeted at net, after waiting for a review from Iwashima-san and others.
On 9/10/2024 6:44 AM, Simon Horman wrote: > On Mon, Sep 09, 2024 at 05:28:54PM -0700, Rao Shoaib wrote: >> 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@oracle.com> > > Hi Rao, > > This is not a proper review, I will leave that to Iwashima-san and others. > > But I would like to note that as a fix for net it needs to be annotated as > such. > > Subject: [PATCH net v1] ... > > Unfortunately while the patch applies to net it does not apply to net-next. > But without the above annotation the CI did not know to apply the patch to > net. So the CI can't process this patch. > > I suggest posting a v2, targeted at net, after waiting for a review from > Iwashima-san and others. > Thanks for pointing out. It may not be necessary for me to post a v2 as another fix has been accepted but let's see. Shoaib
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;
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@oracle.com> --- net/unix/af_unix.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)