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 12:10:15 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	Krzysztof Mazur <krzysiek@...lesie.net>
Cc:	"Chas Williams (CONTRACTOR)" <chas@....nrl.navy.mil>,
	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 10:53 +0100, Krzysztof Mazur wrote:
> On Fri, Nov 30, 2012 at 08:25:22AM +0000, David Woodhouse wrote:
> > 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()?
> 
> no, 

... oops, sorry. My sleep-deprived brain thought that we were calling
pppoatm_send() *from* vcc_sendmsg() with the lock held. But of course
we're not; we're calling directly into the driver. So that's OK.

In that case I think we're fine. I'll just do the same thing in
br2684_push(), fix up the comment you just corrected, and we're all
good.

> I think that the current order is good, now we have:
> 
> 	1. stop_sending_fames	to protocol
> 		now TX is shut down
> 		(currently done by
> 			set_bit(ATM_VF_CLOSE, &vcc->flags);
> 			clear_bit(ATM_VF_READY, &vcc->flags);
> 		)

Right, with the caveat the the socket lock is required for
synchronisation on this. But that's OK. Or we *could* perhaps introduce
an explicit call into the protocol for it, if we really wanted. But I'm
inclined not to.

> 	2. close_device		to device
> 		now RX is shut down
> 	3. device_was_closed	to protocol
> 		ugly push(NULL), but we can add some other callback.

> we also can do:
> 
> 	1. disable RX		to device
> 		now RX is shut down
> 	2. detach	to protocol
> 		now TX is shut down
> 		(now protocol can fully detach because RX is disabled)

Careful. You have to flush the TX packets which are currently in-flight.
It's not sufficient just to stop sending any more. And you have to do it
*before* the data structures are torn down.

> 	3. close_device		to device
> 		(device is not used anymore)

Really, what we're saying is that *one* of the driver or protocol close
functions needs to be split, and we need to do DPD or PDP. Since the
device driver *can* abort/flush the TX queue and also any pending RX
being handled by a tasklet, I think it makes most sense to keep it in
the middle, with the protocol being handled first and last... which is
the current order, as long as we consider setting ATM_VF_CLOSE to be the
first part.

-- 
dwmw2

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