[PATCH] ieee802154: verify packet size before trying to allocate it

Currently when sending data over datagram, the send function will attempt to
allocate any size passed on from the userspace.

We should make sure that this size is checked and limited. The maximum size
of an IP packet seemed like the safest limit here.

Signed-off-by: Sasha Levin <redacted>
---

Change in v2:
 - Limit by maximum size the protocol supports.

 net/ieee802154/dgram.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/ieee802154/dgram.c b/net/ieee802154/dgram.c
index 6fbb2ad..628498c 100644
--- a/net/ieee802154/dgram.c
+++ b/net/ieee802154/dgram.c

@@ -232,6 +232,11 @@ static int dgram_sendmsg(struct kiocb *iocb, struct sock *sk,
 
 	hlen = LL_RESERVED_SPACE(dev);
 	tlen = dev->needed_tailroom;
+	if (hlen + tlen + size > IEEE802154_MTU) {
+		err = -EMSGSIZE;
+		goto out;
+	}
+		
 	skb = sock_alloc_send_skb(sk, hlen + tlen + size,
 			msg->msg_flags & MSG_DONTWAIT,
 			&err);

-- 
1.7.8.6

On Sun, 10 Jun 2012 13:10:19 +0200
Sasha Levin [off-list ref] wrote:

What stops an overflow at this point. We'll then pass a small value to
sock_alloc_send_skb/sock_alloc_send_pskb and copy a large number of bytes
into it.

This does seem to be already broken, and not fixed by the patch ?

Alan

Hi Alan,

On Sun, 2012-06-10 at 12:24 +0100, Alan Cox wrote:

On Sun, 10 Jun 2012 13:10:19 +0200
Sasha Levin [off-list ref] wrote:

quoted

+	if (hlen + tlen + size > IEEE802154_MTU) {
+		err = -EMSGSIZE;
+		goto out;

What stops an overflow at this point. We'll then pass a small value to
sock_alloc_send_skb/sock_alloc_send_pskb and copy a large number of bytes
into it.

This does seem to be already broken, and not fixed by the patch ?

Alan

Hm, nothing.

I've added this check to prevent users from being able to allocate huge kernel buffers, and haven't though about the overflow case at all. Thanks for pointing it out.

How about something like this instead:

-----8<-----

From: Sasha Levin <redacted>
Date: Sun, 10 Jun 2012 13:08:03 +0200
Subject: [PATCH] ieee802154: verify packet size before trying to allocate it

Currently when sending data over datagram, the send function will attempt to
allocate any size passed on from the userspace.

We should make sure that this size is checked and limited. The maximum size
of an IP packet seemed like the safest limit here.

Signed-off-by: Sasha Levin <redacted>
---
 net/ieee802154/dgram.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ieee802154/dgram.c b/net/ieee802154/dgram.c
index 6fbb2ad..b098b9c 100644
--- a/net/ieee802154/dgram.c
+++ b/net/ieee802154/dgram.c

@@ -230,6 +230,12 @@ static int dgram_sendmsg(struct kiocb *iocb, struct sock *sk,

	mtu = dev->mtu;
	pr_debug("name = %s, mtu = %u\n", dev->name, mtu);

+	if (size > mtu) {
+		pr_debug("size = %Zu, mtu = %u\n", size, mtu);
+		err = -EINVAL;
+		goto out_skb;
+	}
+
	hlen = LL_RESERVED_SPACE(dev);
	tlen = dev->needed_tailroom;
	skb = sock_alloc_send_skb(sk, hlen + tlen + size,

@@ -258,12 +264,6 @@ static int dgram_sendmsg(struct kiocb *iocb, struct sock *sk,

	if (err < 0)
		goto out_skb;

-	if (size > mtu) {
-		pr_debug("size = %Zu, mtu = %u\n", size, mtu);
-		err = -EINVAL;
-		goto out_skb;
-	}
-
	skb->dev = dev;
	skb->sk  = sk;
	skb->protocol = htons(ETH_P_IEEE802154);

On 06/10/2012 01:10 PM, Sasha Levin wrote:

Currently when sending data over datagram, the send function will attempt to
allocate any size passed on from the userspace.

We should make sure that this size is checked and limited. The maximum size
of an IP packet seemed like the safest limit here.

Signed-off-by: Sasha Levin <redacted>

As I understand it this issue was present in the original code that was
introduced in 2.6.31 RC1. Should this therefore be submitted to stable
(in which case David will do so)?

Commit ID 9ec7671603573ede31207eb5b0b3e1aa211b2854

Thanks, Jan

From: Sasha Levin <redacted>
Date: Sun, 10 Jun 2012 13:10:19 +0200

Currently when sending data over datagram, the send function will attempt to
allocate any size passed on from the userspace.

We should make sure that this size is checked and limited. The maximum size
of an IP packet seemed like the safest limit here.

Signed-off-by: Sasha Levin <redacted>

Why not limit to the device MTU?  That's exactly what I suggested
to you.

On Sun, 2012-06-10 at 20:04 -0700, David Miller wrote:

From: Sasha Levin <redacted>
Date: Sun, 10 Jun 2012 13:10:19 +0200

quoted

Currently when sending data over datagram, the send function will attempt to
allocate any size passed on from the userspace.

We should make sure that this size is checked and limited. The maximum size
of an IP packet seemed like the safest limit here.

Signed-off-by: Sasha Levin <redacted>

Why not limit to the device MTU?  That's exactly what I suggested
to you.

That's what I ended up doing in the reply to this mail.