netdev - Re: [PATCH v1 net 1/2] tcp: Don't drop head OOB when queuing 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iLqWG_YWKq+Fvg+REWG=cFnNyZ8w+bV-V-xbh+bes3_Bw@mail.gmail.com>
Date: Tue, 20 Aug 2024 15:34:15 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Takamitsu Iwai <takamitz@...zon.co.jp>
Cc: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v1 net 1/2] tcp: Don't drop head OOB when queuing new OOB.

On Tue, Aug 20, 2024 at 5:50 AM Takamitsu Iwai <takamitz@...zon.co.jp> wrote:
>
> 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);
> -               }
> -       }
> -

My opinion is that we should not touch this code.

No one sane uses OOB and TCP, otherwise this issue would have been
discussed a long time ago.

If anyone is using it, then your change will change the behavior.

Fact that you changed tools/testing/selftests/net/af_unix/msg_oob.c
should speak by itself.

Note that OOB has been added to AF_UNIX recently, it should have
followed TCP behavior.
Honestly we should not have accepted OOB on AF_UNIX, this was a real mistake.