netdev - Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

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:	Fri, 30 Nov 2012 08:25:22 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	"Chas Williams (CONTRACTOR)" <chas@....nrl.navy.mil>
Cc:	Krzysztof Mazur <krzysiek@...lesie.net>,
	David Laight <David.Laight@...LAB.COM>, davem@...emloft.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	nathan@...verse.com.au
Subject: Re: [PATCH v2 3/3] pppoatm: protect against freeing of vcc

On Fri, 2012-11-30 at 01:57 +0000, David Woodhouse wrote:
> I think it's actually fixed for pppoatm by the bh_lock_sock() and the
> sock_owned_by_user() check. As soon as vcc_release() calls lock_sock(),
> pppoatm stops accepting packets.
> 
> It should be simple enough to do the same in br2684.

Um... but now I come to look at it... Krzysztof, doesn't your 'pppoatm:
take ATM socket lock in pppoatm_send()' patch actually *break* the case
of sending via vcc_sendmsg()?

Why did you include the sock_owned_by_user() check in there and not just
use bh_lock_sock()?

With the sock_owned_by_user() check, it'll *always* drop packets
submitted through vcc_sendmsg(), won't it?

Admittedly, for PPPoATM and BR2684 we never do want to have packets
submitted directly from userspace that way; they should all come via the
PPP channel or the netdev respectively. So we might want to keep the
sock_owned_by_user() check because it fixes the close race, and
explicitly document it.

But it doesn't necessarily work for other protocols, so we may need a
better solution for the general case. Perhaps drop the
sock_owned_by_user() check, and put bh_lock_sock() around the beginning
of vcc_destroy_socket() where it clears ATM_VF_READY? That'll ensure
that no ->push() is *currently* operating on a skb having seen that the
VCC is still open.

Or maybe we just make the *devices* check the ATM_VF_CLOSE flag and
refuse to send the skb? Put the entire thing into their domain. Although
that may involve extra locking in the driver to synchronise send() and
close() sufficiently.

I'm still reluctant to swap the order of the device/protocol close in
vcc_destroy_socket(). I think that'll just swap one set of problems
which is now fairly well-understood and mostly solved, for another set.
In particular, I think the device needs to see the close first, because
*it* can actually abort or flush any pending TX and RX (including
synchronising with its tasklet as solos-pci does, etc.). Only then does
the protocol tear its data structures down. But I suppose the new set of
problems could be found and overcome, if Chas wants to propose an
alternative patch set...

-- 
dwmw2

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (6171 bytes)