linux-kernel - Re: [PATCH net] ppp: fix race conditions in ppp_fill_forward

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: <CANn89iLEMss3VGiJCo=XGVFBSA12bz0y01vVdmBN7WysBLtoUA@mail.gmail.com>
Date: Mon, 11 Aug 2025 02:19:14 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Qingfang Deng <dqfext@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Pablo Neira Ayuso <pablo@...filter.org>, Felix Fietkau <nbd@....name>, linux-ppp@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] ppp: fix race conditions in ppp_fill_forward_path

On Mon, Aug 11, 2025 at 1:44 AM Qingfang Deng <dqfext@...il.com> wrote:
>
> 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_rcu() after removal.
> - Check for a NULL pch->chan before dereferencing it.
>
> Fixes: f6efc675c9dd ("net: ppp: resolve forwarding path for bridge pppoe devices")
> Signed-off-by: Qingfang Deng <dqfext@...il.com>
> ---
>  drivers/net/ppp/ppp_generic.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 8c98cbd4b06d..fd3ac75a56e3 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -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);

It is unclear if rcu_read_lock() is held at this point.

list_first_or_null_rcu() does not have a builtin __list_check_rcu()



> +       if (!pch)
>                 return -ENODEV;
>
> -       pch = list_first_entry(&ppp->channels, struct channel, clist);
>         chan = pch->chan;

chan = READ_ONCE(pch->chan);

And add a WRITE_ONCE(pch->chan, NULL) in ppp_unregister_channel()

And/or add __rcu to pch->chan

> +       if (!chan)
> +               return -ENODEV;
> +
>         if (!chan->ops->fill_forward_path)
>                 return -EOPNOTSUPP;
>
> @@ -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_rcu();

synchronize_net() is preferred.

>                 if (refcount_dec_and_test(&ppp->file.refcnt))
>                         ppp_destroy_interface(ppp);
>                 err = 0;
> --
> 2.43.0
>