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.
> 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?
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 <[email protected]>
> ---
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
