netdev - Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched

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: <CANn89iLbTAwG-GM-UBFv4fNJ+1RuUZLMFNDCbUumbXx3SxxfBA@mail.gmail.com>
Date: Wed, 30 Oct 2024 15:18:14 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Breno Leitao <leitao@...ian.org>
Cc: kuba@...nel.org, horms@...nel.org, davem@...emloft.net, pabeni@...hat.com, 
	Matthieu Baerts <matttbe@...nel.org>, Mat Martineau <martineau@...nel.org>, 
	Geliang Tang <geliang@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	vlad.wing@...il.com, max@...sevol.com, kernel-team@...a.com, aehkn@...hub.one, 
	stable@...r.kernel.org, 
	"open list:NETWORKING [MPTCP]" <mptcp@...ts.linux.dev>
Subject: Re: [PATCH net] mptcp: Ensure RCU read lock is held when calling mptcp_sched_find()

On Wed, Oct 30, 2024 at 3:02 PM Breno Leitao <leitao@...ian.org> wrote:
>
> The mptcp_sched_find() function must be called with the RCU read lock
> held, as it accesses RCU-protected data structures. This requirement was
> not properly enforced in the mptcp_init_sock() function, leading to a
> RCU list traversal in a non-reader section error when
> CONFIG_PROVE_RCU_LIST is enabled.
>
>         net/mptcp/sched.c:44 RCU-list traversed in non-reader section!!
>
> Fix it by acquiring the RCU read lock before calling the
> mptcp_sched_find() function. This ensures that the function is invoked
> with the necessary RCU protection in place, as it accesses RCU-protected
> data structures.
>
> Additionally, the patch breaks down the mptcp_init_sched() call into
> smaller parts, with the RCU read lock only covering the specific call to
> mptcp_sched_find(). This helps minimize the critical section, reducing
> the time during which RCU grace periods are blocked.
>
> The mptcp_sched_list_lock is not held in this case, and it is not clear
> if it is necessary.
>
> Signed-off-by: Breno Leitao <leitao@...ian.org>
> Fixes: 1730b2b2c5a5 ("mptcp: add sched in mptcp_sock")
> Cc: stable@...r.kernel.org
> ---
>  net/mptcp/protocol.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 6d0e201c3eb2..8ece630f80d4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2854,6 +2854,7 @@ static void mptcp_ca_reset(struct sock *sk)
>  static int mptcp_init_sock(struct sock *sk)
>  {
>         struct net *net = sock_net(sk);
> +       struct mptcp_sched_ops *sched;
>         int ret;
>
>         __mptcp_init_sock(sk);
> @@ -2864,8 +2865,10 @@ static int mptcp_init_sock(struct sock *sk)
>         if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net))
>                 return -ENOMEM;
>
> -       ret = mptcp_init_sched(mptcp_sk(sk),
> -                              mptcp_sched_find(mptcp_get_scheduler(net)));
> +       rcu_read_lock();
> +       sched = mptcp_sched_find(mptcp_get_scheduler(net));
> +       rcu_read_unlock();

You are silencing the warning, but a potential UAF remains.

sched could have been freed already, it is illegal to deref it.

> +       ret = mptcp_init_sched(mptcp_sk(sk), sched);

The rcu_read_unlock() should be moved after this point.

This means that mptcp_sched_ops ->init() functions are not allowed to sleep.

>         if (ret)
>                 return ret;
>
> --
> 2.43.5
>