[net] ppp: hold mutex when unbridging channels in unregister path

Channels are bridged using the PPPIOCBRIDGECHAN ioctl, which executes
with the ppp_mutex held.

Unbridging may occur in two code paths: firstly an explicit
PPPIOCUNBRIDGECHAN ioctl, and secondly on channel unregister.  The
latter may occur when closing the /dev/ppp instance or on teardown of
the channel itself.

This opens up a refcount underflow bug if ppp_bridge_channels called via.
ioctl races with ppp_unbridge_channels called via. file release.

The race is triggered by ppp_unbridge_channels taking the error path
through the 'err_unset' label.  In this scenario, pch->bridge has been
set, but no reference will be taken on pch->file because the function
errors out.  Therefore, if ppp_unbridge_channels is called in the window
between pch->bridge being set and pch->bridge being unset, it will
erroneously drop the reference on pch->file and cause a refcount
underflow.

To avoid this, hold the ppp_mutex while calling ppp_unbridge_channels in
the shutdown path.  This serialises the unbridge operation with any
concurrently executing ioctl.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 drivers/net/ppp/ppp_generic.c | 2 ++
 1 file changed, 2 insertions(+)

On Wed, Dec 23, 2020 at 06:47:30PM +0000, Tom Parkin wrote:
> Channels are bridged using the PPPIOCBRIDGECHAN ioctl, which executes
> with the ppp_mutex held.
> 
> Unbridging may occur in two code paths: firstly an explicit
> PPPIOCUNBRIDGECHAN ioctl, and secondly on channel unregister.  The
> latter may occur when closing the /dev/ppp instance or on teardown of
> the channel itself.
> 
> This opens up a refcount underflow bug if ppp_bridge_channels called via.
> ioctl races with ppp_unbridge_channels called via. file release.
> 
> The race is triggered by ppp_unbridge_channels taking the error path

This is actually ppp_bridge_channels.

> through the 'err_unset' label.  In this scenario, pch->bridge has been
> set, but no reference will be taken on pch->file because the function
> errors out.  Therefore, if ppp_unbridge_channels is called in the window
> between pch->bridge being set and pch->bridge being unset, it will
> erroneously drop the reference on pch->file and cause a refcount
> underflow.
> 
> To avoid this, hold the ppp_mutex while calling ppp_unbridge_channels in
> the shutdown path.  This serialises the unbridge operation with any
> concurrently executing ioctl.
> 
> Signed-off-by: Tom Parkin <tparkin@katalix.com>
> ---
>  drivers/net/ppp/ppp_generic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 09c27f7773f9..e87a05fee2db 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -2938,7 +2938,9 @@ ppp_unregister_channel(struct ppp_channel *chan)
>  	list_del(&pch->list);
>  	spin_unlock_bh(&pn->all_channels_lock);
>  
> +	mutex_lock(&ppp_mutex);
>  	ppp_unbridge_channels(pch);
> +	mutex_unlock(&ppp_mutex);
>  
>  	pch->file.dead = 1;
>  	wake_up_interruptible(&pch->file.rwait);
> -- 
> 2.17.1
> 

The problem is that assigning ->bridge and taking a reference on that
channel isn't atomic. Holding ppp_mutex looks like a workaround for
this problem.

I think the refcount should be taken before unlocking ->upl in
ppp_bridge_channels().

Something like this (completely untested):

---- 8< ----
 static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
 {
 	write_lock_bh(&pch->upl);
 	if (pch->ppp ||
 	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
 		write_unlock_bh(&pch->upl);
 		return -EALREADY;
 	}
+
+	refcount_inc(&pchb->file.refcnt);
 	rcu_assign_pointer(pch->bridge, pchb);
 	write_unlock_bh(&pch->upl);

	write_lock_bh(&pchb->upl);
	if (pchb->ppp ||
	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
		write_unlock_bh(&pchb->upl);
		goto err_unset;
	}
+
+	refcount_inc(&pch->file.refcnt);
	rcu_assign_pointer(pchb->bridge, pch);
	write_unlock_bh(&pchb->upl);

-	refcount_inc(&pch->file.refcnt);
-	refcount_inc(&pchb->file.refcnt);
-
	return 0;

err_unset:
	write_lock_bh(&pch->upl);
	RCU_INIT_POINTER(pch->bridge, NULL);
	write_unlock_bh(&pch->upl);
	synchronize_rcu();
+
+	if (refcount_dec_and_test(&pchb->file.refcnt))
+		ppp_destroy_channel(pchb);
+
	return -EALREADY;
}

