netdev - Re: [PATCH V2 net-next 1/2] tcp: send in-queue bytes in cmsg upon read

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20180430.121024.1354407786745903932.davem@davemloft.net>
Date:   Mon, 30 Apr 2018 12:10:24 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     eric.dumazet@...il.com
Cc:     soheil.kdev@...il.com, netdev@...r.kernel.org, ycheng@...gle.com,
        ncardwell@...gle.com, edumazet@...gle.com, willemb@...gle.com,
        soheil@...gle.com
Subject: Re: [PATCH V2 net-next 1/2] tcp: send in-queue bytes in cmsg upon
 read

From: Eric Dumazet <eric.dumazet@...il.com>
Date: Mon, 30 Apr 2018 09:01:47 -0700

> TCP sockets are read by a single thread really (or synchronized
> threads), or garbage is ensured, regardless of how the kernel
> ensures locking while reporting "queue length"

Whatever applications "typically do", we should never return
garbage, and that is what this code allowing to happen.

Everything else in recvmsg() operates on state under the proper socket
lock, to ensure consistency.

The only reason we are releasing the socket lock first it to make sure
the backlog is processed and we have the most update information
available.

It seems like one is striving for correctness and better accuracy, no?
:-)

Look, this can be fixed really simply.  And if you are worried about
unbounded loops if two apps maliciously do recvmsg() in parallel,
then don't even loop, just fallback to full socket locking and make
the "non-typical" application pay the price:

	tmp1 = A;
	tmp2 = B;
	barrier();
	tmp3 = A;
	if (unlikely(tmp1 != tmp3)) {
		lock_sock(sk);
		tmp1 = A;
		tmp2 = B;
		release_sock(sk);
	}

I'm seriously not applying the patch as-is, sorry.  This issue
must be addressed somehow.

Thank you.