[PATCH net] udp: fix a potential panic in first_packet_length()

Subsystems: networking [general], the rest, user datagram protocol (udp)

STALE3483d

8 messages, 4 authors, 2017-02-10 · open the first message on its own page

[PATCH net] udp: fix a potential panic in first_packet_length()

From: Eric Dumazet <hidden>
Date: 2017-02-09 15:38:02

From: Eric Dumazet <edumazet@google.com>

first_packet_length() is called from udp_ioctl()

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

We shall call udp_rmem_release() only for UDP variants.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

Fixes: 7c13f97ffde63 ("udp: do fwd memory scheduling on dequeue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
---
 net/ipv4/udp.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 8aab7d78d25bc6eaa42dcc960cdbd5086f614cad..7c0807ee82cec6ca8c856da14fa6109dfdf27868 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c

@@ -1376,7 +1376,11 @@ static int first_packet_length(struct sock *sk)
 		kfree_skb(skb);
 	}
 	res = skb ? skb->len : -1;
-	if (total)
+	/* udp_ioctl() can be used by UDP/UDPLite, but also L2TP.
+	 * We only need to call udp_rmem_release() for UDP sockets.
+	 * L2TP does have a proper skb destructor invoked at kfree_skb() time.
+	 */
+	if (total && sk->sk_prot->memory_allocated == &udp_memory_allocated)
 		udp_rmem_release(sk, total, 1);
 	spin_unlock_bh(&rcvq->lock);
 	return res;

Re: [PATCH net] udp: fix a potential panic in first_packet_length()

From: Paolo Abeni <pabeni@redhat.com>
Date: 2017-02-09 17:01:56

On Thu, 2017-02-09 at 07:30 -0800, Eric Dumazet wrote:

quoted hunk

From: Eric Dumazet <edumazet@google.com>

first_packet_length() is called from udp_ioctl()

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

We shall call udp_rmem_release() only for UDP variants.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

Fixes: 7c13f97ffde63 ("udp: do fwd memory scheduling on dequeue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
---
 net/ipv4/udp.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 8aab7d78d25bc6eaa42dcc960cdbd5086f614cad..7c0807ee82cec6ca8c856da14fa6109dfdf27868 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c

@@ -1376,7 +1376,11 @@ static int first_packet_length(struct sock *sk)
 		kfree_skb(skb);
 	}
 	res = skb ? skb->len : -1;
-	if (total)
+	/* udp_ioctl() can be used by UDP/UDPLite, but also L2TP.
+	 * We only need to call udp_rmem_release() for UDP sockets.
+	 * L2TP does have a proper skb destructor invoked at kfree_skb() time.
+	 */
+	if (total && sk->sk_prot->memory_allocated == &udp_memory_allocated)
 		udp_rmem_release(sk, total, 1);
 	spin_unlock_bh(&rcvq->lock);
 	return res;

My bad, I missed completely that call path.

I'm wondering if calling first_packet_length() for l2tp_ip sockets
makes sense ?!? Am I missing something or it touches udp stats and
checks udp csum for non udp packets ?!?

Paolo

Re: [PATCH net] udp: fix a potential panic in first_packet_length()

From: Eric Dumazet <hidden>
Date: 2017-02-09 17:54:25

On Thu, 2017-02-09 at 18:01 +0100, Paolo Abeni wrote:

 

My bad, I missed completely that call path.

I'm wondering if calling first_packet_length() for l2tp_ip sockets
makes sense ?!? Am I missing something or it touches udp stats and
checks udp csum for non udp packets ?!?

Yes, I guess this is a good point.

I will send a v2, thanks.

[PATCH net] l2tp: do not use udp_ioctl()

From: Eric Dumazet <hidden>
Date: 2017-02-09 18:15:16

From: Eric Dumazet <edumazet@google.com>

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

L2TP should use its own handler, because it really does not
look the same.

SIOCINQ for instance should not assume UDP checksum or headers.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

