On Wed, Jun 27, 2018 at 05:10:02PM +0200, Pablo Neira Ayuso wrote: > Hi, > > On Sun, Jun 24, 2018 at 03:03:07PM +0200, Máté Eckl wrote: > > This patch fixes a silent out-of-bound read possibility that was present > > because of the misuse of this function. > > This is a bit confusing. Subject says this is a refactoring, but this > seems not be a clean up, but actually fixing up something. Yes, it is a fix indeed, sorry. > > > 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. > > I think we should rename title to something like: > > netfilter: nf_tproxy: possible non-linear access to transport header > > A "Fixes:" tag would be good? Is this a new bug or it has been > introduced by your recent changes? This code seems to date back to a583636a83ea in 2016. I'll add this to the commit message. > I think we should get this through nf.git, then you will have to wait > a bit to see how this dependency propagates to nf-next.git. > > Another comestic comment below. > > > Signed-off-by: Máté Eckl <ecklm94@xxxxxxxxx> > > --- > > include/net/netfilter/nf_tproxy.h | 4 ++-- > > net/ipv4/netfilter/nf_tproxy_ipv4.c | 15 ++++++++++----- > > net/ipv6/netfilter/nf_tproxy_ipv6.c | 15 ++++++++++----- > > net/netfilter/xt_TPROXY.c | 8 ++++---- > > 4 files changed, 26 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..efbec3b2ad25 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,7 +71,7 @@ __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, > > @@ -79,16 +79,21 @@ nf_tproxy_get_sock_v4(struct net *net, struct sk_buff *skb, void *hp, > > const enum nf_tproxy_lookup_t lookup_type) > > { > > struct sock *sk; > > - struct tcphdr *tcph; > > + struct tcphdr _hdr, *hp; > > While you're updating this code, variable definitions in this form are > prefered: > > struct tcphdr _hdr, *hp; > struct sock *sk; > > Larger line if code first. > > Please, revamp and send v2. > > Thanks Máté. -- 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