On Fri, Jul 06, 2018 at 02:35:41PM +0200, Pablo Neira Ayuso wrote:
> Applied with some changes, see below.
>
> On Thu, Jul 05, 2018 at 12:01:53PM +0200, Máté Eckl wrote:
> > v3: linearize based on layer4 protocol.
> > v4: no WARN_ON_ONCE call
>
> Please, next time place these comments...
>
> > -- 8< --
> > This patch fixes a silent out-of-bound read possibility that was present
> > because of the misuse of this function.
> >
> > Mostly it was called with a struct udphdr *hp which had only the udphdr
> > part linearized by the skb_header_pointer, however
> > nf_tproxy_get_sock_v{4,6} uses it as a tcphdr pointer, so some reads for
> > tcp specific attributes may be invalid.
> >
> > Fixes: a583636a83ea ("inet: refactor inet[6]_lookup functions to take skb")
> > Signed-off-by: Máté Eckl <ecklm94@xxxxxxxxx>
> > ---
>
> *Right here*
Ok.
> 'git am' ignores everything coming after ---, so it's very you want to
> place volatile comments like the one above.
>
> More comments below.
>
> > include/net/netfilter/nf_tproxy.h | 4 ++--
> > net/ipv4/netfilter/nf_tproxy_ipv4.c | 16 +++++++++++-----
> > net/ipv6/netfilter/nf_tproxy_ipv6.c | 16 +++++++++++-----
> > net/netfilter/xt_TPROXY.c | 8 ++++----
> > 4 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/net/netfilter/nf_tproxy.h b/include/net/netfilter/nf_tproxy.h
> > index 9754a50ecde9..4cc64c8446eb 100644
> > --- a/include/net/netfilter/nf_tproxy.h
> > +++ b/include/net/netfilter/nf_tproxy.h
> > @@ -64,7 +64,7 @@ nf_tproxy_handle_time_wait4(struct net *net, struct sk_buff *skb,
> > * belonging to established connections going through that one.
> > */
> > struct sock *
> > -nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp,
> > +nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb,
> > const u8 protocol,
> > const __be32 saddr, const __be32 daddr,
> > const __be16 sport, const __be16 dport,
> > @@ -103,7 +103,7 @@ nf_tproxy_handle_time_wait6(struct sk_buff *skb, int tproto, int thoff,
> > struct sock *sk);
> >
> > struct sock *
> > -nf_tproxy_get_sock_v6(struct net *net, struct sk_buff *skb, int thoff, void *hp,
> > +nf_tproxy_get_sock_v6(struct net *net, struct sk_buff *skb, int thoff,
> > const u8 protocol,
> > const struct in6_addr *saddr, const struct in6_addr *daddr,
> > const __be16 sport, const __be16 dport,
> > diff --git a/net/ipv4/netfilter/nf_tproxy_ipv4.c b/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > index 805e83ec3ad9..e2559a1cdbf4 100644
> > --- a/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > +++ b/net/ipv4/netfilter/nf_tproxy_ipv4.c
> > @@ -37,7 +37,7 @@ nf_tproxy_handle_time_wait4(struct net *net, struct sk_buff *skb,
> > * to a listener socket if there's one */
> > struct sock *sk2;
> >
> > - sk2 = nf_tproxy_get_sock_v4(net, skb, hp, iph->protocol,
> > + sk2 = nf_tproxy_get_sock_v4(net, skb, iph->protocol,
> > iph->saddr, laddr ? laddr : iph->daddr,
> > hp->source, lport ? lport : hp->dest,
> > skb->dev, NF_TPROXY_LOOKUP_LISTENER);
> > @@ -71,24 +71,27 @@ __be32 nf_tproxy_laddr4(struct sk_buff *skb, __be32 user_laddr, __be32 daddr)
> > EXPORT_SYMBOL_GPL(nf_tproxy_laddr4);
> >
> > struct sock *
> > -nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp,
> > +nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb,
> > const u8 protocol,
> > const __be32 saddr, const __be32 daddr,
> > const __be16 sport, const __be16 dport,
> > const struct net_device *in,
> > const enum nf_tproxy_lookup_t lookup_type)
> > {
> > + struct tcphdr _hdr, *hp;
> > struct sock *sk;
> > - struct tcphdr *tcph;
> >
> > switch (protocol) {
> > case IPPROTO_TCP:
>
> I have placed the 'struct tcphdr _hdr, *hp;' here, for clarify so...
>
> > + hp = skb_header_pointer(skb, ip_hdrlen(skb),
> > + sizeof(struct tcphdr), &_hdr);
> > + if (hp == NULL)
> > + return NULL;
> > switch (lookup_type) {
> > case NF_TPROXY_LOOKUP_LISTENER:
> > - tcph = hp;
> > sk = inet_lookup_listener(net, &tcp_hashinfo, skb,
> > ip_hdrlen(skb) +
> > - __tcp_hdrlen(tcph),
> > + __tcp_hdrlen(hp),
> > saddr, sport,
> > daddr, dport,
> > in->ifindex, 0);
> > @@ -111,6 +114,9 @@ nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp,
> > }
> > break;
> > case IPPROTO_UDP:
> > + /* hp and _hdr is not used here so skb_header_pointer do not
> > + * need to be called
> > + */
>
> We can skip this comment. Hope you don't mind the mangling.
Ok, no problem.
> Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
