bat_socket_read missing checks

B.A.T.M.A.N. Applied Paul 3 participants 8 messages Dec 10, 2011 – Dec 12, 2011

Share & tools

Paul

author

2011-12-10 14:01 UTC #0 lore raw

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

Signature · 2 lines

Regards,
Paul

icmp_socket.c +3 -1

index 5bc8649..f6a6536 100644
@@ -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;

Sven Eckelmann

2011-12-10 14:28 UTC #1 lore raw

PATCH v2 1/3 batman-adv: bat_socket_read missing checks

From: Paul Kot <[email protected]>

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 <[email protected]>

[[email protected]: made it checkpatch clean]

Signed-off-by Sven Eckelmann <[email protected]>

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

icmp_socket.c +2 -2

index 5bc8649..a53502a 100644
@@ -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);

Signature · 1 line

1.7.7.3

Sven Eckelmann

2011-12-10 14:28 UTC #2 lore raw

PATCH v2 2/3 batman-adv: Directly check read of icmp packet in copy_from_user

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 <[email protected]>

---
 icmp_socket.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

icmp_socket.c +1 -6

index a53502a..4dc55e8 100644
@@ -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;
 	}

Signature · 1 line

1.7.7.3

Sven Eckelmann

2011-12-10 14:28 UTC #4 lore raw

PATCH v3 3/3 batman-adv: Only write requested number of byte to user buffer

In reply to 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 <[email protected]>

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

icmp_socket.c +2 -3

index 4dc55e8..5d69e10 100644
@@ -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)

Signature · 1 line

1.7.7.3

Sven Eckelmann

2011-12-10 15:36 UTC #6 lore raw

PATCH v3 1/2 batman-adv: Only write requested number of byte to user buffer

In reply to 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 <[email protected]>

Signed-off-by Sven Eckelmann <[email protected]>

---
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(-)

icmp_socket.c +2 -3

index 5bc8649..66923d2 100644
@@ -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)

Signature · 1 line

1.7.7.3

Marek Lindner

Applied

2011-12-12 10:44 UTC #7 lore raw

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

In reply to Sven Eckelmann

Quoted 8 lines · On Saturday, December 10, 2011 22:28:34 Sven Eckelmann wrote:

From: Paul Kot <[email protected]>
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 <[email protected]>
[[email protected]: made it checkpatch clean]
Signed-off-by Sven Eckelmann <[email protected]>

Applied in revision 2013715.

Thanks,
Marek

Marek Lindner

Applied

2011-12-12 10:45 UTC #3 lore raw

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

In reply to Sven Eckelmann

Quoted 3 lines · 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

Marek Lindner

Applied

2011-12-12 10:48 UTC #5 lore raw

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

In reply to Sven Eckelmann

Quoted 3 lines · 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