| 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
| ||
|
Message-ID: <20201109232401.GM2366@linux.home>
Date: Tue, 10 Nov 2020 00:24:01 +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 Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> This new ioctl allows two ppp channels to be bridged together: frames
> arriving in one channel are transmitted in the other channel and vice
> versa.
>
> The practical use for this is primarily to support the L2TP Access
> Concentrator use-case. The end-user session is presented as a ppp
> channel (typically PPPoE, although it could be e.g. PPPoA, or even PPP
> over a serial link) and is switched into a PPPoL2TP session for
> transmission to the LNS. At the LNS the PPP session is terminated in
> the ISP's network.
>
> When a PPP channel is bridged to another it takes a reference on the
> other's struct ppp_file. This reference is dropped when the channel is
> unregistered: if the dereference causes the bridged channel's reference
> count to reach zero it is destroyed at that point.
> ---
> drivers/net/ppp/ppp_generic.c | 35 +++++++++++++++++++++++++++++++++-
> include/uapi/linux/ppp-ioctl.h | 1 +
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 7d005896a0f9..d893bf4470f4 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -175,6 +175,7 @@ struct channel {
> struct net *chan_net; /* the net channel belongs to */
> struct list_head clist; /* link in list of channels per unit */
> rwlock_t upl; /* protects `ppp' */
> + struct channel *bridge; /* "bridged" ppp channel */
> #ifdef CONFIG_PPP_MULTILINK
> u8 avail; /* flag used in multilink stuff */
> u8 had_frag; /* >= 1 fragments have been sent */
> @@ -641,8 +642,9 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> }
>
> if (pf->kind == CHANNEL) {
> - struct channel *pch;
> + struct channel *pch, *pchb;
> struct ppp_channel *chan;
> + struct ppp_net *pn;
>
> pch = PF_TO_CHANNEL(pf);
>
> @@ -657,6 +659,24 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> err = ppp_disconnect_channel(pch);
> break;
>
> + 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.
> + 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.
> + 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).
> + return;
> + }
> +
> read_lock_bh(&pch->upl);
> if (!ppp_decompress_proto(skb)) {
> kfree_skb(skb);
> @@ -2791,6 +2817,13 @@ ppp_unregister_channel(struct ppp_channel *chan)
> up_write(&pch->chan_sem);
> ppp_disconnect_channel(pch);
>
> + /* Drop our reference on a bridged channel, if any */
> + if (pch->bridge) {
> + if (refcount_dec_and_test(&pch->bridge->file.refcnt))
> + ppp_destroy_channel(pch->bridge);
> + pch->bridge = NULL;
> + }
> +
> pn = ppp_pernet(pch->chan_net);
> spin_lock_bh(&pn->all_channels_lock);
> list_del(&pch->list);
> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
> index 7bd2a5a75348..4b97ab519c19 100644
> --- a/include/uapi/linux/ppp-ioctl.h
> +++ b/include/uapi/linux/ppp-ioctl.h
> @@ -115,6 +115,7 @@ struct pppol2tp_ioc_stats {
> #define PPPIOCATTCHAN _IOW('t', 56, int) /* attach to ppp channel */
> #define PPPIOCGCHAN _IOR('t', 55, int) /* get ppp channel number */
> #define PPPIOCGL2TPSTATS _IOR('t', 54, struct pppol2tp_ioc_stats)
> +#define PPPIOCBRIDGECHAN _IOW('t', 53, int) /* bridge one channel to another */
>
> #define SIOCGPPPSTATS (SIOCDEVPRIVATE + 0)
> #define SIOCGPPPVER (SIOCDEVPRIVATE + 1) /* NEVER change this!! */
> --
> 2.17.1
>
Powered by blists - more mailing lists