netdev - Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190129223727.GA4062@linux.home>
Date:   Tue, 29 Jan 2019 23:37:28 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Jacob Wen <jian.w.wen@...cle.com>
Cc:     netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3

On Tue, Jan 29, 2019 at 02:18:13PM +0800, Jacob Wen wrote:
> Use pskb_may_pull() to make sure the optional fields are in skb linear
> parts, so we can safely read them in l2tp_recv_common.
> 
Looks fine to me. Just a few nitpicks. Not sure if they're worth a repost.
But if you send a v4, you can:
  * Add the proper Fixes tag.
  * Drop 'net:' from the subsystem prefix ('l2tp:' is enough).
  * Move your patch history inside the commit description.
  * Keep my Acked-by tag.

> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 26f1d435696a..82c28008b438 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>  		goto error;
>  	}
>  
> +	if (tunnel->version != L2TP_HDR_VER_2 &&
> 
Using tunnel->version == L2TP_HDR_VER_3 would have been clearer.

> diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
> index 9c9afe94d389..870f8ccf95f7 100644
> --- a/net/l2tp/l2tp_core.h
> +++ b/net/l2tp/l2tp_core.h
> @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel)
>  }
>  #endif
>  
> +/* Pull optional fields of L2TPv3 */
> +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb,
> 
The comment and function name are a bit misleading: nothing is pulled
here.

> +				   unsigned char **ptr, unsigned char **optr)
> +{
> +	int opt_len = session->peer_cookie_len + l2tp_get_l2specific_len(session);
> +
> +	if (opt_len > 0) {
> +		int off = *ptr - *optr;
> +
> +		if (!pskb_may_pull(skb, off + opt_len))
> +			return -1;
> +
> +		if (skb->data != *optr) {
> +			*optr = skb->data;
> +			*ptr = skb->data + off;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  #define l2tp_printk(ptr, type, func, fmt, ...)				\
>  do {									\
>  	if (((ptr)->debug) & (type))					\

Fixes: f7faffa3ff8e ("l2tp: Add L2TPv3 protocol support")
Fixes: 0d76751fad77 ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")
Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support for IPv6")

Acked-by: Guillaume Nault <gnault@...hat.com>

Thanks for reporting and fixing this.

BTW, Do you plan to also fix L2TPv2?
It looks like defining L2TP_HDR_SIZE_MAX to 14 (size of L2TPv2 header
with all optional fields) and using it in place of L2TP_HDR_SIZE_SEQ in
l2tp_udp_recv_core() should be enough.