netdev - Re: [PATCH v3 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

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: <20201207162228.GA28888@linux.home>
Date:   Mon, 7 Dec 2020 17:22:28 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Tom Parkin <tparkin@...alix.com>
Cc:     netdev@...r.kernel.org, jchapman@...alix.com
Subject: Re: [PATCH v3 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and
 PPPIOCUNBRIDGECHAN ioctls

On Fri, Dec 04, 2020 at 04:36:55PM +0000, Tom Parkin wrote:
> +static int ppp_unbridge_channels(struct channel *pch)
> +{
> +	struct channel *pchb, *pchbb;
> +
> +	write_lock_bh(&pch->upl);
> +	pchb = rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl));
> +	if (!pchb) {
> +		write_unlock_bh(&pch->upl);
> +		return -EINVAL;
> +	}
> +	RCU_INIT_POINTER(pch->bridge, NULL);
> +	write_unlock_bh(&pch->upl);
> +
> +	write_lock_bh(&pchb->upl);
> +	pchbb = rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl));
> +	if (pchbb == pch)
> +		RCU_INIT_POINTER(pchb->bridge, NULL);
> +	write_unlock_bh(&pchb->upl);
> +
> +	synchronize_rcu();
> +
> +	if (pchbb == pch)
> +		if (refcount_dec_and_test(&pch->file.refcnt))
> +			ppp_destroy_channel(pch);

Since a respin is needed (see below), maybe add a comment explaining
why we need to verify that pchbb == pch.

> +	if (refcount_dec_and_test(&pchb->file.refcnt))
> +		ppp_destroy_channel(pchb);
> +
> +	return 0;
> +}
> +
>  static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct ppp_file *pf;
> @@ -641,8 +714,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 +731,29 @@ 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;
> +			pn = ppp_pernet(current->nsproxy->net_ns);
> +			spin_lock_bh(&pn->all_channels_lock);
> +			pchb = ppp_find_channel(pn, unit);
> +			/* Hold a reference to prevent pchb being freed while
> +			 * we establish the bridge.
> +			 */
> +			if (pchb)
> +				refcount_inc(&pchb->file.refcnt);

The !pchb case isn't handled. With this code, if ppp_find_channel()
returns NULL, ppp_bridge_channels() will crash when trying to lock
pchb->upl.

> +			spin_unlock_bh(&pn->all_channels_lock);
> +			err = ppp_bridge_channels(pch, pchb);
> +			/* Drop earlier refcount now bridge establishment is complete */
> +			if (refcount_dec_and_test(&pchb->file.refcnt))
> +				ppp_destroy_channel(pchb);
> +			break;
> +

The rest looks good to me.