| 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
| ||
|
Message-ID: <20240820034920.77419-2-takamitz@amazon.co.jp>
Date: Tue, 20 Aug 2024 12:49:19 +0900
From: Takamitsu Iwai <takamitz@...zon.co.jp>
To: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, David Ahern <dsahern@...nel.org>
CC: Kuniyuki Iwashima <kuniyu@...zon.com>, Takamitsu Iwai
<takamitz@...zon.co.jp>, <netdev@...r.kernel.org>
Subject: [PATCH v1 net 1/2] tcp: Don't drop head OOB when queuing new OOB.
If OOB is not at the head of recvq, it's not dropped when a new OOB is
queued.
OTOH, as commit f5ea0768a255 ("selftest: af_unix: Add
non-TCP-compliant test cases in msg_oob.c.") points out, TCP drops OOB
data at the head of recvq when queuing a new OOB data subsequently.
This behavior has been introduced in tcp_check_urg() by deleting
preceding skb when next MSG_OOB data is received. This process is
weird OOB handling, and the comment also says this is wrong.
We remove this code block for appropriate OOB handling.
Now TCP works exactly the same way as AF_UNIX, so this patch enables
kernel to pass the test when removing tcp_incompliant braces.
# RUN msg_oob.no_peek.inline_ex_oob_drop ...
# OK msg_oob.no_peek.inline_ex_oob_drop
ok 18 msg_oob.no_peek.inline_ex_oob_drop
# RUN msg_oob.peek.ex_oob_drop ...
# OK msg_oob.peek.ex_oob_drop
ok 28 msg_oob.peek.ex_oob_drop
# RUN msg_oob.peek.ex_oob_drop_2 ...
# OK msg_oob.peek.ex_oob_drop_2
ok 29 msg_oob.peek.ex_oob_drop_2
Fixes tag refers to the commit of Linux-2.6.12-rc2, but this code was
written at v2.4.4 which is older than this version.
This is a long-standing bug since then, and technically the patch
slightly changes uAPI, but RFC 6091, published in 2011, suggests TCP
urgent mechanism should not be used for newer applications in 2011.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Takamitsu Iwai <takamitz@...zon.co.jp>
---
net/ipv4/tcp_input.c | 25 ---------
tools/testing/selftests/net/af_unix/msg_oob.c | 55 ++++++++-----------
2 files changed, 22 insertions(+), 58 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e37488d3453f..648d0f3ade78 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5830,31 +5830,6 @@ static void tcp_check_urg(struct sock *sk, const struct tcphdr *th)
/* Tell the world about our new urgent pointer. */
sk_send_sigurg(sk);
- /* We may be adding urgent data when the last byte read was
- * urgent. To do this requires some care. We cannot just ignore
- * tp->copied_seq since we would read the last urgent byte again
- * as data, nor can we alter copied_seq until this data arrives
- * or we break the semantics of SIOCATMARK (and thus sockatmark())
- *
- * NOTE. Double Dutch. Rendering to plain English: author of comment
- * above did something sort of send("A", MSG_OOB); send("B", MSG_OOB);
- * and expect that both A and B disappear from stream. This is _wrong_.
- * Though this happens in BSD with high probability, this is occasional.
- * Any application relying on this is buggy. Note also, that fix "works"
- * only in this artificial test. Insert some normal data between A and B and we will
- * decline of BSD again. Verdict: it is better to remove to trap
- * buggy users.
- */
- if (tp->urg_seq == tp->copied_seq && tp->urg_data &&
- !sock_flag(sk, SOCK_URGINLINE) && tp->copied_seq != tp->rcv_nxt) {
- struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
- tp->copied_seq++;
- if (skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq)) {
- __skb_unlink(skb, &sk->sk_receive_queue);
- __kfree_skb(skb);
- }
- }
-
WRITE_ONCE(tp->urg_data, TCP_URG_NOTYET);
WRITE_ONCE(tp->urg_seq, ptr);
diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c
index 535eb2c3d7d1..f3435575dfa5 100644
--- a/tools/testing/selftests/net/af_unix/msg_oob.c
+++ b/tools/testing/selftests/net/af_unix/msg_oob.c
@@ -484,20 +484,17 @@ TEST_F(msg_oob, ex_oob_drop)
epollpair(true);
siocatmarkpair(true);
- sendpair("y", 1, MSG_OOB); /* TCP drops "x" at this moment. */
+ sendpair("y", 1, MSG_OOB);
epollpair(true);
+ siocatmarkpair(false);
- tcp_incompliant {
- siocatmarkpair(false);
-
- recvpair("x", 1, 1, 0); /* TCP drops "y" by passing through it. */
- epollpair(true);
- siocatmarkpair(true);
+ recvpair("x", 1, 1, 0);
+ epollpair(true);
+ siocatmarkpair(true);
- recvpair("y", 1, 1, MSG_OOB); /* TCP returns -EINVAL. */
- epollpair(false);
- siocatmarkpair(true);
- }
+ recvpair("y", 1, 1, MSG_OOB);
+ epollpair(false);
+ siocatmarkpair(true);
}
TEST_F(msg_oob, ex_oob_drop_2)
@@ -506,23 +503,17 @@ TEST_F(msg_oob, ex_oob_drop_2)
epollpair(true);
siocatmarkpair(true);
- sendpair("y", 1, MSG_OOB); /* TCP drops "x" at this moment. */
+ sendpair("y", 1, MSG_OOB);
epollpair(true);
-
- tcp_incompliant {
- siocatmarkpair(false);
- }
+ siocatmarkpair(false);
recvpair("y", 1, 1, MSG_OOB);
epollpair(false);
+ siocatmarkpair(false);
- tcp_incompliant {
- siocatmarkpair(false);
-
- recvpair("x", 1, 1, 0); /* TCP returns -EAGAIN. */
- epollpair(false);
- siocatmarkpair(true);
- }
+ recvpair("x", 1, 1, 0);
+ epollpair(false);
+ siocatmarkpair(true);
}
TEST_F(msg_oob, ex_oob_ahead_break)
@@ -692,22 +683,20 @@ TEST_F(msg_oob, inline_ex_oob_drop)
epollpair(true);
siocatmarkpair(true);
- sendpair("y", 1, MSG_OOB); /* TCP drops "x" at this moment. */
+ sendpair("y", 1, MSG_OOB);
epollpair(true);
setinlinepair();
- tcp_incompliant {
- siocatmarkpair(false);
+ siocatmarkpair(false);
- recvpair("x", 1, 1, 0); /* TCP recv()s "y". */
- epollpair(true);
- siocatmarkpair(true);
+ recvpair("x", 1, 1, 0);
+ epollpair(true);
+ siocatmarkpair(true);
- recvpair("y", 1, 1, 0); /* TCP returns -EAGAIN. */
- epollpair(false);
- siocatmarkpair(false);
- }
+ recvpair("y", 1, 1, 0);
+ epollpair(false);
+ siocatmarkpair(false);
}
TEST_F(msg_oob, inline_ex_oob_siocatmark)
--
2.39.3 (Apple Git-145)
Powered by blists - more mailing lists