net: socket: NULL ptr deref in sendmsg

net: socket: NULL ptr deref in sendmsg

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel with the KASAN patchset, I've stumbled on the following spew:

[ 4448.949424] ==================================================================
[ 4448.951737] AddressSanitizer: user-memory-access on address 0
[ 4448.952988] Read of size 2 by thread T19638:
[ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
[ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
[ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
[ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
[ 4448.961266] Call Trace:
[ 4448.963158] dump_stack (lib/dump_stack.c:52)
[ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
[ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
[ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
[ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
[ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
[ 4448.970103] sock_sendmsg (net/socket.c:654)
[ 4448.971584] ? might_fault (mm/memory.c:3741)
[ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
[ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
[ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
[ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
[ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
[ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
[ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
[ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
[ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
[ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
[ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
[ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
[ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
[ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
[ 4448.988929] ==================================================================

It's similar to another variation:

[ 2918.108434] ==================================================================
[ 2918.109923] AddressSanitizer: user-memory-access on address 4
[ 2918.111600] Read of size 4 by thread T5793:
[ 2918.112867] CPU: 4 PID: 5793 Comm: trinity-c4 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
[ 2918.114335]  ffff8805da310700 0000000000000000 0000000000000000 ffff880458b239b0
[ 2918.115632]  ffffffff85e47068 ffff880458b239d8 ffff880458b239c8 ffffffff8142708d
[ 2918.116904]  ffff880458b23e78 ffff880458b239f8 ffffffff81425811 0000000000000004
[ 2918.118075] Call Trace:
[ 2918.118583] dump_stack (lib/dump_stack.c:52)
[ 2918.119449] kasan_report_user_access (mm/kasan/report.c:184)
[ 2918.120928] __asan_load4 (mm/kasan/kasan.c:358)
[ 2918.121916] ? raw_sendmsg (net/ipv4/raw.c:507)
[ 2918.122893] raw_sendmsg (net/ipv4/raw.c:507)
[ 2918.124048] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:304)
[ 2918.124895] ? sched_clock_local (kernel/sched/clock.c:214)
[ 2918.125901] ? get_parent_ip (kernel/sched/core.c:2555)
[ 2918.126741] ? check_chain_key (kernel/locking/lockdep.c:2188)
[ 2918.127657] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
[ 2918.128617] inet_sendmsg (net/ipv4/af_inet.c:738)
[ 2918.129546] ? inet_sendmsg (net/ipv4/af_inet.c:727)
[ 2918.130886] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
[ 2918.132088] sock_sendmsg (net/socket.c:654)
[ 2918.132891] ? might_fault (mm/memory.c:3741)
[ 2918.133765] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
[ 2918.134626] ? verify_iovec (net/core/iovec.c:64)
[ 2918.135654] ___sys_sendmsg (net/socket.c:2096)
[ 2918.136649] ? pvclock_clocksource_read (arch/x86/kernel/pvclock.c:83)
[ 2918.137792] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[ 2918.138682] ? sched_clock (./arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:304)
[ 2918.139686] ? __lock_is_held (kernel/locking/lockdep.c:3513)
[ 2918.140907] ? sockfd_lookup_light (net/socket.c:461)
[ 2918.141862] __sys_sendmmsg (net/socket.c:2181)
[ 2918.142744] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
[ 2918.144719] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
[ 2918.146433] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
[ 2918.148317] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
[ 2918.150321] SyS_sendmmsg (net/socket.c:2201)
[ 2918.151971] tracesys (arch/x86/kernel/entry_64.S:542)
[ 2918.153398] ==================================================================

I've tried debugging it, but I don't see a code path that could lead to that.


Thanks,
Sasha

Re: net: socket: NULL ptr deref in sendmsg

[David Miller]

From: Sasha Levin <sasha.levin@oracle.com>
Date: Sun, 13 Jul 2014 17:50:53 -0400

> While fuzzing with trinity inside a KVM tools guest running the latest -next
> kernel with the KASAN patchset, I've stumbled on the following spew:
 ...
> It's similar to another variation:
 ...
> I've tried debugging it, but I don't see a code path that could lead to that.

Both of these cases involve working with pointers declared with
DECLARE_SOCKADDR, maybe that somehow confuses ASAN code generation?

Re: net: socket: NULL ptr deref in sendmsg

[Sasha Levin]

On 07/14/2014 06:08 PM, David Miller wrote:
> From: Sasha Levin <sasha.levin@oracle.com>
> Date: Sun, 13 Jul 2014 17:50:53 -0400
> 
>> While fuzzing with trinity inside a KVM tools guest running the latest -next
>> kernel with the KASAN patchset, I've stumbled on the following spew:
>  ...
>> It's similar to another variation:
>  ...
>> I've tried debugging it, but I don't see a code path that could lead to that.
> 
> Both of these cases involve working with pointers declared with
> DECLARE_SOCKADDR, maybe that somehow confuses ASAN code generation?
> 

Hey David,

Sorry for the delay.

I've confirmed that it's not ASAN's fault by adding:

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 1b38f7f..81d86b9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2331,7 +2331,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *so
        err = scm_send(sock, msg, siocb->scm, true);
        if (err < 0)
                return err;
-
+       BUG_ON(msg->msg_namelen && !msg->msg_name);
        if (msg->msg_namelen) {
                err = -EINVAL;
                if (addr->nl_family != AF_NETLINK)

And got:

[ 1322.890135] kernel BUG at net/netlink/af_netlink.c:2334!
[ 1322.890135] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 1322.890135] Dumping ftrace buffer:
[ 1322.890135]    (ftrace buffer empty)
[ 1322.890135] Modules linked in:
[ 1322.890135] CPU: 8 PID: 31343 Comm: trinity-c259 Not tainted 3.16.0-rc6-next-20140724-sasha-00046-g7324c87-dirty #931
[ 1322.890135] task: ffff880311268000 ti: ffff88031bf5c000 task.ti: ffff88031bf5c000
[ 1322.890135] RIP: 0010:[<ffffffffb567e01b>]  [<ffffffffb567e01b>] netlink_sendmsg+0xc6b/0xce0
[ 1322.902991] RSP: 0018:ffff88031bf5faa0  EFLAGS: 00010246
[ 1322.902991] RAX: 0000000000000000 RBX: ffff88031bf5fb38 RCX: dfff97060a600000
[ 1322.902991] RDX: ffff88031bf5fe80 RSI: 0000000000000000 RDI: ffff88031bf5fe80
[ 1322.902991] RBP: ffff88031bf5fb80 R08: dfff97060a600000 R09: 0000000000000000
[ 1322.902991] R10: 0000000000000080 R11: 0000000000000001 R12: ffff88031bf5fe78
[ 1322.902991] R13: ffff8801d18fd388 R14: 0000000000000000 R15: 0000000000feff98
[ 1322.902991] FS:  00007f67138b8700(0000) GS:ffff8801de000000(0000) knlGS:0000000000000000
[ 1322.902991] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1322.902991] CR2: 00007f6708260088 CR3: 000000036ad59000 CR4: 00000000000006a0
[ 1322.902991] Stack:
[ 1322.902991]  ffff8801de1e2dc0 ffff88025efbb118 ffffffffb9b9ae30 000000000000092d
[ 1322.902991]  ffff880311268d00 ffff88031bf5fae0 ffffffffb121185d 0000000000000001
[ 1322.902991]  ffff88031bf5faf8 ffff88031bf5fea8 ffff8801d7d9c220 0000000000000000
[ 1322.902991] Call Trace:
[ 1322.902991]  [<ffffffffb121185d>] ? get_parent_ip+0xd/0x50
[ 1322.902991]  [<ffffffffb559bc3a>] sock_sendmsg+0xca/0x100
[ 1322.902991]  [<ffffffffb13b32ed>] ? might_fault+0xed/0x100
[ 1322.902991]  [<ffffffffb13b327a>] ? might_fault+0x7a/0x100
[ 1322.902991]  [<ffffffffb55b3ced>] ? verify_iovec+0xcd/0x180
[ 1322.902991]  [<ffffffffb559cb52>] ___sys_sendmsg+0x312/0x530
[ 1322.902991]  [<ffffffffb124f42e>] ? put_lock_stats.isra.13+0xe/0x30
[ 1322.902991]  [<ffffffffb124fad1>] ? lock_release_holdtime+0x121/0x260
[ 1322.902991]  [<ffffffffb125b2bb>] ? lock_release_non_nested+0x42b/0x4f0
[ 1322.902991]  [<ffffffffb124f004>] ? check_chain_key+0x1f4/0x2e0
[ 1322.902991]  [<ffffffffb559daeb>] __sys_sendmmsg+0x9b/0x1c0
[ 1322.902991]  [<ffffffffb125496d>] ? trace_hardirqs_on_caller+0x1ad/0x380
[ 1322.902991]  [<ffffffffb1254b4d>] ? trace_hardirqs_on+0xd/0x10
[ 1322.902991]  [<ffffffffb10b9222>] ? syscall_trace_enter+0x1e2/0x540
[ 1322.902991]  [<ffffffffb125496d>] ? trace_hardirqs_on_caller+0x1ad/0x380
[ 1322.902991]  [<ffffffffb559dc22>] SyS_sendmmsg+0x12/0x30
[ 1322.902991]  [<ffffffffb5e43a13>] tracesys+0xe1/0xe6
[ 1322.902991] Code: e4 00 00 00 8b 4d 98 45 31 c9 41 b8 d0 00 00 00 48 89 de 8b 55 90 48 c7 04 24 00 00 00 00 4c 89 ef e8 da cb ff ff e9 8d f8 ff ff <0f> 0b e8 5e 3f b9 fb 48 8b bd 68 ff ff ff e8 c2 be da fb 48 8b
[ 1322.902991] RIP  [<ffffffffb567e01b>] netlink_sendmsg+0xc6b/0xce0
[ 1322.902991]  RSP <ffff88031bf5faa0>


Thanks,
Sasha

Re: net: socket: NULL ptr deref in sendmsg

[Andrey Ryabinin]

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

On 07/14/14 01:50, Sasha Levin wrote:

> 
> I've tried debugging it, but I don't see a code path that could lead to that.
> 

I finally found some time to take look at this and I've found where the problem is.

Sasha, I suppose there was no usual "Unable to handle NULL pointer deference" after KASAN's report, right?

This gave me a clue that address 0 is actually mapped and contains valid socket address structure in it.
I've managed to write a simple code (in attachment), which could easily reproduce this bug.

I've fixed it with the following patch, please take a look.


From: Andrey Ryabinin <a.ryabinin@samsung.com>
Subject: [PATCH] net: sendmsg: fix NULL pointer dereference

Sasha's report:
	> While fuzzing with trinity inside a KVM tools guest running the latest -next
	> kernel with the KASAN patchset, I've stumbled on the following spew:
	>
	> [ 4448.949424] ==================================================================
	> [ 4448.951737] AddressSanitizer: user-memory-access on address 0
	> [ 4448.952988] Read of size 2 by thread T19638:
	> [ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
	> [ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
	> [ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
	> [ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
	> [ 4448.961266] Call Trace:
	> [ 4448.963158] dump_stack (lib/dump_stack.c:52)
	> [ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
	> [ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
	> [ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
	> [ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
	> [ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
	> [ 4448.970103] sock_sendmsg (net/socket.c:654)
	> [ 4448.971584] ? might_fault (mm/memory.c:3741)
	> [ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
	> [ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
	> [ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
	> [ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
	> [ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
	> [ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
	> [ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
	> [ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
	> [ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
	> [ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
	> [ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
	> [ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
	> [ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
	> [ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
	> [ 4448.988929] ==================================================================

This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.

After this report there was no usual "Unable to handle kernel NULL pointer dereference"
and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.

This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
(net: rework recvmsg handler msg_name and msg_namelen logic).
Commit message states that:
	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
	 affect sendto as it would bail out earlier while trying to copy-in the
	 address."
But in fact this affects sendto when address 0 is mapped and contains
socket address structure in it. In such case copy-in address will succeed,
verify_iovec() function will successfully exit with msg->msg_namelen > 0
and msg->msg_name == NULL.

This patch fixes it by assigning m->msg_name to address if move_addr_to_kernel()
was successful.

Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: <stable@vger.kernel.org>
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 net/compat.c     | 6 ++++--
 net/core/iovec.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/compat.c b/net/compat.c
index 9a76eaf..eefd989 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -92,9 +92,11 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
 						      kern_address);
 			if (err < 0)
 				return err;
-		}
-		if (kern_msg->msg_name)
+
 			kern_msg->msg_name = kern_address;
+		} else
+			if (kern_msg->msg_name)
+				kern_msg->msg_name = kern_address;
 	} else
 		kern_msg->msg_name = NULL;

diff --git a/net/core/iovec.c b/net/core/iovec.c
index 827dd6b..16bd954 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -47,9 +47,11 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *a
 						  address);
 			if (err < 0)
 				return err;
-		}
-		if (m->msg_name)
+
 			m->msg_name = address;
+		} else
+			if (m->msg_name)
+				m->msg_name = address;
 	} else {
 		m->msg_name = NULL;
 	}
-- 
1.8.5.5




[-- Attachment #2: sendmsg_nl.c --]
[-- Type: text/x-csrc, Size: 1449 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <memory.h>
#include <sys/mman.h>
#include <sys/socket.h>
#include <linux/netlink.h>

#define PAYLOAD 1024

int main(void)
{
	struct sockaddr_nl src_addr;
	struct sockaddr_nl *dest_addr;
	struct nlmsghdr *nlh = NULL;
	struct iovec iov;
	struct msghdr msg;
	int fd;

	memset(&msg, 0, sizeof(msg));

	fd = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
	if (fd < 0) {
		perror("socket:");
		return -1;
	}

	dest_addr = mmap(0, sizeof(*dest_addr), PROT_READ|PROT_WRITE,
			MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
	if (dest_addr == MAP_FAILED) {
		perror("mmap:");
		close(fd);
		return -1;
	}

	memset(&src_addr, 0, sizeof(src_addr));
	src_addr.nl_family = AF_NETLINK;
	src_addr.nl_pid = getpid();

	bind(fd, (struct sockaddr*)&src_addr, sizeof(src_addr));

	memset(dest_addr, 0, sizeof(*dest_addr));
	dest_addr->nl_family = AF_NETLINK;
	dest_addr->nl_pid = 0;
	dest_addr->nl_groups = 0;

	nlh = (struct nlmsghdr *)malloc(NLMSG_SPACE(PAYLOAD));
	memset(nlh, 0, NLMSG_SPACE(PAYLOAD));
	nlh->nlmsg_len = NLMSG_SPACE(PAYLOAD);
	nlh->nlmsg_pid = getpid();
	nlh->nlmsg_flags = 0;

	strcpy(NLMSG_DATA(nlh), "Die kernel! Die!");

	iov.iov_base = (void *)nlh;
	iov.iov_len = nlh->nlmsg_len;
	msg.msg_name = dest_addr;
	msg.msg_namelen = sizeof(*dest_addr);
	msg.msg_iov = &iov;
	msg.msg_iovlen = 1;

	if (sendmsg(fd, &msg, 0) < 0) {
		perror("sendmsg:");
		close(fd);
		return -1;
	}

	close(fd);
	return 0;
}

Re: net: socket: NULL ptr deref in sendmsg

[Eric Dumazet]

On Fri, 2014-07-25 at 19:23 +0400, Andrey Ryabinin wrote:
> On 07/14/14 01:50, Sasha Levin wrote:
> 
> > 
> > I've tried debugging it, but I don't see a code path that could lead to that.
> > 
> 
> I finally found some time to take look at this and I've found where the problem is.
> 
> Sasha, I suppose there was no usual "Unable to handle NULL pointer deference" after KASAN's report, right?
> 
> This gave me a clue that address 0 is actually mapped and contains valid socket address structure in it.
> I've managed to write a simple code (in attachment), which could easily reproduce this bug.
> 
> I've fixed it with the following patch, please take a look.
> 
> 
> From: Andrey Ryabinin <a.ryabinin@samsung.com>
> Subject: [PATCH] net: sendmsg: fix NULL pointer dereference
> 
> Sasha's report:
> 	> While fuzzing with trinity inside a KVM tools guest running the latest -next
> 	> kernel with the KASAN patchset, I've stumbled on the following spew:
> 	>
> 	> [ 4448.949424] ==================================================================
> 	> [ 4448.951737] AddressSanitizer: user-memory-access on address 0
> 	> [ 4448.952988] Read of size 2 by thread T19638:
> 	> [ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
> 	> [ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
> 	> [ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
> 	> [ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
> 	> [ 4448.961266] Call Trace:
> 	> [ 4448.963158] dump_stack (lib/dump_stack.c:52)
> 	> [ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
> 	> [ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
> 	> [ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
> 	> [ 4448.970103] sock_sendmsg (net/socket.c:654)
> 	> [ 4448.971584] ? might_fault (mm/memory.c:3741)
> 	> [ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
> 	> [ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
> 	> [ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
> 	> [ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
> 	> [ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
> 	> [ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
> 	> [ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
> 	> [ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
> 	> [ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
> 	> [ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
> 	> [ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
> 	> [ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
> 	> [ 4448.988929] ==================================================================
> 
> This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.
> 
> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
> 
> This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
> (net: rework recvmsg handler msg_name and msg_namelen logic).
> Commit message states that:
> 	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
> 	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
> 	 affect sendto as it would bail out earlier while trying to copy-in the
> 	 address."
> But in fact this affects sendto when address 0 is mapped and contains
> socket address structure in it. In such case copy-in address will succeed,
> verify_iovec() function will successfully exit with msg->msg_namelen > 0
> and msg->msg_name == NULL.
> 
> This patch fixes it by assigning m->msg_name to address if move_addr_to_kernel()
> was successful.
> 
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> ---
>  net/compat.c     | 6 ++++--
>  net/core/iovec.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/compat.c b/net/compat.c
> index 9a76eaf..eefd989 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -92,9 +92,11 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
>  						      kern_address);
>  			if (err < 0)
>  				return err;
> -		}
> -		if (kern_msg->msg_name)
> +
>  			kern_msg->msg_name = kern_address;
> +		} else
> +			if (kern_msg->msg_name)
> +				kern_msg->msg_name = kern_address;


I agree with your fix, but could you add {} to the else clause.
		
		if {
			multi
			line
			body
		} else {
			if (kern_msg->msg_name)
				kern_msg->msg_name = kern_address;
		}




>  	} else
>  		kern_msg->msg_name = NULL;
> 
> diff --git a/net/core/iovec.c b/net/core/iovec.c
> index 827dd6b..16bd954 100644
> --- a/net/core/iovec.c
> +++ b/net/core/iovec.c
> @@ -47,9 +47,11 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *a
>  						  address);
>  			if (err < 0)
>  				return err;
> -		}
> -		if (m->msg_name)
> +
>  			m->msg_name = address;
> +		} else
> +			if (m->msg_name)
> +				m->msg_name = address;

same here.

>  	} else {
>  		m->msg_name = NULL;
>  	}

Re: net: socket: NULL ptr deref in sendmsg

[Sasha Levin]

On 07/25/2014 11:23 AM, Andrey Ryabinin wrote:
> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.

Interesting. Does it mean that all network protocols that check it for being NULL instead of checking
the length are incorrect?

(such as:)

        if (msg->msg_name) {
                DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);

                [...]


Thanks,
Sasha

Re: net: socket: NULL ptr deref in sendmsg

[Hannes Frederic Sowa]

On Fr, 2014-07-25 at 16:52 -0400, Sasha Levin wrote:
> On 07/25/2014 11:23 AM, Andrey Ryabinin wrote:
> > After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> > and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
> 
> Interesting. Does it mean that all network protocols that check it for being NULL instead of checking
> the length are incorrect?

I would not like to go down this route and keep msg->msg_namelen and
msg->msg_name in sync after verify_iovec.

> (such as:)
> 
>         if (msg->msg_name) {
>                 DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
> 
>                 [...]
> 

Thanks,
Hannes

Re: net: socket: NULL ptr deref in sendmsg

[Andrey Ryabinin]

2014-07-26 0:52 GMT+04:00 Sasha Levin <sasha.levin@oracle.com>:
> On 07/25/2014 11:23 AM, Andrey Ryabinin wrote:
>> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
>> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
>
> Interesting. Does it mean that all network protocols that check it for being NULL instead of checking
> the length are incorrect?
>

I think they are correct. After verify_iovec() we should have either
both msg->msg_name == 0 and msg->msg_namelen == 0,
or  both != 0 (and msg_name should be a kernel address).

That bug allows to leave verify_iovec() with msg_namelen > 0 and
msg_name == NULL, causing troubles for protocols checking only
msg_namelen.

> (such as:)
>
>         if (msg->msg_name) {
>                 DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
>
>                 [...]
>
>
> Thanks,
> Sasha
>

-- 
Best regards,
Andrey Ryabinin

Re: net: socket: NULL ptr deref in sendmsg

[Hannes Frederic Sowa]

On Fr, 2014-07-25 at 19:23 +0400, Andrey Ryabinin wrote:
> On 07/14/14 01:50, Sasha Levin wrote:
> 
> > 
> > I've tried debugging it, but I don't see a code path that could lead to that.
> > 
> 
> I finally found some time to take look at this and I've found where the problem is.
> 
> Sasha, I suppose there was no usual "Unable to handle NULL pointer deference" after KASAN's report, right?
> 
> This gave me a clue that address 0 is actually mapped and contains valid socket address structure in it.
> I've managed to write a simple code (in attachment), which could easily reproduce this bug.
> 
> I've fixed it with the following patch, please take a look.
> 
> 
> From: Andrey Ryabinin <a.ryabinin@samsung.com>
> Subject: [PATCH] net: sendmsg: fix NULL pointer dereference
> 
> Sasha's report:
> 	> While fuzzing with trinity inside a KVM tools guest running the latest -next
> 	> kernel with the KASAN patchset, I've stumbled on the following spew:
> 	>
> 	> [ 4448.949424] ==================================================================
> 	> [ 4448.951737] AddressSanitizer: user-memory-access on address 0
> 	> [ 4448.952988] Read of size 2 by thread T19638:
> 	> [ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
> 	> [ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
> 	> [ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
> 	> [ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
> 	> [ 4448.961266] Call Trace:
> 	> [ 4448.963158] dump_stack (lib/dump_stack.c:52)
> 	> [ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
> 	> [ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
> 	> [ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
> 	> [ 4448.970103] sock_sendmsg (net/socket.c:654)
> 	> [ 4448.971584] ? might_fault (mm/memory.c:3741)
> 	> [ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
> 	> [ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
> 	> [ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
> 	> [ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
> 	> [ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
> 	> [ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
> 	> [ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
> 	> [ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
> 	> [ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
> 	> [ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
> 	> [ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
> 	> [ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
> 	> [ 4448.988929] ==================================================================
> 
> This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.
> 
> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
> 
> This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
> (net: rework recvmsg handler msg_name and msg_namelen logic).
> Commit message states that:
> 	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
> 	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
> 	 affect sendto as it would bail out earlier while trying to copy-in the
> 	 address."
> But in fact this affects sendto when address 0 is mapped and contains
> socket address structure in it. In such case copy-in address will succeed,
> verify_iovec() function will successfully exit with msg->msg_namelen > 0
> and msg->msg_name == NULL.
> 
> This patch fixes it by assigning m->msg_name to address if move_addr_to_kernel()
> was successful.
> 
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> ---
>  net/compat.c     | 6 ++++--
>  net/core/iovec.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/compat.c b/net/compat.c
> index 9a76eaf..eefd989 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -92,9 +92,11 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
>  						      kern_address);
>  			if (err < 0)
>  				return err;
> -		}
> -		if (kern_msg->msg_name)
> +
>  			kern_msg->msg_name = kern_address;
> +		} else
> +			if (kern_msg->msg_name)
> +				kern_msg->msg_name = kern_address;
>  	} else
>  		kern_msg->msg_name = NULL;
> 

Thanks for looking at this! I certainly have overlooked this case.

I wonder, if we allow sendto with valid NULL pointer and positive
msg_namelen to work, why don't we do the same for recvmsg, as in
replacing the VERIFY_WRITE case non-null check with an
access_ok(VERIFY_WRITE, ...) check? We can even get rid of the check
because recvmsg will do it nonetheless when copying the sockaddrs to
user space. So kern_msg->msg_name can always be set to kern_address.

Otherwise I would just set msg_namelen = 0, too, and just not handle
passed in NULL pointers to sockaddrs.

Thanks,
Hannes

Re: net: socket: NULL ptr deref in sendmsg

[Andrey Ryabinin]

2014-07-26 2:15 GMT+04:00 Hannes Frederic Sowa <hannes@stressinduktion.org>:
>
> Thanks for looking at this! I certainly have overlooked this case.
>
> I wonder, if we allow sendto with valid NULL pointer and positive
> msg_namelen to work, why don't we do the same for recvmsg, as in
> replacing the VERIFY_WRITE case non-null check with an
> access_ok(VERIFY_WRITE, ...) check? We can even get rid of the check
> because recvmsg will do it nonetheless when copying the sockaddrs to
> user space. So kern_msg->msg_name can always be set to kern_address.
>

recvmsg's man page are pretty clear about that:

     "If the application does not need to
       know the source address, msg_name can be specified as NULL"

I think we shouldn't change current recvmsg behavior, this could break
some crappy
userpace that doesn't set msg_namelen to zero.


> Otherwise I would just set msg_namelen = 0, too, and just not handle
> passed in NULL pointers to sockaddrs.
>

I like that, how about such chage:

diff --git a/net/compat.c b/net/compat.c
index 9a76eaf..bc8aeef 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -85,7 +85,7 @@ int verify_compat_iovec(struct msghdr *kern_msg,
struct iovec *kern_iov,
 {
        int tot_len;

-       if (kern_msg->msg_namelen) {
+       if (kern_msg->msg_name && kern_msg->msg_namelen) {
                if (mode == VERIFY_READ) {
                        int err = move_addr_to_kernel(kern_msg->msg_name,
                                                      kern_msg->msg_namelen,
@@ -93,10 +93,11 @@ int verify_compat_iovec(struct msghdr *kern_msg,
struct iovec *kern_iov,
                        if (err < 0)
                                return err;
                }
-               if (kern_msg->msg_name)
-                       kern_msg->msg_name = kern_address;
-       } else
+               kern_msg->msg_name = kern_address;
+       } else {
                kern_msg->msg_name = NULL;
+               kern_msg->msg_namelen = 0;
+       }

        tot_len = iov_from_user_compat_to_kern(kern_iov,
                                          (struct compat_iovec __user
*)kern_msg->msg_iov,
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 827dd6b..e1ec45a 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -39,7 +39,7 @@ int verify_iovec(struct msghdr *m, struct iovec
*iov, struct sockaddr_storage *a
 {
        int size, ct, err;

-       if (m->msg_namelen) {
+       if (m->msg_name && m->msg_namelen) {
                if (mode == VERIFY_READ) {
                        void __user *namep;
                        namep = (void __user __force *) m->msg_name;
@@ -48,10 +48,10 @@ int verify_iovec(struct msghdr *m, struct iovec
*iov, struct sockaddr_storage *a
                        if (err < 0)
                                return err;
                }
-               if (m->msg_name)
-                       m->msg_name = address;
+               m->msg_name = address;
        } else {
                m->msg_name = NULL;
+               m->msg_namelen = 0;
        }

        size = m->msg_iovlen * sizeof(struct iovec);


-- 
Best regards,
Andrey Ryabinin

Re: net: socket: NULL ptr deref in sendmsg

[Hannes Frederic Sowa]

On Sa, 2014-07-26 at 19:48 +0400, Andrey Ryabinin wrote:
> 2014-07-26 2:15 GMT+04:00 Hannes Frederic Sowa <hannes@stressinduktion.org>:
> > Otherwise I would just set msg_namelen = 0, too, and just not handle
> > passed in NULL pointers to sockaddrs.
> >
> 
> I like that, how about such chage:
> 
> diff --git a/net/compat.c b/net/compat.c
> index 9a76eaf..bc8aeef 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -85,7 +85,7 @@ int verify_compat_iovec(struct msghdr *kern_msg,
> struct iovec *kern_iov,
>  {
>         int tot_len;
> 
> -       if (kern_msg->msg_namelen) {
> +       if (kern_msg->msg_name && kern_msg->msg_namelen) {
>                 if (mode == VERIFY_READ) {
>                         int err = move_addr_to_kernel(kern_msg->msg_name,
>                                                       kern_msg->msg_namelen,
> @@ -93,10 +93,11 @@ int verify_compat_iovec(struct msghdr *kern_msg,
> struct iovec *kern_iov,
>                         if (err < 0)
>                                 return err;
>                 }
> -               if (kern_msg->msg_name)
> -                       kern_msg->msg_name = kern_address;
> -       } else
> +               kern_msg->msg_name = kern_address;
> +       } else {
>                 kern_msg->msg_name = NULL;
> +               kern_msg->msg_namelen = 0;
> +       }
> 
>         tot_len = iov_from_user_compat_to_kern(kern_iov,
>                                           (struct compat_iovec __user
> *)kern_msg->msg_iov,
> diff --git a/net/core/iovec.c b/net/core/iovec.c
> index 827dd6b..e1ec45a 100644
> --- a/net/core/iovec.c
> +++ b/net/core/iovec.c
> @@ -39,7 +39,7 @@ int verify_iovec(struct msghdr *m, struct iovec
> *iov, struct sockaddr_storage *a
>  {
>         int size, ct, err;
> 
> -       if (m->msg_namelen) {
> +       if (m->msg_name && m->msg_namelen) {
>                 if (mode == VERIFY_READ) {
>                         void __user *namep;
>                         namep = (void __user __force *) m->msg_name;
> @@ -48,10 +48,10 @@ int verify_iovec(struct msghdr *m, struct iovec
> *iov, struct sockaddr_storage *a
>                         if (err < 0)
>                                 return err;
>                 }
> -               if (m->msg_name)
> -                       m->msg_name = address;
> +               m->msg_name = address;
>         } else {
>                 m->msg_name = NULL;
> +               m->msg_namelen = 0;
>         }
> 
>         size = m->msg_iovlen * sizeof(struct iovec);
> 
> 

LGTM! Can you send a patch?

Thanks,
Hannes

[PATCH] net: sendmsg: fix NULL pointer dereference

[Andrey Ryabinin]

Sasha's report:
	> While fuzzing with trinity inside a KVM tools guest running the latest -next
	> kernel with the KASAN patchset, I've stumbled on the following spew:
	>
	> [ 4448.949424] ==================================================================
	> [ 4448.951737] AddressSanitizer: user-memory-access on address 0
	> [ 4448.952988] Read of size 2 by thread T19638:
	> [ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
	> [ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
	> [ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
	> [ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
	> [ 4448.961266] Call Trace:
	> [ 4448.963158] dump_stack (lib/dump_stack.c:52)
	> [ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
	> [ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
	> [ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
	> [ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
	> [ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
	> [ 4448.970103] sock_sendmsg (net/socket.c:654)
	> [ 4448.971584] ? might_fault (mm/memory.c:3741)
	> [ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
	> [ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
	> [ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
	> [ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
	> [ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
	> [ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
	> [ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
	> [ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
	> [ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
	> [ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
	> [ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
	> [ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
	> [ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
	> [ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
	> [ 4448.988929] ==================================================================

This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.

After this report there was no usual "Unable to handle kernel NULL pointer dereference"
and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.

This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
(net: rework recvmsg handler msg_name and msg_namelen logic).
Commit message states that:
	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
	 affect sendto as it would bail out earlier while trying to copy-in the
	 address."
But in fact this affects sendto when address 0 is mapped and contains
socket address structure in it. In such case copy-in address will succeed,
verify_iovec() function will successfully exit with msg->msg_namelen > 0
and msg->msg_name == NULL.

This patch fixes it by setting msg_namelen to 0 if msg_name == NULL.

Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: <stable@vger.kernel.org>
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 net/compat.c     | 9 +++++----
 net/core/iovec.c | 6 +++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/compat.c b/net/compat.c
index 9a76eaf..bc8aeef 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -85,7 +85,7 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
 {
 	int tot_len;
 
-	if (kern_msg->msg_namelen) {
+	if (kern_msg->msg_name && kern_msg->msg_namelen) {
 		if (mode == VERIFY_READ) {
 			int err = move_addr_to_kernel(kern_msg->msg_name,
 						      kern_msg->msg_namelen,
@@ -93,10 +93,11 @@ int verify_compat_iovec(struct msghdr *kern_msg, struct iovec *kern_iov,
 			if (err < 0)
 				return err;
 		}
-		if (kern_msg->msg_name)
-			kern_msg->msg_name = kern_address;
-	} else
+		kern_msg->msg_name = kern_address;
+	} else {
 		kern_msg->msg_name = NULL;
+		kern_msg->msg_namelen = 0;
+	}
 
 	tot_len = iov_from_user_compat_to_kern(kern_iov,
 					  (struct compat_iovec __user *)kern_msg->msg_iov,
diff --git a/net/core/iovec.c b/net/core/iovec.c
index 827dd6b..e1ec45a 100644
--- a/net/core/iovec.c
+++ b/net/core/iovec.c
@@ -39,7 +39,7 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *a
 {
 	int size, ct, err;
 
-	if (m->msg_namelen) {
+	if (m->msg_name && m->msg_namelen) {
 		if (mode == VERIFY_READ) {
 			void __user *namep;
 			namep = (void __user __force *) m->msg_name;
@@ -48,10 +48,10 @@ int verify_iovec(struct msghdr *m, struct iovec *iov, struct sockaddr_storage *a
 			if (err < 0)
 				return err;
 		}
-		if (m->msg_name)
-			m->msg_name = address;
+		m->msg_name = address;
 	} else {
 		m->msg_name = NULL;
+		m->msg_namelen = 0;
 	}
 
 	size = m->msg_iovlen * sizeof(struct iovec);
-- 
1.8.5.5

Re: [PATCH] net: sendmsg: fix NULL pointer dereference

[Hannes Frederic Sowa]

On Sa, 2014-07-26 at 21:26 +0400, Andrey Ryabinin wrote:
> Sasha's report:
> 	> While fuzzing with trinity inside a KVM tools guest running the latest -next
> 	> kernel with the KASAN patchset, I've stumbled on the following spew:
> 	>
> 	> [ 4448.949424] ==================================================================
> 	> [ 4448.951737] AddressSanitizer: user-memory-access on address 0
> 	> [ 4448.952988] Read of size 2 by thread T19638:
> 	> [ 4448.954510] CPU: 28 PID: 19638 Comm: trinity-c76 Not tainted 3.16.0-rc4-next-20140711-sasha-00046-g07d3099-dirty #813
> 	> [ 4448.956823]  ffff88046d86ca40 0000000000000000 ffff880082f37e78 ffff880082f37a40
> 	> [ 4448.958233]  ffffffffb6e47068 ffff880082f37a68 ffff880082f37a58 ffffffffb242708d
> 	> [ 4448.959552]  0000000000000000 ffff880082f37a88 ffffffffb24255b1 0000000000000000
> 	> [ 4448.961266] Call Trace:
> 	> [ 4448.963158] dump_stack (lib/dump_stack.c:52)
> 	> [ 4448.964244] kasan_report_user_access (mm/kasan/report.c:184)
> 	> [ 4448.965507] __asan_load2 (mm/kasan/kasan.c:352)
> 	> [ 4448.966482] ? netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.967541] netlink_sendmsg (net/netlink/af_netlink.c:2339)
> 	> [ 4448.968537] ? get_parent_ip (kernel/sched/core.c:2555)
> 	> [ 4448.970103] sock_sendmsg (net/socket.c:654)
> 	> [ 4448.971584] ? might_fault (mm/memory.c:3741)
> 	> [ 4448.972526] ? might_fault (./arch/x86/include/asm/current.h:14 mm/memory.c:3740)
> 	> [ 4448.973596] ? verify_iovec (net/core/iovec.c:64)
> 	> [ 4448.974522] ___sys_sendmsg (net/socket.c:2096)
> 	> [ 4448.975797] ? put_lock_stats.isra.13 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
> 	> [ 4448.977030] ? lock_release_holdtime (kernel/locking/lockdep.c:273)
> 	> [ 4448.978197] ? lock_release_non_nested (kernel/locking/lockdep.c:3434 (discriminator 1))
> 	> [ 4448.979346] ? check_chain_key (kernel/locking/lockdep.c:2188)
> 	> [ 4448.980535] __sys_sendmmsg (net/socket.c:2181)
> 	> [ 4448.981592] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.982773] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
> 	> [ 4448.984458] ? syscall_trace_enter (arch/x86/kernel/ptrace.c:1500 (discriminator 2))
> 	> [ 4448.985621] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2600)
> 	> [ 4448.986754] SyS_sendmmsg (net/socket.c:2201)
> 	> [ 4448.987708] tracesys (arch/x86/kernel/entry_64.S:542)
> 	> [ 4448.988929] ==================================================================
> 
> This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.
> 
> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
> 
> This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
> (net: rework recvmsg handler msg_name and msg_namelen logic).
> Commit message states that:
> 	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
> 	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
> 	 affect sendto as it would bail out earlier while trying to copy-in the
> 	 address."
> But in fact this affects sendto when address 0 is mapped and contains
> socket address structure in it. In such case copy-in address will succeed,
> verify_iovec() function will successfully exit with msg->msg_namelen > 0
> and msg->msg_name == NULL.
> 
> This patch fixes it by setting msg_namelen to 0 if msg_name == NULL.
> 
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thank you!

Re: [PATCH] net: sendmsg: fix NULL pointer dereference

[David Miller]

From: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Date: Sat, 26 Jul 2014 21:26:58 +0400

> Sasha's report:
 ...
> This reports means that we've come to netlink_sendmsg() with msg->msg_name == NULL and msg->msg_namelen > 0.
> 
> After this report there was no usual "Unable to handle kernel NULL pointer dereference"
> and this gave me a clue that address 0 is mapped and contains valid socket address structure in it.
> 
> This bug was introduced in f3d3342602f8bcbf37d7c46641cb9bca7618eb1c
> (net: rework recvmsg handler msg_name and msg_namelen logic).
> Commit message states that:
> 	"Set msg->msg_name = NULL if user specified a NULL in msg_name but had a
> 	 non-null msg_namelen in verify_iovec/verify_compat_iovec. This doesn't
> 	 affect sendto as it would bail out earlier while trying to copy-in the
> 	 address."
> But in fact this affects sendto when address 0 is mapped and contains
> socket address structure in it. In such case copy-in address will succeed,
> verify_iovec() function will successfully exit with msg->msg_namelen > 0
> and msg->msg_name == NULL.
> 
> This patch fixes it by setting msg_namelen to 0 if msg_name == NULL.
> 
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>

Applied and queued up for -stable, thanks!

Re: net: socket: NULL ptr deref in sendmsg

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Sat, 26 Jul 2014 17:54:40 +0200

> On Sa, 2014-07-26 at 19:48 +0400, Andrey Ryabinin wrote:
>> 2014-07-26 2:15 GMT+04:00 Hannes Frederic Sowa <hannes@stressinduktion.org>:
>> > Otherwise I would just set msg_namelen = 0, too, and just not handle
>> > passed in NULL pointers to sockaddrs.
>> >
>> 
>> I like that, how about such chage:
 ...
> LGTM! Can you send a patch?

I definitely agree that we should treat NULL as no address specified.