Commit d503b30bd648b3cb4e5f50b65d27e389960cc6d9

Authored by Florian Westphal
Committed by Patrick McHardy
1 parent de9963f0f2

netfilter: tproxy: do not assign timewait sockets to skb->sk

Assigning a socket in timewait state to skb->sk can trigger
kernel oops, e.g. in nfnetlink_log, which does:

if (skb->sk) {
        read_lock_bh(&skb->sk->sk_callback_lock);
        if (skb->sk->sk_socket && skb->sk->sk_socket->file) ...

in the timewait case, accessing sk->sk_callback_lock and sk->sk_socket
is invalid.

Either all of these spots will need to add a test for sk->sk_state != TCP_TIME_WAIT,
or xt_TPROXY must not assign a timewait socket to skb->sk.

This does the latter.

If a TW socket is found, assign the tproxy nfmark, but skip the skb->sk assignment,
thus mimicking behaviour of a '-m socket .. -j MARK/ACCEPT' re-routing rule.

The 'SYN to TW socket' case is left unchanged -- we try to redirect to the
listener socket.

Cc: Balazs Scheidler <bazsi@balabit.hu>
Cc: KOVACS Krisztian <hidden@balabit.hu>
Signed-off-by: Florian Westphal <fwestphal@astaro.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>

Showing 4 changed files with 44 additions and 30 deletions Side-by-side Diff

include/net/netfilter/nf_tproxy_core.h
... ... @@ -201,18 +201,8 @@
201 201 }
202 202 #endif
203 203  
204   -static inline void
205   -nf_tproxy_put_sock(struct sock *sk)
206   -{
207   - /* TIME_WAIT inet sockets have to be handled differently */
208   - if ((sk->sk_protocol == IPPROTO_TCP) && (sk->sk_state == TCP_TIME_WAIT))
209   - inet_twsk_put(inet_twsk(sk));
210   - else
211   - sock_put(sk);
212   -}
213   -
214 204 /* assign a socket to the skb -- consumes sk */
215   -int
  205 +void
216 206 nf_tproxy_assign_sock(struct sk_buff *skb, struct sock *sk);
217 207  
218 208 #endif
net/netfilter/nf_tproxy_core.c
... ... @@ -28,26 +28,23 @@
28 28 skb->destructor = NULL;
29 29  
30 30 if (sk)
31   - nf_tproxy_put_sock(sk);
  31 + sock_put(sk);
32 32 }
33 33  
34 34 /* consumes sk */
35   -int
  35 +void
36 36 nf_tproxy_assign_sock(struct sk_buff *skb, struct sock *sk)
37 37 {
38   - bool transparent = (sk->sk_state == TCP_TIME_WAIT) ?
39   - inet_twsk(sk)->tw_transparent :
40   - inet_sk(sk)->transparent;
  38 + /* assigning tw sockets complicates things; most
  39 + * skb->sk->X checks would have to test sk->sk_state first */
  40 + if (sk->sk_state == TCP_TIME_WAIT) {
  41 + inet_twsk_put(inet_twsk(sk));
  42 + return;
  43 + }
41 44  
42   - if (transparent) {
43   - skb_orphan(skb);
44   - skb->sk = sk;
45   - skb->destructor = nf_tproxy_destructor;
46   - return 1;
47   - } else
48   - nf_tproxy_put_sock(sk);
49   -
50   - return 0;
  45 + skb_orphan(skb);
  46 + skb->sk = sk;
  47 + skb->destructor = nf_tproxy_destructor;
51 48 }
52 49 EXPORT_SYMBOL_GPL(nf_tproxy_assign_sock);
53 50  
net/netfilter/xt_TPROXY.c
... ... @@ -33,6 +33,20 @@
33 33 #include <net/netfilter/nf_tproxy_core.h>
34 34 #include <linux/netfilter/xt_TPROXY.h>
35 35  
  36 +static bool tproxy_sk_is_transparent(struct sock *sk)
  37 +{
  38 + if (sk->sk_state != TCP_TIME_WAIT) {
  39 + if (inet_sk(sk)->transparent)
  40 + return true;
  41 + sock_put(sk);
  42 + } else {
  43 + if (inet_twsk(sk)->tw_transparent)
  44 + return true;
  45 + inet_twsk_put(inet_twsk(sk));
  46 + }
  47 + return false;
  48 +}
  49 +
36 50 static inline __be32
37 51 tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
38 52 {
... ... @@ -141,7 +155,7 @@
141 155 skb->dev, NFT_LOOKUP_LISTENER);
142 156  
143 157 /* NOTE: assign_sock consumes our sk reference */
144   - if (sk && nf_tproxy_assign_sock(skb, sk)) {
  158 + if (sk && tproxy_sk_is_transparent(sk)) {
145 159 /* This should be in a separate target, but we don't do multiple
146 160 targets on the same rule yet */
147 161 skb->mark = (skb->mark & ~mark_mask) ^ mark_value;
... ... @@ -149,6 +163,8 @@
149 163 pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n",
150 164 iph->protocol, &iph->daddr, ntohs(hp->dest),
151 165 &laddr, ntohs(lport), skb->mark);
  166 +
  167 + nf_tproxy_assign_sock(skb, sk);
152 168 return NF_ACCEPT;
153 169 }
154 170  
... ... @@ -306,7 +322,7 @@
306 322 par->in, NFT_LOOKUP_LISTENER);
307 323  
308 324 /* NOTE: assign_sock consumes our sk reference */
309   - if (sk && nf_tproxy_assign_sock(skb, sk)) {
  325 + if (sk && tproxy_sk_is_transparent(sk)) {
310 326 /* This should be in a separate target, but we don't do multiple
311 327 targets on the same rule yet */
312 328 skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value;
... ... @@ -314,6 +330,8 @@
314 330 pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n",
315 331 tproto, &iph->saddr, ntohs(hp->source),
316 332 laddr, ntohs(lport), skb->mark);
  333 +
  334 + nf_tproxy_assign_sock(skb, sk);
317 335 return NF_ACCEPT;
318 336 }
319 337  
net/netfilter/xt_socket.c
... ... @@ -35,6 +35,15 @@
35 35 #include <net/netfilter/nf_conntrack.h>
36 36 #endif
37 37  
  38 +static void
  39 +xt_socket_put_sk(struct sock *sk)
  40 +{
  41 + if (sk->sk_state == TCP_TIME_WAIT)
  42 + inet_twsk_put(inet_twsk(sk));
  43 + else
  44 + sock_put(sk);
  45 +}
  46 +
38 47 static int
39 48 extract_icmp4_fields(const struct sk_buff *skb,
40 49 u8 *protocol,
... ... @@ -164,7 +173,7 @@
164 173 (sk->sk_state == TCP_TIME_WAIT &&
165 174 inet_twsk(sk)->tw_transparent));
166 175  
167   - nf_tproxy_put_sock(sk);
  176 + xt_socket_put_sk(sk);
168 177  
169 178 if (wildcard || !transparent)
170 179 sk = NULL;
... ... @@ -298,7 +307,7 @@
298 307 (sk->sk_state == TCP_TIME_WAIT &&
299 308 inet_twsk(sk)->tw_transparent));
300 309  
301   - nf_tproxy_put_sock(sk);
  310 + xt_socket_put_sk(sk);
302 311  
303 312 if (wildcard || !transparent)
304 313 sk = NULL;