netdev - Re: [PATCH v2] tcp: fix MSG

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: <200905111450.06749.elendil@planet.nl>
Date:	Mon, 11 May 2009 14:50:05 +0200
From:	Frans Pop <elendil@...net.nl>
To:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
Cc:	Matthias Andree <matthias.andree@....de>,
	Netdev <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH v2] tcp: fix MSG_PEEK race check

On Monday 11 May 2009, Ilpo Järvinen wrote:
> I took my time to fix the urg_hole madness too. The patch below.

Hmm. I wonder if it wouldn't be better to keep the two issues separate. 
The initial patch is a clear regression fix (4 people have reported it 
against fetchmail for Debian). The URG part is IMO a separate issue which 
I at least have never seen in practice.
And my Tested-by doesn't cover the additional change either.

That said, I have added the URG change (as an incremental patch) in my 
local git repo and will give it a go when I next build a kernel (may take 
a week). I don't expect to be able to confirm it fixes the URG race, but 
I can at least check that it doesn't cause any false messages with my 
(spectacularly unspectacular) network traffic. I'll report the result.

> --
> [PATCH v2] tcp: fix MSG_PEEK race check
>
> Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of
> blocking behavior) lets the loop run longer than the race check
> did previously expect, so we need to be more careful with this
> check and consider the work we have been doing.
>
> I tried my best to deal with urg hole madness too which happens
> here:
> 	if (!sock_flag(sk, SOCK_URGINLINE)) {
> 		++*seq;
> 		...
> by using additional offset by one but I certainly have very
> little interest in testing that part.
>
> Signed-off-by: Ilpo J?rvinen <ilpo.jarvinen@...sinki.fi>
> Tested-by: Frans Pop <elendil@...net.nl>
> ---
>  net/ipv4/tcp.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1d7f49c..7a0f0b2 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1321,6 +1321,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock
> *sk, struct msghdr *msg, struct task_struct *user_recv = NULL;
>  	int copied_early = 0;
>  	struct sk_buff *skb;
> +	u32 urg_hole = 0;
>
>  	lock_sock(sk);
>
> @@ -1532,7 +1533,8 @@ do_prequeue:
>  				}
>  			}
>  		}
> -		if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) {
> +		if ((flags & MSG_PEEK) &&
> +		    (peek_seq - copied - urg_hole != tp->copied_seq)) {
>  			if (net_ratelimit())
>  				printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in
> MSG_PEEK.\n", current->comm, task_pid_nr(current));
> @@ -1553,6 +1555,7 @@ do_prequeue:
>  				if (!urg_offset) {
>  					if (!sock_flag(sk, SOCK_URGINLINE)) {
>  						++*seq;
> +						urg_hole++;
>  						offset++;
>  						used--;
>  						if (!used)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html