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: <20201110120429.GB5635@katalix.com>
Date:   Tue, 10 Nov 2020 12:04:29 +0000
From:   Tom Parkin <tparkin@...alix.com>
To:     Guillaume Nault <gnault@...hat.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 00:24:01 +0100, Guillaume Nault wrote:
> On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > +		case PPPIOCBRIDGECHAN:
> > +			if (get_user(unit, p))
> > +				break;
> > +			err = -ENXIO;
> > +			if (pch->bridge) {
> > +				err = -EALREADY;
> > +				break;
> > +			}
> > +			pn = ppp_pernet(current->nsproxy->net_ns);
> > +			spin_lock_bh(&pn->all_channels_lock);
> > +			pchb = ppp_find_channel(pn, unit);
> > +			if (pchb) {
> > +				refcount_inc(&pchb->file.refcnt);
> > +				pch->bridge = pchb;
> 
> I think we shouldn't modify ->bridge if it's already set or if the
> channel is already part of of a PPP unit  (that is, if pch->ppp or
> pch->bridge is not NULL).
> 
> This also means that we have to use appropriate locking.

Yes, good point about checking for the channel being part of a PPP
unit.

> > +				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?

> 
> > +		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.

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)