netdev - Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

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: <20201115162838.GF11274@linux.home>
Date:   Sun, 15 Nov 2020 17:28:38 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Tom Parkin <tparkin@...alix.com>
Cc:     netdev@...r.kernel.org, jchapman@...alix.com
Subject: Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > > +				err = 0;
> > > +			}
> > > +			spin_unlock_bh(&pn->all_channels_lock);
> > > +			break;
> > >  		default:
> > >  			down_read(&pch->chan_sem);
> > >  			chan = pch->chan;
> > > @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> > >  		return;
> > >  	}
> > >  
> > > +	if (pch->bridge) {
> > > +		skb_queue_tail(&pch->bridge->file.xq, skb);
> > 
> > The bridged channel might reside in a different network namespace.
> > So it seems that skb_scrub_packet() is needed before sending the
> > packet.
> 
> I'm not sure about this.
> 
> PPPIOCBRIDGECHAN is looking up the bridged channel in the ppp_pernet
> list.  Unless the channel can migrate across network namespaces after
> the bridge is set up I don't think it would be possible for the
> bridged channel to be in a different namespace.
> 
> Am I missing something here?

So yes, channels can't migrate across namespaces. However, the bridged
channel is looked up from the caller's current namespace, which isn't
guaranteed to be the same namespace as the channel used in the ioctl().

For example:

setns(ns1, CLONE_NEWNET);
chan_ns1 = open("/dev/ppp");
...
setns(ns2, CLONE_NEWNET);
chan_ns2 = open("/dev/ppp");
...
ioctl(chan_ns1, PPPIOCBRIDGECHAN, chan_ns2_id);

Here, chan_ns1 belongs to ns1, but chan_ns2_id will be looked up in the
context of ns2. I find it nice to have the possibility to bridge
channels from different namespaces, but we have to handle the case
correctly.

> > > +		ppp_channel_push(pch->bridge);
> > 
> > I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
> > is necessary. We're not going through a PPP unit, so we have no risk of
> > recursion here. Also, if ->start_xmit() fails, I see no reason for
> > requeuing the skb, like __ppp_channel_push() does. I'd have to think
> > more about it, but I believe that we could call the channel's
> > ->start_xmit() function directly (respecting the locking constraints
> > of course).
> 
> I take your point about re-queuing based on the return of
> ->start_xmit().  For pppoe and pppol2tp start_xmit just swallows the
> skb on failure in any case, so for this specific usecase queuing is
> not an issue.

Indeed.

> However, my primary motivation for using ppp_channel_push was actually
> the handling for managing dropping the packet if the channel was
> deregistered.

I might be missing something, but I don't see what ppp_channel_push()
does appart from holding the xmit lock and handling the xmit queue.
If we agree that there's no need to use the xmit queue, all
ppp_channel_push() does for us is taking pch->downl, which we probably
can do on our own.

> It'd be simple enough to add another function which performed the same
> deregistration check but didn't transmit via. the queue.

That's probably what I'm missing: what do you mean by "deregistration
check"? I can't see anything like this in ppp_channel_push().