[B.A.T.M.A.N.] bat_socket_read missing checks

[B.A.T.M.A.N.] bat_socket_read missing checks

[Paul]

[-- Attachment #1: Type: text/plain, Size: 153 bytes --]

Writing a icmp_packet_rr and then reading icmp_packet can lead to kernel
memory corruption, if __user *buf is just below TASK_SIZE.

-- 
Regards,
Paul



[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 622 bytes --]

diff --git a/icmp_socket.c b/icmp_socket.c
index 5bc8649..f6a6536 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -136,7 +136,9 @@ static ssize_t bat_socket_read(struct file *file, char __user *buf,
 
        spin_unlock_bh(&socket_client->lock);
 
-       error = __copy_to_user(buf, &socket_packet->icmp_packet,
+       //queue can contain packets larger than icmp_packet (like icmp_packet_rr),
+       //so we can't rely just on the access_ok above
+       error = copy_to_user(buf, &socket_packet->icmp_packet,
                               socket_packet->icmp_len);
 
        packet_len = socket_packet->icmp_len;

[B.A.T.M.A.N.] [PATCHv2 1/3] batman-adv: bat_socket_read missing checks

[Sven Eckelmann]

From: Paul Kot <pawlkt@gmail.com>

Writing a icmp_packet_rr and then reading icmp_packet can lead to kernel
memory corruption, if __user *buf is just below TASK_SIZE.

Signed-off-by: Paul Kot <pawlkt@gmail.com>
[sven@narfation.org: made it checkpatch clean]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 icmp_socket.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/icmp_socket.c b/icmp_socket.c
index 5bc8649..a53502a 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -136,8 +136,8 @@ static ssize_t bat_socket_read(struct file *file, char __user *buf,
 
 	spin_unlock_bh(&socket_client->lock);
 
-	error = __copy_to_user(buf, &socket_packet->icmp_packet,
-			       socket_packet->icmp_len);
+	error = copy_to_user(buf, &socket_packet->icmp_packet,
+			     socket_packet->icmp_len);
 
 	packet_len = socket_packet->icmp_len;
 	kfree(socket_packet);
-- 
1.7.7.3

[B.A.T.M.A.N.] [PATCHv2 2/3] batman-adv: Directly check read of icmp packet in copy_from_user

[Sven Eckelmann]

The access_ok read check can be directly done in copy_from_user since a failure
of access_ok is handled the same way as an error in __copy_from_user.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 icmp_socket.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/icmp_socket.c b/icmp_socket.c
index a53502a..4dc55e8 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -187,12 +187,7 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 	skb_reserve(skb, sizeof(struct ethhdr));
 	icmp_packet = (struct icmp_packet_rr *)skb_put(skb, packet_len);
 
-	if (!access_ok(VERIFY_READ, buff, packet_len)) {
-		len = -EFAULT;
-		goto free_skb;
-	}
-
-	if (__copy_from_user(icmp_packet, buff, packet_len)) {
+	if (copy_from_user(icmp_packet, buff, packet_len)) {
 		len = -EFAULT;
 		goto free_skb;
 	}
-- 
1.7.7.3

Re: [B.A.T.M.A.N.] [PATCHv2 2/3] batman-adv: Directly check read of icmp packet in copy_from_user

[Marek Lindner]

On Saturday, December 10, 2011 22:28:35 Sven Eckelmann wrote:
> The access_ok read check can be directly done in copy_from_user since a
> failure of access_ok is handled the same way as an error in
> __copy_from_user.

Applied in revision abe653f.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCHv3 3/3] batman-adv: Only write requested number of byte to user buffer

[Sven Eckelmann]

Don't write more than the requested number of bytes of an batman-adv icmp
packet to the userspace buffer. Otherwise unrelated userspace memory might get
overridden by the kernel.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 icmp_socket.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/icmp_socket.c b/icmp_socket.c
index 4dc55e8..5d69e10 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -136,10 +136,9 @@ static ssize_t bat_socket_read(struct file *file, char __user *buf,
 
 	spin_unlock_bh(&socket_client->lock);
 
-	error = copy_to_user(buf, &socket_packet->icmp_packet,
-			     socket_packet->icmp_len);
+	packet_len = min(count, socket_packet->icmp_len);
+	error = copy_to_user(buf, &socket_packet->icmp_packet, packet_len);
 
-	packet_len = socket_packet->icmp_len;
 	kfree(socket_packet);
 
 	if (error)
-- 
1.7.7.3

Re: [B.A.T.M.A.N.] [PATCHv3 3/3] batman-adv: Only write requested number of byte to user buffer

[Marek Lindner]

On Saturday, December 10, 2011 22:28:36 Sven Eckelmann wrote:
> Don't write more than the requested number of bytes of an batman-adv icmp
> packet to the userspace buffer. Otherwise unrelated userspace memory might
> get overridden by the kernel.

Applied in revision d22e13c.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCHv3 1/2] batman-adv: Only write requested number of byte to user buffer

[Sven Eckelmann]

Don't write more than the requested number of bytes of an batman-adv icmp
packet to the userspace buffer. Otherwise unrelated userspace memory might get
overwritten by the kernel.

Reported-by: Paul Kot <pawlkt@gmail.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Marek pointed out that it is better to merge patch 1 and 2. I think it doesn't
make sense to leave Paul Kot as author because it doesn't look like his patch
at all.

And thanks to Andrew for s/overridden/overwritten/

 icmp_socket.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/icmp_socket.c b/icmp_socket.c
index 5bc8649..66923d2 100644
--- a/icmp_socket.c
+++ b/icmp_socket.c
@@ -136,10 +136,9 @@ static ssize_t bat_socket_read(struct file *file, char __user *buf,
 
 	spin_unlock_bh(&socket_client->lock);
 
-	error = __copy_to_user(buf, &socket_packet->icmp_packet,
-			       socket_packet->icmp_len);
+	packet_len = min(count, socket_packet->icmp_len);
+	error = copy_to_user(buf, &socket_packet->icmp_packet, packet_len);
 
-	packet_len = socket_packet->icmp_len;
 	kfree(socket_packet);
 
 	if (error)
-- 
1.7.7.3

Re: [B.A.T.M.A.N.] [PATCHv2 1/3] batman-adv: bat_socket_read missing checks

[Marek Lindner]

On Saturday, December 10, 2011 22:28:34 Sven Eckelmann wrote:
> From: Paul Kot <pawlkt@gmail.com>
> 
> Writing a icmp_packet_rr and then reading icmp_packet can lead to kernel
> memory corruption, if __user *buf is just below TASK_SIZE.
> 
> Signed-off-by: Paul Kot <pawlkt@gmail.com>
> [sven@narfation.org: made it checkpatch clean]
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Applied in revision 2013715.

Thanks,
Marek