netdev - Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJ9biqisZ3o_HUw=7cVWcUOD5XVjCg2F2FmAn67KTc8mQ@mail.gmail.com>
Date:   Wed, 24 Apr 2019 04:33:47 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        James Chapman <jchapman@...alix.com>
Subject: Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()

On Wed, Apr 24, 2019 at 2:58 AM Guillaume Nault <gnault@...hat.com> wrote:
>
> On Tue, Apr 23, 2019 at 09:43:26AM -0700, Eric Dumazet wrote:
> > Canonical way to fetch sk_user_data from an encap_rcv() handler called
> > from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(),
> > otherwise compiler might read it multiple times.
> >
> That reminds me the more general problem we have with ->sk_user_data:
> https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
>
> We're not even guarateed that ->sk_user_data points to a struct l2tp_tunnel
> (some external modules can still probably override it).
>

RCU rules should prevail.

An RCU grace period must be enforced if the same UDP socket has its
sk_user_data changed.

Normally, UDP socket observes an RCU grace period at close/destroy
time. (SOCK_RCU_FREE)

If we detect the same UDP socket has not been closed between clearing
sk_user_data
and setting it again to a non NULL value, we must insert a synchronize_rcu()

A generic change could look like the following (but we must check that all
rcu_assign_sk_user_data() callers can sleep)

diff --git a/include/net/sock.h b/include/net/sock.h
index 784cd19d5ff76b53865f79d4580f3fa269fa2408..3cf0dfd9d83a956220d69138339561b62407addd
100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -522,7 +522,6 @@ enum sk_pacing {
 #define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))

 #define rcu_dereference_sk_user_data(sk)
rcu_dereference(__sk_user_data((sk)))
-#define rcu_assign_sk_user_data(sk, ptr)
rcu_assign_pointer(__sk_user_data((sk)), ptr)

 /*
  * SK_CAN_REUSE and SK_NO_REUSE on a socket mean that the socket is OK
@@ -811,6 +810,7 @@ enum sock_flags {
        SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */
        SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */
        SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */
+       SOCK_USER_DATA_SET,
        SOCK_TXTIME,
        SOCK_XDP, /* XDP is attached */
        SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
@@ -2582,4 +2582,15 @@ static inline bool sk_dev_equal_l3scope(struct
sock *sk, int dif)
        return false;
 }

+static inline void rcu_assign_sk_user_data(struct sock *sk, void *ptr)
+{
+       if (ptr) {
+               might_sleep();
+               if (sock_flag(sk, SOCK_USER_DATA_SET))
+                       synchronize_net();
+       }
+       rcu_assign_pointer(__sk_user_data((sk)), ptr);
+       if (ptr)
+               sock_set_flag(sk, SOCK_USER_DATA_SET);
+}
 #endif /* _SOCK_H */



> There were some locking rules defined for setting ->sk_user_data:
> https://lore.kernel.org/netdev/20180124203541.3172-3-tom@quantonium.net/
> Converting all users to either avoid using ->sk_user_data or using it
> under the proper pre-conditions has been on my TODO list for a while...
> I'll try to revive this effort.

Avoiding using sk_user_data seems not practical.

However making sure it is not blindly overwritten by a buggy module is doable.

>
> > Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close")
>                         ^
> Spurious "i". Vi user? :)

I am not a vi user ;)

>
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: James Chapman <jchapman@...alix.com>
> > ---
> >  net/l2tp/l2tp_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> >  {
> >       struct l2tp_tunnel *tunnel;
> >
> > -     tunnel = l2tp_tunnel(sk);
> > +     tunnel = rcu_dereference_sk_user_data(sk);
> >       if (tunnel == NULL)
> >               goto pass_up;
> >
> > --
> > 2.21.0.593.g511ec345e18-goog
> >