Commit 0417adf3 authored by Qingfang Deng's avatar Qingfang Deng Committed by Paolo Abeni
Browse files

ppp: fix race conditions in ppp_fill_forward_path 



ppp_fill_forward_path() has two race conditions:

1. The ppp->channels list can change between list_empty() and
   list_first_entry(), as ppp_lock() is not held. If the only channel
   is deleted in ppp_disconnect_channel(), list_first_entry() may
   access an empty head or a freed entry, and trigger a panic.

2. pch->chan can be NULL. When ppp_unregister_channel() is called,
   pch->chan is set to NULL before pch is removed from ppp->channels.

Fix these by using a lockless RCU approach:
- Use list_first_or_null_rcu() to safely test and access the first list
  entry.
- Convert list modifications on ppp->channels to their RCU variants and
  add synchronize_net() after removal.
- Check for a NULL pch->chan before dereferencing it.

Fixes: f6efc675 ("net: ppp: resolve forwarding path for bridge pppoe devices")
Signed-off-by: default avatarQingfang Deng <dqfext@gmail.com>
Link: https://patch.msgid.link/20250814012559.3705-2-dqfext@gmail.com


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parent 62c30c54

drivers/net/ppp/ppp_generic.c

+11 −6
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ 
#include <linux/ppp_channel.h>

#include <linux/ppp-comp.h>

#include <linux/skbuff.h>

#include <linux/rculist.h>

#include <linux/rtnetlink.h>

#include <linux/if_arp.h>

#include <linux/ip.h>
@@ -1598,11 +1599,14 @@ static int ppp_fill_forward_path(struct net_device_path_ctx *ctx, 
	if (ppp->flags & SC_MULTILINK)

		return -EOPNOTSUPP;



	if (list_empty(&ppp->channels))

	pch = list_first_or_null_rcu(&ppp->channels, struct channel, clist);

	if (!pch)

		return -ENODEV;



	chan = READ_ONCE(pch->chan);

	if (!chan)

		return -ENODEV;



	pch = list_first_entry(&ppp->channels, struct channel, clist);

	chan = pch->chan;

	if (!chan->ops->fill_forward_path)

		return -EOPNOTSUPP;
@@ -2994,7 +2998,7 @@ ppp_unregister_channel(struct ppp_channel *chan) 
	 */

	down_write(&pch->chan_sem);

	spin_lock_bh(&pch->downl);

	pch->chan = NULL;

	WRITE_ONCE(pch->chan, NULL);

	spin_unlock_bh(&pch->downl);

	up_write(&pch->chan_sem);

	ppp_disconnect_channel(pch);
@@ -3515,7 +3519,7 @@ ppp_connect_channel(struct channel *pch, int unit) 
	hdrlen = pch->file.hdrlen + 2;	/* for protocol bytes */

	if (hdrlen > ppp->dev->hard_header_len)

		ppp->dev->hard_header_len = hdrlen;

	list_add_tail(&pch->clist, &ppp->channels);

	list_add_tail_rcu(&pch->clist, &ppp->channels);

	++ppp->n_channels;

	pch->ppp = ppp;

	refcount_inc(&ppp->file.refcnt);
@@ -3545,10 +3549,11 @@ ppp_disconnect_channel(struct channel *pch) 
	if (ppp) {

		/* remove it from the ppp unit's list */

		ppp_lock(ppp);

		list_del(&pch->clist);

		list_del_rcu(&pch->clist);

		if (--ppp->n_channels == 0)

			wake_up_interruptible(&ppp->file.rwait);

		ppp_unlock(ppp);

		synchronize_net();

		if (refcount_dec_and_test(&ppp->file.refcnt))

			ppp_destroy_interface(ppp);

		err = 0;