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 <tpar...@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;
}
