On Wed, Jun 20, 2018 at 01:36:49PM +0200, Florian Westphal wrote:
> Máté Eckl <[email protected]> wrote:
> > There are some changes compared to the iptables implementation:
> > - tproxy statement is not terminal here
> > - no transport protocol criterion is necessary to set target ip address
>
> > + const struct nft_tproxy *priv = nft_expr_priv(expr);
> > + struct sk_buff *skb = pkt->skb;
> > + struct sock *sk = skb->sk;
> > + const struct iphdr *iph = ip_hdr(skb);
> > + struct udphdr _hdr, *hp;
> > + __be32 taddr = 0;
> > + __be16 tport = 0;
> > +
> > + hp = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(_hdr), &_hdr);
> > + if (!hp)
> > + regs->verdict.code = NFT_BREAK;
>
> This is missing needed 'return'.
Fixed.
>
> > + /* UDP has no TCP_TIME_WAIT state, so we never enter here */
> > + if (sk && sk->sk_state == TCP_TIME_WAIT)
> > + /* reopening a TIME_WAIT connection needs special handling */
> > + sk = nf_tproxy_handle_time_wait4(nft_net(pkt), skb, taddr,
> > tport, sk);
> > + else if (!sk)
> > + /* no, there's no established connection, check if
> > + * there's a listener on the redirected addr/port */
> > + sk = nf_tproxy_get_sock_v4(nft_net(pkt), skb, hp, iph->protocol,
> > + iph->saddr, taddr,
> > + hp->source, tport,
> > + skb->dev, NF_TPROXY_LOOKUP_LISTENER);
>
> This looks like its subtly broken, inherited from xt_TPROXY.
> Above skb_header_pointer uses sizeof(udphdr) only, but nf_tproxy_get_sock_v4
> assumes it gets tcphdr (it checks th->doff, and that might be garbage).
I thought about why iptables uses udphdr consequently and I think it does
because we do not nead other than source and destination address and port which
is part of the udp header too at the same position.
I think they paid attention to this. nf_tproxy_get_sock_v4 treats that pointer
as a tcphdr indeed, but it only uses tcp-related attributes and functions if ip
protocol is IPPROTO_TCP, so what you described does not happen with an udp
packet.
> So I suggest to remove 'hp' argument from nf_tproxy_get_sock_v4/6 and repeat
> the skb_header_pointer() call there, using struct tcphdr size as backend
> storage for TCP case.
>
> This will need to be a extra patch vs. nf.git tree.
>
> > + if (sk && nf_tproxy_sk_is_transparent(sk)) {
> > + nf_tproxy_assign_sock(skb, sk);
> > + }
>
> No need for extra { }, see scripts/checkpatch.pl (no need to follow
> every advice that script provides of course, decide for yourself).
I thought I removed all of these. I'll pay more attention to these.
>
> > + /* NOTE: assign_sock consumes our sk reference */
> > + if (sk && nf_tproxy_sk_is_transparent(sk)) {
> > + nf_tproxy_assign_sock(skb, sk);
> > + return;
> > + }
> > +
> > + regs->verdict.code = NFT_BREAK;
> > +}
>
> So ipv4 and ipv6 behavce differenty?
> Why does one set BREAK but not the other?
>
> I *guess* its best to BREAK in case sk wasn't assigned for both.
It's an error of mine. I'll review the v4/v6 once more and eliminate these
differences.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
