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

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

[Jacob Wen]

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.

It's easy to reproduce the issue with a net driver that supports paged
skb data. Just create a L2TPv3 over IP tunnel and then generates some
network traffic.
Once reproduced, rx err in /sys/kernel/debug/l2tp/tunnels will increase.

Signed-off-by: Jacob Wen <jian.w.wen@oracle.com>
---
Changes in v3:
1. To keep consistency, move the code out of l2tp_recv_common.
2. Use "net" instead of "net-next", since this is a bug fix.

Changes in v2:
1. Only fix L2TPv3 to make code simple. 
   To fix both L2TPv3 and L2TPv2, we'd better refactor l2tp_recv_common. 
   It's complicated to do so.
2. Reloading pointers after pskb_may_pull
---
 net/l2tp/l2tp_core.c |  4 ++++
 net/l2tp/l2tp_core.h | 21 +++++++++++++++++++++
 net/l2tp/l2tp_ip.c   |  3 +++
 net/l2tp/l2tp_ip6.c  |  3 +++
 4 files changed, 31 insertions(+)

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 &&
+	    l2tp_v3_pull_opt(session, skb, &ptr, &optr))
+		goto error;
+
 	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
 	l2tp_session_dec_refcount(session);
 
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,
+				   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))					\
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 35f6f86d4dcc..adc736d0a553 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -165,6 +165,9 @@ static int l2tp_ip_recv(struct sk_buff *skb)
 		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length);
 	}
 
+	if (l2tp_v3_pull_opt(session, skb, &ptr, &optr))
+		goto discard_sess;
+
 	l2tp_recv_common(session, skb, ptr, optr, 0, skb->len);
 	l2tp_session_dec_refcount(session);
 
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 237f1a4a0b0c..a023bb6eae03 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -178,6 +178,9 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
 		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length);
 	}
 
+	if (l2tp_v3_pull_opt(session, skb, &ptr, &optr))
+		goto discard_sess;
+
 	l2tp_recv_common(session, skb, ptr, optr, 0, skb->len);
 	l2tp_session_dec_refcount(session);
 
-- 
2.17.1

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

[Guillaume Nault]

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@redhat.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.

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

[Jacob Wen]

Thanks for the detailed review.

On 1/30/19 6:37 AM, Guillaume Nault wrote:
> 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.
Done in v4.
>> 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.
Ditto.
>
>> 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.
>
You are right. The misleading is inherited from pskb_may_pull.
s/l2tp_v3_pull_opt/l2tp_v3_ensure_opt_in_linear/ in v4.
>
> 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.
I will do that with your Suggested-by. Thanks.

-- 
Jacob