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]

Date:	Mon, 18 May 2009 15:04:33 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	ilpo.jarvinen@...sinki.fi
Cc:	elendil@...net.nl, matthias.andree@....de, netdev@...r.kernel.org
Subject: Re: [PATCH v2] tcp: fix MSG_PEEK race check

From: "Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
Date: Mon, 18 May 2009 10:24:23 +0300 (EEST)

> Sorry, I'm not sure you thought this fully through here. What my patch 
> with urg_hole does is that it keeps peek_seq non-advancing using the 
> offsets. Now without the urg offsetting, the peek_seq side of the race 
> check advances, which means that we didn't catch the race when it happened 
> for real as copied_seq advanced too?!?

I see now what the situation is, thanks for explaining.

You're account for the "*seq" advance for URG that happens in
tcp_recvmsg() rather than the one that happens in tcp_check_urg().

> From another perspective when the race didn't happen but potential for it 
> existed, the current check should catch that since peek_seq advanced and 
> copied_seq stayed where it was. But that certainly doesn't match to your 
> description above.

Right.

> I wonder why all this fuzz about this particular race as we will do our 
> best anyway to skip the hole on MSG_PEEK side (if I read the code right)? 
> Either we never see the hole in MSG_PEEK side, or we're happily past it 
> already, does it make any difference anymore in the latter case? <insert 
> some "not that I fully understand all of that multipage function" 
> disclaimer here though, I may think too simple scenarios here>. Not that 
> I'm too interested in "improving" urg or so anywa, I'm just curious... :-)

A long long time ago we had an assertion here checking whether
->copied_seq moved without our knowledge.  Alexey and I found this
could trigger and investigation of that is what helped us
find the tcp_check_urg()+MSG_PEEK case.  That's when we added this
test and log message.

When the MSG_PEEK test existing in the !copied if() branch of
tcp_recvmsg(), so many of these code paths we are dealing with in this
fix could simply not be reached when MSG_PEEK.  That ++*seq could
never happen, because if "copied" was non-zero and MSG_PEEK was true
we would leave the loop and return from tcp_recvmsg() immediately.

Now, we have to accomodate those adjustments.

Since I now understand your urg_hole code I'm going to apply your v2
patch which takes care of the URG stuff.  I also buy the argument that
perhaps we should get rid of the log message, but look at how it
helped us find this kernel regression :-)
--
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