While crashes only happen on recent kernels (after commit 
7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")), this
probably needs to be backported to older kernels.

Fixes: 7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")
Fixes: 85584672012e ("udp: Fix udp_poll() and ioctl()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 net/l2tp/l2tp_core.h |    1 +
 net/l2tp/l2tp_ip.c   |   26 +++++++++++++++++++++++++-
 net/l2tp/l2tp_ip6.c  |    2 +-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8f560f7140a05694c13904d9b171ba67d9d11292..aebf281d09eeb31c531eb624bd2ddd78cab8da9b 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h

@@ -263,6 +263,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb,
 int l2tp_nl_register_ops(enum l2tp_pwtype pw_type,
 			 const struct l2tp_nl_cmd_ops *ops);
 void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 
 /* Session reference counts. Incremented when code obtains a reference
  * to a session.

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 3d73278b86ca34bfbd774dc8f52e490169445e1b..d4e5d16d97d4b612f8a76557516288f4a011448a 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c

@@ -11,6 +11,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <asm/ioctls.h>
 #include <linux/icmp.h>
 #include <linux/module.h>
 #include <linux/skbuff.h>

@@ -553,6 +554,29 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
 	return err ? err : copied;
 }
 
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg)
+{
+	struct sk_buff *skb;
+	int amount;
+
+	switch (cmd) {
+	case SIOCOUTQ:
+		amount = sk_wmem_alloc_get(sk);
+		break;
+	case SIOCINQ:
+		spin_lock_bh(&sk->sk_receive_queue.lock);
+		skb = skb_peek(&sk->sk_receive_queue);
+		amount = skb ? skb->len : 0;
+		spin_unlock_bh(&sk->sk_receive_queue.lock);
+		break;
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return put_user(amount, (int __user *)arg);
+}
+
 static struct proto l2tp_ip_prot = {
 	.name		   = "L2TP/IP",
 	.owner		   = THIS_MODULE,

@@ -561,7 +585,7 @@ static struct proto l2tp_ip_prot = {
 	.bind		   = l2tp_ip_bind,
 	.connect	   = l2tp_ip_connect,
 	.disconnect	   = l2tp_ip_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip_destroy_sock,
 	.setsockopt	   = ip_setsockopt,
 	.getsockopt	   = ip_getsockopt,

diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 331ccf5a7bad80e011997e071489d7775b0c68c6..f47c45250f86c9189e0a6bbfd92b21cbe2069406 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c

@@ -722,7 +722,7 @@ static struct proto l2tp_ip6_prot = {
 	.bind		   = l2tp_ip6_bind,
 	.connect	   = l2tp_ip6_connect,
 	.disconnect	   = l2tp_ip6_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip6_destroy_sock,
 	.setsockopt	   = ipv6_setsockopt,
 	.getsockopt	   = ipv6_getsockopt,

Re: [PATCH net] l2tp: do not use udp_ioctl()

From: Paolo Abeni <pabeni@redhat.com>
Date: 2017-02-09 21:17:51

On Thu, 2017-02-09 at 10:15 -0800, Eric Dumazet wrote:

quoted hunk

From: Eric Dumazet <edumazet@google.com>

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

L2TP should use its own handler, because it really does not
look the same.

SIOCINQ for instance should not assume UDP checksum or headers.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

While crashes only happen on recent kernels (after commit 
7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")), this
probably needs to be backported to older kernels.

Fixes: 7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")
Fixes: 85584672012e ("udp: Fix udp_poll() and ioctl()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 net/l2tp/l2tp_core.h |    1 +
 net/l2tp/l2tp_ip.c   |   26 +++++++++++++++++++++++++-
 net/l2tp/l2tp_ip6.c  |    2 +-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8f560f7140a05694c13904d9b171ba67d9d11292..aebf281d09eeb31c531eb624bd2ddd78cab8da9b 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h

@@ -263,6 +263,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb,
 int l2tp_nl_register_ops(enum l2tp_pwtype pw_type,
 			 const struct l2tp_nl_cmd_ops *ops);
 void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 
 /* Session reference counts. Incremented when code obtains a reference
  * to a session.

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 3d73278b86ca34bfbd774dc8f52e490169445e1b..d4e5d16d97d4b612f8a76557516288f4a011448a 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c

@@ -11,6 +11,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <asm/ioctls.h>
 #include <linux/icmp.h>
 #include <linux/module.h>
 #include <linux/skbuff.h>

@@ -553,6 +554,29 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
 	return err ? err : copied;
 }
 
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg)
+{
+	struct sk_buff *skb;
+	int amount;
+
+	switch (cmd) {
+	case SIOCOUTQ:
+		amount = sk_wmem_alloc_get(sk);
+		break;
+	case SIOCINQ:
+		spin_lock_bh(&sk->sk_receive_queue.lock);
+		skb = skb_peek(&sk->sk_receive_queue);
+		amount = skb ? skb->len : 0;
+		spin_unlock_bh(&sk->sk_receive_queue.lock);
+		break;
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return put_user(amount, (int __user *)arg);
+}
+
 static struct proto l2tp_ip_prot = {
 	.name		   = "L2TP/IP",
 	.owner		   = THIS_MODULE,

@@ -561,7 +585,7 @@ static struct proto l2tp_ip_prot = {
 	.bind		   = l2tp_ip_bind,
 	.connect	   = l2tp_ip_connect,
 	.disconnect	   = l2tp_ip_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip_destroy_sock,
 	.setsockopt	   = ip_setsockopt,
 	.getsockopt	   = ip_getsockopt,

diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 331ccf5a7bad80e011997e071489d7775b0c68c6..f47c45250f86c9189e0a6bbfd92b21cbe2069406 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c

@@ -722,7 +722,7 @@ static struct proto l2tp_ip6_prot = {
 	.bind		   = l2tp_ip6_bind,
 	.connect	   = l2tp_ip6_connect,
 	.disconnect	   = l2tp_ip6_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip6_destroy_sock,
 	.setsockopt	   = ipv6_setsockopt,
 	.getsockopt	   = ipv6_getsockopt,

Thank you for taking care of this! LGTM.

Acked-by: Paolo Abeni <pabeni@redhat.com>

[PATCH v2 net] l2tp: do not use udp_ioctl()

From: Eric Dumazet <hidden>
Date: 2017-02-10 00:24:22

From: Eric Dumazet <edumazet@google.com>

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

L2TP should use its own handler, because it really does not
look the same.

SIOCINQ for instance should not assume UDP checksum or headers.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

While crashes only happen on recent kernels (after commit 
7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")), this
probably needs to be backported to older kernels.

Fixes: 7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")
Fixes: 85584672012e ("udp: Fix udp_poll() and ioctl()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
Acked-by: Paolo Abeni <pabeni@redhat.com>
---
v2: Adding the EXPORT_SYMBOL(l2tp_ioctl) for ipv6, of course...

 net/l2tp/l2tp_core.h |    1 +
 net/l2tp/l2tp_ip.c   |   27 ++++++++++++++++++++++++++-
 net/l2tp/l2tp_ip6.c  |    2 +-
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8f560f7140a05694c13904d9b171ba67d9d11292..aebf281d09eeb31c531eb624bd2ddd78cab8da9b 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h

@@ -263,6 +263,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb,
 int l2tp_nl_register_ops(enum l2tp_pwtype pw_type,
 			 const struct l2tp_nl_cmd_ops *ops);
 void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
 
 /* Session reference counts. Incremented when code obtains a reference
  * to a session.

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 3d73278b86ca34bfbd774dc8f52e490169445e1b..28c21546d5b60dcd07bbf6347389e97c918bf40f 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c

@@ -11,6 +11,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <asm/ioctls.h>
 #include <linux/icmp.h>
 #include <linux/module.h>
 #include <linux/skbuff.h>

@@ -553,6 +554,30 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
 	return err ? err : copied;
 }
 
+int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg)
+{
+	struct sk_buff *skb;
+	int amount;
+
+	switch (cmd) {
+	case SIOCOUTQ:
+		amount = sk_wmem_alloc_get(sk);
+		break;
+	case SIOCINQ:
+		spin_lock_bh(&sk->sk_receive_queue.lock);
+		skb = skb_peek(&sk->sk_receive_queue);
+		amount = skb ? skb->len : 0;
+		spin_unlock_bh(&sk->sk_receive_queue.lock);
+		break;
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return put_user(amount, (int __user *)arg);
+}
+EXPORT_SYMBOL(l2tp_ioctl);
+
 static struct proto l2tp_ip_prot = {
 	.name		   = "L2TP/IP",
 	.owner		   = THIS_MODULE,

@@ -561,7 +586,7 @@ static struct proto l2tp_ip_prot = {
 	.bind		   = l2tp_ip_bind,
 	.connect	   = l2tp_ip_connect,
 	.disconnect	   = l2tp_ip_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip_destroy_sock,
 	.setsockopt	   = ip_setsockopt,
 	.getsockopt	   = ip_getsockopt,

diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 331ccf5a7bad80e011997e071489d7775b0c68c6..f47c45250f86c9189e0a6bbfd92b21cbe2069406 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c

@@ -722,7 +722,7 @@ static struct proto l2tp_ip6_prot = {
 	.bind		   = l2tp_ip6_bind,
 	.connect	   = l2tp_ip6_connect,
 	.disconnect	   = l2tp_ip6_disconnect,
-	.ioctl		   = udp_ioctl,
+	.ioctl		   = l2tp_ioctl,
 	.destroy	   = l2tp_ip6_destroy_sock,
 	.setsockopt	   = ipv6_setsockopt,
 	.getsockopt	   = ipv6_getsockopt,

Re: [PATCH net] l2tp: do not use udp_ioctl()

From: kbuild test robot <hidden>
Date: 2017-02-10 02:12:05

Hi Eric,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Eric-Dumazet/l2tp-do-not-use-udp_ioctl/20170210-042926
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

quoted

ERROR: "l2tp_ioctl" [net/l2tp/l2tp_ip6.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v2 net] l2tp: do not use udp_ioctl()

From: David Miller <davem@davemloft.net>
Date: 2017-02-10 20:58:50

From: Eric Dumazet <redacted>
Date: Thu, 09 Feb 2017 16:15:52 -0800

From: Eric Dumazet <edumazet@google.com>

udp_ioctl(), as its name suggests, is used by UDP protocols,
but is also used by L2TP :(

L2TP should use its own handler, because it really does not
look the same.

SIOCINQ for instance should not assume UDP checksum or headers.

Thanks to Andrey and syzkaller team for providing the report
and a nice reproducer.

While crashes only happen on recent kernels (after commit 
7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")), this
probably needs to be backported to older kernels.

Fixes: 7c13f97ffde6 ("udp: do fwd memory scheduling on dequeue")
Fixes: 85584672012e ("udp: Fix udp_poll() and ioctl()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andrey Konovalov <redacted>
Acked-by: Paolo Abeni <pabeni@redhat.com>
---
v2: Adding the EXPORT_SYMBOL(l2tp_ioctl) for ipv6, of course...

Applied and queued up for -stable, thanks Eric.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help