[PATCH net-next v2] net: l2tp: fix reading optional fields of L2TPv3

[PATCH net-next v2] 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 later.

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 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 26f1d435696a..e9a17c634c1a 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -625,6 +625,20 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 	int offset;
 	u32 ns, nr;
 
+	if (tunnel->version != L2TP_HDR_VER_2) {
+		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))
+				goto discard;
+			optr = skb->data;
+			ptr = optr + off;
+		}
+	}
+
 	/* Parse and check optional cookie */
 	if (session->peer_cookie_len > 0) {
 		if (memcmp(ptr, &session->peer_cookie[0], session->peer_cookie_len)) {
-- 
2.17.1

Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of L2TPv3

[Guillaume Nault]

On Thu, Jan 24, 2019 at 03:49:17PM +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 later.
> 
> 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 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.
> 
Yes, the L2TP data path definitely needs some care. But for a one-off
patch like this, it'd probably make more sense to respect the current
code structure instead of adding yet more special cases.

I mean, l2tp_recv_common() assumes that it can safely access the L2TP
header: pskb_may_pull() is done in l2tp_udp_recv_core() (which probably
should pull more bytes in case the length field is present BTW).
It's up to l2tp_ip (and l2tp_ip6) to respect this requirement, so that's
where pskb_may_pull() should be done. Yes it'd be better to linearise
data close to the place we access them, but that'd be long term
refactoring. If we don't have the resources to do that, let's just, at
least keep some consistency.

Re: [PATCH net-next v2] net: l2tp: fix reading optional fields of L2TPv3

[Jacob Wen]

On 1/25/19 12:01 AM, Guillaume Nault wrote:
> On Thu, Jan 24, 2019 at 03:49:17PM +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 later.
>>
>> 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 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.
>>
> Yes, the L2TP data path definitely needs some care. But for a one-off
> patch like this, it'd probably make more sense to respect the current
> code structure instead of adding yet more special cases.
Agree. I didn't give l2tp_udp_recv_core enough attention.
> I mean, l2tp_recv_common() assumes that it can safely access the L2TP
> header: pskb_may_pull() is done in l2tp_udp_recv_core() (which probably
> should pull more bytes in case the length field is present BTW).
Yes. Two more bytes are required for L2TPv2.
> It's up to l2tp_ip (and l2tp_ip6) to respect this requirement, so that's
> where pskb_may_pull() should be done. Yes it'd be better to linearise
> data close to the place we access them, but that'd be long term
> refactoring. If we don't have the resources to do that, let's just, at
> least keep some consistency.
I will send a new patch.
Thanks.

-- 
Jacob