On  Thu, Dec 24, 2020 at 11:28:18 +0100, Guillaume Nault wrote:
> On Wed, Dec 23, 2020 at 06:47:30PM +0000, Tom Parkin wrote:
> > Channels are bridged using the PPPIOCBRIDGECHAN ioctl, which executes
> > with the ppp_mutex held.
> > 
> > Unbridging may occur in two code paths: firstly an explicit
> > PPPIOCUNBRIDGECHAN ioctl, and secondly on channel unregister.  The
> > latter may occur when closing the /dev/ppp instance or on teardown of
> > the channel itself.
> > 
> > This opens up a refcount underflow bug if ppp_bridge_channels called via.
> > ioctl races with ppp_unbridge_channels called via. file release.
> > 
> > The race is triggered by ppp_unbridge_channels taking the error path
> 
> This is actually ppp_bridge_channels.
> 

Will fix, thanks.

> > through the 'err_unset' label.  In this scenario, pch->bridge has been
> > set, but no reference will be taken on pch->file because the function
> > errors out.  Therefore, if ppp_unbridge_channels is called in the window
> > between pch->bridge being set and pch->bridge being unset, it will
> > erroneously drop the reference on pch->file and cause a refcount
> > underflow.
> > 
> > To avoid this, hold the ppp_mutex while calling ppp_unbridge_channels in
> > the shutdown path.  This serialises the unbridge operation with any
> > concurrently executing ioctl.
> > 
> > Signed-off-by: Tom Parkin <tparkin@katalix.com>
> > ---
> >  drivers/net/ppp/ppp_generic.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 09c27f7773f9..e87a05fee2db 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -2938,7 +2938,9 @@ ppp_unregister_channel(struct ppp_channel *chan)
> >  	list_del(&pch->list);
> >  	spin_unlock_bh(&pn->all_channels_lock);
> >  
> > +	mutex_lock(&ppp_mutex);
> >  	ppp_unbridge_channels(pch);
> > +	mutex_unlock(&ppp_mutex);
> >  
> >  	pch->file.dead = 1;
> >  	wake_up_interruptible(&pch->file.rwait);
> > -- 
> > 2.17.1
> > 
> 
> The problem is that assigning ->bridge and taking a reference on that
> channel isn't atomic. Holding ppp_mutex looks like a workaround for
> this problem.

You're quite right -- that is the underlying issue.

> I think the refcount should be taken before unlocking ->upl in
> ppp_bridge_channels().

Aye, that's the other option :-)

I wasn't sure whether it was better to use the same locking structure
as the ioctl call, or to rework the code to make the two things
effectively atomic as you suggest.

I'll try this approach.

Thanks for your review!

> 
> Something like this (completely untested):
> 
> ---- 8< ----
>  static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
>  {
>  	write_lock_bh(&pch->upl);
>  	if (pch->ppp ||
>  	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
>  		write_unlock_bh(&pch->upl);
>  		return -EALREADY;
>  	}
> +
> +	refcount_inc(&pchb->file.refcnt);
>  	rcu_assign_pointer(pch->bridge, pchb);
>  	write_unlock_bh(&pch->upl);
> 
> 	write_lock_bh(&pchb->upl);
> 	if (pchb->ppp ||
> 	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
> 		write_unlock_bh(&pchb->upl);
> 		goto err_unset;
> 	}
> +
> +	refcount_inc(&pch->file.refcnt);
> 	rcu_assign_pointer(pchb->bridge, pch);
> 	write_unlock_bh(&pchb->upl);
> 
> -	refcount_inc(&pch->file.refcnt);
> -	refcount_inc(&pchb->file.refcnt);
> -
> 	return 0;
> 
> err_unset:
> 	write_lock_bh(&pch->upl);
> 	RCU_INIT_POINTER(pch->bridge, NULL);
> 	write_unlock_bh(&pch->upl);
> 	synchronize_rcu();
> +
> +	if (refcount_dec_and_test(&pchb->file.refcnt))
> +		ppp_destroy_channel(pchb);
> +
> 	return -EALREADY;
> }
>