l2tp: make magic feather checks more useful

The l2tp tunnel and session structures contain a "magic feather" field
which was originally intended to help trace lifetime bugs in the code.

Since the introduction of the shared kernel refcount code in refcount.h,
and l2tp's porting to those APIs, we are covered by the refcount code's
checks and warnings.  Duplicating those checks in the l2tp code isn't
useful.

However, magic feather checks are still useful to help to detect bugs
stemming from misuse/trampling of the sk_user_data pointer in struct
sock.  The l2tp code makes extensive use of sk_user_data to stash
pointers to the tunnel and session structures, and if another subsystem
overwrites sk_user_data it's important to detect this.

As such, rework l2tp's magic feather checks to focus on validating the
tunnel and session data structures when they're extracted from
sk_user_data.

 * Add a new accessor function l2tp_sk_to_tunnel which contains a magic
   feather check, and is used by l2tp_core and l2tp_ip[6]
 * Comment l2tp_udp_encap_recv which doesn't use this new accessor function
   because of the specific nature of the codepath it is called in
 * Drop l2tp_session_queue_purge's check on the session magic feather:
   it is called from code which is walking the tunnel session list, and
   hence doesn't need validation
 * Drop l2tp_session_free's check on the tunnel magic feather: the
   intention of this check is covered by refcount.h's reference count
   sanity checking
 * Add session magic validation in pppol2tp_ioctl.  On failure return
   -EBADF, which mirrors the approach in pppol2tp_[sg]etsockopt.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index d2672df7e65a4c4069ef59b6fe9b17568ea29d4e..b02b3cc67df049bd1778e10a49de297fce4623c8 100644 (file)
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -120,11 +120,6 @@ static bool l2tp_sk_is_v6(struct sock *sk)
 }
 #endif
 
-static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
-{
-       return sk->sk_user_data;
-}
-
 static inline struct l2tp_net *l2tp_pernet(const struct net *net)
 {
        return net_generic(net, l2tp_net_id);
@@ -162,19 +157,23 @@ static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 
 static void l2tp_session_free(struct l2tp_session *session)
 {
-       struct l2tp_tunnel *tunnel = session->tunnel;
-
        trace_free_session(session);
+       if (session->tunnel)
+               l2tp_tunnel_dec_refcount(session->tunnel);
+       kfree(session);
+}
 
-       if (tunnel) {
+struct l2tp_tunnel *l2tp_sk_to_tunnel(struct sock *sk)
+{
+       struct l2tp_tunnel *tunnel = sk->sk_user_data;
+
+       if (tunnel)
                if (WARN_ON(tunnel->magic != L2TP_TUNNEL_MAGIC))
-                       goto out;
-               l2tp_tunnel_dec_refcount(tunnel);
-       }
+                       return NULL;
 
-out:
-       kfree(session);
+       return tunnel;
 }
+EXPORT_SYMBOL_GPL(l2tp_sk_to_tunnel);
 
 void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)
 {
@@ -782,9 +781,6 @@ static void l2tp_session_queue_purge(struct l2tp_session *session)
 {
        struct sk_buff *skb = NULL;
 
-       if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
-               return;
-
        while ((skb = skb_dequeue(&session->reorder_q))) {
                atomic_long_inc(&session->stats.rx_errors);
                kfree_skb(skb);
@@ -898,9 +894,17 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
        struct l2tp_tunnel *tunnel;
 
+       /* Note that this is called from the encap_rcv hook inside an
+        * RCU-protected region, but without the socket being locked.
+        * Hence we use rcu_dereference_sk_user_data to access the
+        * tunnel data structure rather the usual l2tp_sk_to_tunnel
+        * accessor function.
+        */
        tunnel = rcu_dereference_sk_user_data(sk);
        if (!tunnel)
                goto pass_up;
+       if (WARN_ON(tunnel->magic != L2TP_TUNNEL_MAGIC))
+               goto pass_up;
 
        if (l2tp_udp_recv_core(tunnel, skb))
                goto pass_up;
@@ -1118,7 +1122,7 @@ EXPORT_SYMBOL_GPL(l2tp_xmit_skb);
  */
 static void l2tp_tunnel_destruct(struct sock *sk)
 {
-       struct l2tp_tunnel *tunnel = l2tp_tunnel(sk);
+       struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk);
 
        if (!tunnel)
                goto end;
@@ -1219,7 +1223,7 @@ again:
 /* Tunnel socket destroy hook for UDP encapsulation */
 static void l2tp_udp_encap_destroy(struct sock *sk)
 {
-       struct l2tp_tunnel *tunnel = l2tp_tunnel(sk);
+       struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk);
 
        if (tunnel)
                l2tp_tunnel_delete(tunnel);

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 3ce90c3f3491bb79c317d07916446f0bda148239..cb21d906343e8a569307be62bd951db473d0e572 100644 (file)
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -273,6 +273,11 @@ void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
 /* IOCTL helper for IP encap modules. */
 int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 
+/* Extract the tunnel structure from a socket's sk_user_data pointer,
+ * validating the tunnel magic feather.
+ */
+struct l2tp_tunnel *l2tp_sk_to_tunnel(struct sock *sk);
+
 static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
 {
        switch (session->l2specific_type) {

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 7086d97f293c6e76298c76517d4f4e8a1603f13c..97ae1255fcb69076a5fd5918349d43312840eedd 100644 (file)
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -233,8 +233,8 @@ static void l2tp_ip_close(struct sock *sk, long timeout)
 
 static void l2tp_ip_destroy_sock(struct sock *sk)
 {
+       struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk);
        struct sk_buff *skb;
-       struct l2tp_tunnel *tunnel = sk->sk_user_data;
 
        while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
                kfree_skb(skb);

diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 409ea8927f6cf8a052f17ace5e98941672a1285a..e5e5036257b058af0e0240b6bfa5c2235031f159 100644 (file)
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -247,7 +247,7 @@ static void l2tp_ip6_close(struct sock *sk, long timeout)
 
 static void l2tp_ip6_destroy_sock(struct sock *sk)
 {
-       struct l2tp_tunnel *tunnel = sk->sk_user_data;
+       struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk);
 
        lock_sock(sk);
        ip6_flush_pending_frames(sk);

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 68d2489fc1335adb1666d6d8ed30cdd8df8540ea..aea85f91f05995bb670809c1a29781a410b80288 100644 (file)
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -1065,6 +1065,9 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
                if (!session)
                        return -ENOTCONN;
 
+               if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
+                       return -EBADF;
+
                /* Not defined for tunnels */
                if (!session->session_id && !session->peer_session_id)
                        return -ENOSYS;
@@ -1079,6 +1082,9 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
                if (!session)
                        return -ENOTCONN;
 
+               if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
+                       return -EBADF;
+
                /* Not defined for tunnels */
                if (!session->session_id && !session->peer_session_id)
                        return -ENOSYS;
@@ -1092,6 +1098,9 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
                if (!session)
                        return -ENOTCONN;
 
+               if (WARN_ON(session->magic != L2TP_SESSION_MAGIC))
+                       return -EBADF;
+
                /* Session 0 represents the parent tunnel */
                if (!session->session_id && !session->peer_session_id) {
                        u32 session_id;