Question regarding sendmmsg().

Question regarding sendmmsg().

[Tetsuo Handa]

TOMOYO was about to add support for permission checks for
PF_INET/PF_INET6/PF_UNIX socket's bind()/listen()/connect()/send() operations
( http://www.spinics.net/linux/fedora/linux-security-module/msg11496.html ).

According to http://ozlabs.org/~anton/junkcode/sendmmsg_test.c , the sendmmsg()
introduced by commit 228e548e "net: Add sendmmsg socket system call" is capable
of sending to multiple different destinations with single sendmmsg(), isn't it?

If yes, my plan (restricting sendmsg() based on destination's address) became
impossible since security_socket_sendmsg() (which receives the destination's
address) is called for only once even if there are multiple different
destinations.

Re: Question regarding sendmmsg().

[Tetsuo Handa]

Tetsuo Handa wrote:
> According to http://ozlabs.org/~anton/junkcode/sendmmsg_test.c , the sendmmsg()
> introduced by commit 228e548e "net: Add sendmmsg socket system call" is capable
> of sending to multiple different destinations with single sendmmsg(), isn't it?
> 
> If yes, my plan (restricting sendmsg() based on destination's address) became
> impossible since security_socket_sendmsg() (which receives the destination's
> address) is called for only once even if there are multiple different
> destinations.

It seems to me that sendmmsg() caused a regression for SMACK.
SMACK implements security_socket_sendmsg() which uses destination address.

static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
                                int size)
{
        struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;

        /*
         * Perfectly reasonable for this to be NULL
         */
        if (sip == NULL || sip->sin_family != AF_INET)
                return 0;

        return smack_netlabel_send(sock->sk, sip);
}

I think SMACK wants to know all destination addresses as with TOMOYO.

[PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

I think the regression for SMACK can be fixed with below patch.

Should I pass nosec flags down to "struct security_operations"->sendmsg()
so that SELinux checks sock_has_perm() for only once when multiple different
destination's addresses are passed to sendmmsg()?

static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
				  int size, int nosec)
{
	return nosec ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
}
----------------------------------------
[PATCH] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than only the first destination address.

Fix this problem by removing nosec flags.
Also, remove sock_sendmsg_nosec() because it is no longer used.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |   28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -580,20 +580,6 @@ int sock_sendmsg(struct socket *sock, st
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
-{
-	struct kiocb iocb;
-	struct sock_iocb siocb;
-	int ret;
-
-	init_sync_kiocb(&iocb, NULL);
-	iocb.private = &siocb;
-	ret = __sock_sendmsg_nosec(&iocb, sock, msg, size);
-	if (-EIOCBQUEUED == ret)
-		ret = wait_on_sync_kiocb(&iocb);
-	return ret;
-}
-
 int kernel_sendmsg(struct socket *sock, struct msghdr *msg,
 		   struct kvec *vec, size_t num, size_t size)
 {
@@ -1872,7 +1858,7 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1953,8 +1939,7 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
-							  total_len);
+	err = sock_sendmsg(sock, msg_sys, total_len);
 
 out_freectl:
 	if (ctl_buf != ctl)
@@ -1979,7 +1964,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -2014,18 +1999,19 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 	while (datagrams < vlen) {
 		/*
-		 * No need to ask LSM for more than the first datagram.
+		 * Need to ask LSM every time in case LSM might check
+		 * destination's address.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 23 Jul 2011 00:12:53 +0900

> I think the regression for SMACK can be fixed with below patch.
> 
> Should I pass nosec flags down to "struct security_operations"->sendmsg()
> so that SELinux checks sock_has_perm() for only once when multiple different
> destination's addresses are passed to sendmmsg()?
> 
> static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
> 				  int size, int nosec)
> {
> 	return nosec ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
> }

Ugh, this takes away a non-trivial part of the performance gain of
sendmmsg().

I would instead rather that you check ahead of time whether this
actually is a send to different addresses.  If they are all the
same, keep the nosec code path.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

David Miller wrote:
> Ugh, this takes away a non-trivial part of the performance gain of
> sendmmsg().
> 
> I would instead rather that you check ahead of time whether this
> actually is a send to different addresses.  If they are all the
> same, keep the nosec code path.
> 
OK. Something like this? Not tested at all.
----------------------------------------
[PATCH 1/3] net: Add "static" to sock_sendmsg_nosec().

sock_sendmsg_nosec() is called from only __sys_sendmsg().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -580,7 +580,8 @@ int sock_sendmsg(struct socket *sock, st
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
+static int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg,
+			      size_t size)
 {
 	struct kiocb iocb;
 	struct sock_iocb siocb;

----------------------------------------
[PATCH 2/3] net: Introduce helper for copying msghdr struct.

This patch introduces copy_msghdr_to_kenrel().
This helper is used for avoiding race condition when comparing destination's
addresses passed to sendmmsg().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |   50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -1872,11 +1872,29 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_NAMELEN(msg)	COMPAT_MSG(msg, msg_namelen)
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
-static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+/**
+ * move_msghdr_to_kernel - copy a "struct msghdr" into kernel space
+ *
+ * @msg:     "struct msghdr" in user space
+ * @msg_sys: "struct msghdr" in kernel space
+ * @flags:   sendmsg() flags.
+ */
+static int copy_msghdr_to_kernel(struct msghdr __user *msg,
+				 struct msghdr *msg_sys, unsigned flags)
+{
+	if (MSG_CMSG_COMPAT & flags) {
+		struct compat_msghdr __user *msg_compat =
+			(struct compat_msghdr __user *) msg;
+		if (get_compat_msghdr(msg_sys, msg_compat))
+			return -EFAULT;
+	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
+		return -EFAULT;
+	return 0;
+}
+
+static int __sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
+			 unsigned flags, int nosec)
 {
-	struct compat_msghdr __user *msg_compat =
-	    (struct compat_msghdr __user *)msg;
 	struct sockaddr_storage address;
 	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
 	unsigned char ctl[sizeof(struct cmsghdr) + 20]
@@ -1885,13 +1903,6 @@ static int __sys_sendmsg(struct socket *
 	unsigned char *ctl_buf = ctl;
 	int err, ctl_len, iov_size, total_len;
 
-	err = -EFAULT;
-	if (MSG_CMSG_COMPAT & flags) {
-		if (get_compat_msghdr(msg_sys, msg_compat))
-			return -EFAULT;
-	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
-		return -EFAULT;
-
 	/* do not move before msg_sys is valid */
 	err = -EMSGSIZE;
 	if (msg_sys->msg_iovlen > UIO_MAXIOV)
@@ -1980,7 +1991,9 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = copy_msghdr_to_kernel(msg, &msg_sys, flags);
+	if (!err)
+		err = __sys_sendmsg(sock, &msg_sys, flags, 0);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -2018,15 +2031,20 @@ int __sys_sendmmsg(int fd, struct mmsghd
 		 * No need to ask LSM for more than the first datagram.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
-			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+			err = copy_msghdr_to_kernel((struct msghdr __user *)
+						    compat_entry, &msg_sys,
+						    flags);
+			if (err)
+				break;
+			err = __sys_sendmsg(sock, &msg_sys, flags, datagrams);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
-			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+			err = copy_msghdr_to_kernel((struct msghdr __user *)
+						    entry, &msg_sys, flags);
+			err = __sys_sendmsg(sock, &msg_sys, flags, datagrams);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);

----------------------------------------
[PATCH 3/3] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than the first destination address.

Fix this problem by calling security_socket_sendmsg() for each destination
address.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |   52 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)

--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -2011,7 +2011,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct socket *sock;
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
-	struct msghdr msg_sys;
+	struct msghdr *msg_tmp = NULL;
 
 	datagrams = 0;
 
@@ -2026,27 +2026,44 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	entry = mmsg;
 	compat_entry = (struct compat_mmsghdr __user *)mmsg;
 
+	/*
+	 * Remember all destination addresses to kernel memory because LSM
+	 * might check destination addresses.
+	 */
+	err = -ENOMEM;
+	msg_tmp = kmalloc(vlen * sizeof(*msg_tmp), GFP_KERNEL);
+	if (!msg_tmp || msg_tmp == ZERO_SIZE_PTR)
+		goto out_put;
 	while (datagrams < vlen) {
-		/*
-		 * No need to ask LSM for more than the first datagram.
-		 */
+		struct msghdr *msg_sys = &msg_tmp[datagrams];
+		err = copy_msghdr_to_kernel((MSG_CMSG_COMPAT & flags) ?
+					    (struct msghdr __user *)
+					    compat_entry :
+					    (struct msghdr __user *) entry,
+					    &msg_tmp[datagrams], flags);
+		if (err)
+			goto out_put;
+		/* Ask LSM only once for each destination address. */
+		if (msg_sys->msg_namelen && msg_sys->msg_name)
+			for (err = 0; err < datagrams; err++) {
+				if (msg_tmp[err].msg_namelen ==
+				    msg_sys->msg_namelen &&
+				    msg_tmp[err].msg_name &&
+				    !memcmp(msg_tmp[err].msg_name,
+					    msg_sys->msg_name,
+					    msg_sys->msg_namelen))
+					break;
+			}
+		/* Ask LSM every time if destination address is not passed. */
+		else
+			err = datagrams;
+		err = __sys_sendmsg(sock, msg_sys, flags, err < datagrams);
+		if (err < 0)
+			break;
 		if (MSG_CMSG_COMPAT & flags) {
-			err = copy_msghdr_to_kernel((struct msghdr __user *)
-						    compat_entry, &msg_sys,
-						    flags);
-			if (err)
-				break;
-			err = __sys_sendmsg(sock, &msg_sys, flags, datagrams);
-			if (err < 0)
-				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
-			err = copy_msghdr_to_kernel((struct msghdr __user *)
-						    entry, &msg_sys, flags);
-			err = __sys_sendmsg(sock, &msg_sys, flags, datagrams);
-			if (err < 0)
-				break;
 			err = put_user(err, &entry->msg_len);
 			++entry;
 		}
@@ -2058,6 +2075,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 out_put:
 	fput_light(sock->file, fput_needed);
+	kfree(msg_tmp);
 
 	if (err == 0)
 		return datagrams;

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Tetsuo Handa wrote:
> David Miller wrote:
> > Ugh, this takes away a non-trivial part of the performance gain of
> > sendmmsg().
> > 
> > I would instead rather that you check ahead of time whether this
> > actually is a send to different addresses.  If they are all the
> > same, keep the nosec code path.
> > 
> OK. Something like this? Not tested at all.

No. We can't compare destination address before entering __sys_sendmsg(), for
it is copied to kernel memory by verify_iovec()/verify_compat_iovec().

To be able to compare, we need to pass "struct sockaddr_storage address;" to
__sys_sendmsg(). Also, the content the kernel has to hold is not array of
"struct msghdr" but array of "struct sockaddr_storage".
It needs larger modification than expected.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > David Miller wrote:
> > > Ugh, this takes away a non-trivial part of the performance gain of
> > > sendmmsg().
> > > 
> > > I would instead rather that you check ahead of time whether this
> > > actually is a send to different addresses.  If they are all the
> > > same, keep the nosec code path.
> > > 
> > OK. Something like this? Not tested at all.
> 
> No. We can't compare destination address before entering __sys_sendmsg(), for
> it is copied to kernel memory by verify_iovec()/verify_compat_iovec().
> 
OK. Something like this? Not tested at all.
----------------------------------------
[PATCH] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than the first destination address.

Fix this problem by maintaining a list of already-checked destination address.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 79 insertions(+), 5 deletions(-)

--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -1871,8 +1871,19 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_NAMELEN(msg)	COMPAT_MSG(msg, msg_namelen)
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
+/*
+ * Structure for remembering destination's address used by send_mmsg().
+ * This is for calling security_socket_sendms() only once for each destination.
+ */
+struct sendmmsg_dest_info {
+	struct list_head list;
+	unsigned int address_len;
+	struct sockaddr_storage address;
+};
+
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags,
+			 struct list_head *list)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1883,6 +1894,7 @@ static int __sys_sendmsg(struct socket *
 	/* 20 is size of ipv6_pktinfo */
 	unsigned char *ctl_buf = ctl;
 	int err, ctl_len, iov_size, total_len;
+	bool nosec = false;
 
 	err = -EFAULT;
 	if (MSG_CMSG_COMPAT & flags) {
@@ -1953,6 +1965,58 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
+	if (list) {
+		/*
+		 * We need to pass destination address to
+		 * security_socket_sendmsg() since some LSM modules want it.
+		 * But passing already-checked destination address twice is
+		 * waste of time.
+		 *
+		 * Therefore, check for already-checked destination address in
+		 * order to see whether we can omit security_socket_sendmsg()
+		 * call or not.
+		 *
+		 * This optimization assumes that LSM modules use only
+		 * destination address (i.e. "struct msghdr"->msg_name and
+		 * "struct msghdr"->msg_namelen). We can't use this assumption
+		 * if LSM modules want to use other factors (e.g. total_len
+		 * argument below).
+		 */
+		struct sendmmsg_dest_info *ptr;
+		list_for_each_entry(ptr, list, list) {
+			/*
+			 * verify_iovec()/verify_compat_iovec() above assigned
+			 * appropriate values to msg_sys->msg_namelen and
+			 * msg_sys->msg_name.
+			 */
+			if (ptr->address_len != msg_sys->msg_namelen ||
+			    memcmp(&ptr->address, msg_sys->msg_name,
+				   ptr->address_len))
+				continue;
+			nosec = true;
+			break;
+		}
+		if (!nosec) {
+			/*
+			 * Remember the destination address passed to
+			 * sendmmsg() so that we can avoid calling
+			 * security_sendmsg_permission() again for
+			 * already-checked destination address.
+			 *
+			 * Out of memory error is not fatal here because
+			 * calling security_sendmsg_permission() again for
+			 * already-checked destination address should be
+			 * harmless.
+			 */
+			ptr = kmalloc(sizeof(*ptr), GFP_KERNEL);
+			if (ptr) {
+				ptr->address_len = msg_sys->msg_namelen;
+				memcpy(&ptr->address, msg_sys->msg_name,
+				       ptr->address_len);
+				list_add(&ptr->list, list);
+			}
+		}
+	}
 	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
 							  total_len);
 
@@ -1979,7 +2043,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags, NULL);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -1998,6 +2062,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
+	LIST_HEAD(list); /* List for finding duplicated destination address. */
 
 	datagrams = 0;
 
@@ -2014,18 +2079,19 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 	while (datagrams < vlen) {
 		/*
-		 * No need to ask LSM for more than the first datagram.
+		 * No need to ask LSM for more than the first datagram for
+		 * each destination.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, &list);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, &list);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);
@@ -2038,6 +2104,14 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	}
 
 out_put:
+	{ /* Clean up destination addresses. */
+		struct sendmmsg_dest_info *ptr;
+		struct sendmmsg_dest_info *tmp;
+		list_for_each_entry_safe(ptr, tmp, &list, list) {
+			list_del(&ptr->list);
+			kfree(ptr);
+		}
+	}
 	fput_light(sock->file, fput_needed);
 
 	if (err == 0)

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Michael Tokarev]

22.07.2011 19:22, David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 23 Jul 2011 00:12:53 +0900
> 
>> I think the regression for SMACK can be fixed with below patch.
>>
>> Should I pass nosec flags down to "struct security_operations"->sendmsg()
>> so that SELinux checks sock_has_perm() for only once when multiple different
>> destination's addresses are passed to sendmmsg()?
>>
>> static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>> 				  int size, int nosec)
>> {
>> 	return nosec ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
>> }
> 
> Ugh, this takes away a non-trivial part of the performance gain of
> sendmmsg().
> 
> I would instead rather that you check ahead of time whether this
> actually is a send to different addresses.  If they are all the
> same, keep the nosec code path.

Why to optimize for this case when destination addresses are the
same?  How common this usage case is, or even where it _can_
happen alot (I noticed samba.org address in the Cc list).

When I saw recvmmsg()/sendmmsg() here, my first thought was an
authoritative DNS server which can read several requests at a
time and answer them all at once too - this way it all will go
to different addresses.

I understand the initial change takes away good portion of
performance improvement, but I think the optimisation should
be performed in a different place than for a not-so-common
cenario.

Thanks,

/mjt

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Michael Tokarev wrote:
> (I noticed samba.org address in the Cc list).

That's because Anton Blanchard is author of sendmmsg() system call.

> When I saw recvmmsg()/sendmmsg() here, my first thought was an
> authoritative DNS server which can read several requests at a
> time and answer them all at once too - this way it all will go
> to different addresses.

I don't know what application wants sendmmsg(). Since users can send up to
UIO_MAXIOV (= 1024) "struct iovec" blocks using sendmsg(), they will use
sendmsg() rather than sendmmsg() if the destination address are the same.

Therefore, I guess users will use sendmmsg() for sending to multiple different
destination addresses. If so, optimization based on destination address will do
more harm than benefit; simply passing nosec flag down to LSM modules (so that
SELinux will skip sock_has_perm() call and SMACK will not skip
smack_netlabel_send() call) will be sufficient for 3.0.x stable release.

Anton, how do you want to use sendmmsg()?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi,

> > (I noticed samba.org address in the Cc list).
> 
> That's because Anton Blanchard is author of sendmmsg() system call.

Ignore the From address - I wasn't adding sendmmsg with samba in mind.

> > When I saw recvmmsg()/sendmmsg() here, my first thought was an
> > authoritative DNS server which can read several requests at a
> > time and answer them all at once too - this way it all will go
> > to different addresses.
> 
> I don't know what application wants sendmmsg(). Since users can send
> up to UIO_MAXIOV (= 1024) "struct iovec" blocks using sendmsg(), they
> will use sendmsg() rather than sendmmsg() if the destination address
> are the same.

But if an application needs to maintain packet boundaries, then sendmsg
isn't going to help is it?
 
> Therefore, I guess users will use sendmmsg() for sending to multiple
> different destination addresses. If so, optimization based on
> destination address will do more harm than benefit; simply passing
> nosec flag down to LSM modules (so that SELinux will skip
> sock_has_perm() call and SMACK will not skip smack_netlabel_send()
> call) will be sufficient for 3.0.x stable release.
> 
> Anton, how do you want to use sendmmsg()?

I was using it for packet generation, using raw sockets.

Anton

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Anton Blanchard wrote:
> > > When I saw recvmmsg()/sendmmsg() here, my first thought was an
> > > authoritative DNS server which can read several requests at a
> > > time and answer them all at once too - this way it all will go
> > > to different addresses.
> > 
> > I don't know what application wants sendmmsg(). Since users can send
> > up to UIO_MAXIOV (= 1024) "struct iovec" blocks using sendmsg(), they
> > will use sendmsg() rather than sendmmsg() if the destination address
> > are the same.
> 
> But if an application needs to maintain packet boundaries, then sendmsg
> isn't going to help is it?

Well, such application might want to use RDM or SeqPacket... but your point is
to maintain packet boundaries. You are assuming that sendmmsg() will be used
for sending as much data as possible while preserving packet boundaries.

OK. Then, the question is how to reduce performance loss by redundant
security_socket_sendmsg() calls. If sendmmsg() likely contains single (or few)
destination(s), trying to optimize security_socket_sendmsg() calls by comparing
destination address (as proposed at
http://www.spinics.net/linux/fedora/linux-security-module/msg11510.html
) would help. Otherwise, no optimization (as proposed at
http://www.spinics.net/linux/fedora/linux-security-module/msg11504.html
) would be better. Which approach do you like?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Casey Schaufler]

On 7/25/2011 6:15 AM, Tetsuo Handa wrote:
> Anton Blanchard wrote:
>>>> When I saw recvmmsg()/sendmmsg() here, my first thought was an
>>>> authoritative DNS server which can read several requests at a
>>>> time and answer them all at once too - this way it all will go
>>>> to different addresses.
>>> I don't know what application wants sendmmsg(). Since users can send
>>> up to UIO_MAXIOV (= 1024) "struct iovec" blocks using sendmsg(), they
>>> will use sendmsg() rather than sendmmsg() if the destination address
>>> are the same.
>> But if an application needs to maintain packet boundaries, then sendmsg
>> isn't going to help is it?
> Well, such application might want to use RDM or SeqPacket... but your point is
> to maintain packet boundaries. You are assuming that sendmmsg() will be used
> for sending as much data as possible while preserving packet boundaries.
>
> OK. Then, the question is how to reduce performance loss by redundant
> security_socket_sendmsg() calls.

Not to be splitting hairs, but if the packets are headed to
different destinations the calls to security_socket_sendmsg()
are not redundant, they are necessary and appropriate. What
you have with sendmmsg() is an optimization that sacrifices
correctness for performance.

> If sendmmsg() likely contains single (or few)
> destination(s), trying to optimize security_socket_sendmsg() calls by comparing
> destination address (as proposed at
> http://www.spinics.net/linux/fedora/linux-security-module/msg11510.html
> ) would help. Otherwise, no optimization (as proposed at
> http://www.spinics.net/linux/fedora/linux-security-module/msg11504.html
> ) would be better. Which approach do you like?

I fear that you are going to find that the work you have
to do to reduce the number of calls is going to outweigh
the benefits of your optimization, as has been pointed out
earlier. My recommendation is that the sendmmsg() interface
is ill conceived and that you should look for alternative
ways to improve the performance of the use case.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Casey Schaufler wrote:
> > OK. Then, the question is how to reduce performance loss by redundant
> > security_socket_sendmsg() calls.
> 
> Not to be splitting hairs, but if the packets are headed to
> different destinations the calls to security_socket_sendmsg()
> are not redundant, they are necessary and appropriate. What
> you have with sendmmsg() is an optimization that sacrifices
> correctness for performance.

Excuse me, but this thread is not trying to remove necessary and appropriate
security_socket_sendmsg() calls. Linux 3.0 was released without necessary and
appropriate security_socket_sendmsg() calls, and I'm trying to correct it (via
msg11504.html or msg11510.html) for Linux 3.0.x stable release.

> I fear that you are going to find that the work you have
> to do to reduce the number of calls is going to outweigh
> the benefits of your optimization, as has been pointed out
> earlier.

I fear it too. Unless many dozens (maybe some hundreds) of packets are sent by
sendmmsg(), msg11504.html might show better performance than msg11510.html .
But I don't have a machine to benchmark.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Casey Schaufler]

On 7/25/2011 9:43 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>>> OK. Then, the question is how to reduce performance loss by redundant
>>> security_socket_sendmsg() calls.
>> Not to be splitting hairs, but if the packets are headed to
>> different destinations the calls to security_socket_sendmsg()
>> are not redundant, they are necessary and appropriate. What
>> you have with sendmmsg() is an optimization that sacrifices
>> correctness for performance.
> Excuse me, but this thread is not trying to remove necessary and appropriate
> security_socket_sendmsg() calls. Linux 3.0 was released without necessary and
> appropriate security_socket_sendmsg() calls, and I'm trying to correct it (via
> msg11504.html or msg11510.html) for Linux 3.0.x stable release.

I understand. Sorry if I did a poor job of jumping into
the thread.

>> I fear that you are going to find that the work you have
>> to do to reduce the number of calls is going to outweigh
>> the benefits of your optimization, as has been pointed out
>> earlier.
> I fear it too. Unless many dozens (maybe some hundreds) of packets are sent by
> sendmmsg(), msg11504.html might show better performance than msg11510.html .
> But I don't have a machine to benchmark.

Is there some chance that the original authors could step up
to help with the benchmarking effort on this repair? Having been
on the end where I introduced problems more than once, I have a
good understanding of the principle "you broke it, you bought it".

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi,

> I fear it too. Unless many dozens (maybe some hundreds) of packets
> are sent by sendmmsg(), msg11504.html might show better performance
> than msg11510.html . But I don't have a machine to benchmark.

Not sure what happened to your email but the gains are evident at
just 2 packets.

I can help with testing - the commit included a microbenchmark for
the purposes of analysing its performance.

Anton

--

    net: Add sendmmsg socket system call
    
    This patch adds a multiple message send syscall and is the send
    version of the existing recvmmsg syscall. This is heavily
    based on the patch by Arnaldo that added recvmmsg.
    
    I wrote a microbenchmark to test the performance gains of using
    this new syscall:
    
    http://ozlabs.org/~anton/junkcode/sendmmsg_test.c
    
    The test was run on a ppc64 box with a 10 Gbit network card. The
    benchmark can send both UDP and RAW ethernet packets.
    
    64B UDP
    
    batch   pkts/sec
    1       804570
    2       872800 (+ 8 %)
    4       916556 (+14 %)
    8       939712 (+17 %)
    16      952688 (+18 %)
    32      956448 (+19 %)
    64      964800 (+20 %)
    
    64B raw socket
    
    batch   pkts/sec
    1       1201449
    2       1350028 (+12 %)
    4       1461416 (+22 %)
    8       1513080 (+26 %)
    16      1541216 (+28 %)
    32      1553440 (+29 %)
    64      1557888 (+30 %)
    
    We see a 20% improvement in throughput on UDP send and 30%
    on raw socket send.
    
    [ Add sparc syscall entries. -DaveM ]

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Anton Blanchard wrote:
> Not sure what happened to your email but the gains are evident at
> just 2 packets.

I know your benchmark result, but that result is based on calling
security_socket_sendmsg() only once.

What we worry is that overhead by security_socket_sendmsg() kills the
performance gain for batched case.

> I can help with testing - the commit included a microbenchmark for
> the purposes of analysing its performance.

Yes please. The patch in msg11510.html would want some more discussion, but
the patch in msg11504.html is ready to benchmark on your environment.

Does SELinux want to receive nosec flag at selinux_socket_sendmsg() because
calling security_socket_sendmsg() more than once is meaningless for SELinux?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Eric Paris]

On Tue, Jul 26, 2011 at 7:21 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:

> Does SELinux want to receive nosec flag at selinux_socket_sendmsg() because
> calling security_socket_sendmsg() more than once is meaningless for SELinux?

SELinux would not mind having a flag such that we could expedite the
call on the 2nd or later.  Did we finally find the first place where
SELinux is going to be the faster LSM!  Never thought I'd see the day!

-Eric

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Here is an optimized version. Only compile tested.

Regarding SELinux, there should be little performance loss by this change.

Regarding SMACK, please test both functionality and performance improvement.
Unoptimized version will be measurable by applying
http://www.spinics.net/linux/fedora/linux-security-module/msg11504.html .

Regarding TOMOYO, I'll update tomoyo_socket_sendmsg() like SMACK does.

Regarding AppArmor, please update apparmor_socket_sendmsg() in Oneiric's patch
like SELinux does.

Regarding no-LSM case, there should be little performance loss by this change.
----------------------------------------
[PATCH] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than only the first destination address.

Fix this regression by
(1) passing "int datagrams" argument to security_socket_sendmsg() so that
    SELinux can omit sock_has_perm() checks on the 2nd or later.
(2) passing "struct list_head *list" argument to security_socket_sendmsg() so
    that SMACK can omit smack_netlabel_send() checks for duplicated destination
    address.
(3) letting __sys_sendmmsg() provide "struct list_head list" for
    security_socket_sendmsg() and clean it up before return.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 include/linux/security.h   |   16 ++++++++++---
 include/linux/socket.h     |    8 ++++++
 net/socket.c               |   42 +++++++++++++++++++++++++-----------
 security/capability.c      |    3 +-
 security/security.c        |   52 +++++++++++++++++++++++++++++++++++++++++++--
 security/selinux/hooks.c   |    5 ++--
 security/smack/smack_lsm.c |    5 +++-
 7 files changed, 108 insertions(+), 23 deletions(-)

--- linux-3.0.orig/include/linux/security.h
+++ linux-3.0/include/linux/security.h
@@ -93,6 +93,7 @@ struct xfrm_policy;
 struct xfrm_state;
 struct xfrm_user_sec_ctx;
 struct seq_file;
+struct list_head;
 
 extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
 extern int cap_netlink_recv(struct sk_buff *skb, int cap);
@@ -880,6 +881,10 @@ static inline void security_free_mnt_opt
  *	@sock contains the socket structure.
  *	@msg contains the message to be transmitted.
  *	@size contains the size of message.
+ *	@datagrams contains the index of messages in sendmmsg(). This is 0 if
+ *	not sendmmsg().
+ *	@list contains the list head which can be used for holding
+ *	already-checked destination address. This is NULL if not sendmmsg().
  *	Return 0 if permission is granted.
  * @socket_recvmsg:
  *	Check permission before receiving a message from a socket.
@@ -1584,8 +1589,8 @@ struct security_operations {
 			       struct sockaddr *address, int addrlen);
 	int (*socket_listen) (struct socket *sock, int backlog);
 	int (*socket_accept) (struct socket *sock, struct socket *newsock);
-	int (*socket_sendmsg) (struct socket *sock,
-			       struct msghdr *msg, int size);
+	int (*socket_sendmsg) (struct socket *sock, struct msghdr *msg,
+			       int size, int datagrams, struct list_head *list);
 	int (*socket_recvmsg) (struct socket *sock,
 			       struct msghdr *msg, int size, int flags);
 	int (*socket_getsockname) (struct socket *sock);
@@ -2551,7 +2556,9 @@ int security_socket_bind(struct socket *
 int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen);
 int security_socket_listen(struct socket *sock, int backlog);
 int security_socket_accept(struct socket *sock, struct socket *newsock);
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list);
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
 int security_socket_getsockname(struct socket *sock);
@@ -2636,7 +2643,8 @@ static inline int security_socket_accept
 }
 
 static inline int security_socket_sendmsg(struct socket *sock,
-					  struct msghdr *msg, int size)
+					  struct msghdr *msg, int size,
+					  int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0.orig/include/linux/socket.h
+++ linux-3.0/include/linux/socket.h
@@ -23,6 +23,7 @@ struct __kernel_sockaddr_storage {
 #include <linux/uio.h>			/* iovec support		*/
 #include <linux/types.h>		/* pid_t			*/
 #include <linux/compiler.h>		/* __user			*/
+#include <linux/list.h>			/* struct list_head             */
 
 struct pid;
 struct cred;
@@ -75,6 +76,13 @@ struct mmsghdr {
 	unsigned        msg_len;
 };
 
+/* For remembering destination's address passed to sendmmsg(). */
+struct sendmmsg_dest_info {
+	struct list_head list;
+	unsigned int address_len;
+	struct sockaddr_storage address;
+};
+
 /*
  *	POSIX 1003.1g - ancillary data object information
  *	Ancillary data consits of a sequence of pairs of
--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -558,9 +558,10 @@ static inline int __sock_sendmsg_nosec(s
 }
 
 static inline int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,
-				 struct msghdr *msg, size_t size)
+				 struct msghdr *msg, size_t size,
+				 int datagrams, struct list_head *list)
 {
-	int err = security_socket_sendmsg(sock, msg, size);
+	int err = security_socket_sendmsg(sock, msg, size, datagrams, list);
 
 	return err ?: __sock_sendmsg_nosec(iocb, sock, msg, size);
 }
@@ -573,14 +574,16 @@ int sock_sendmsg(struct socket *sock, st
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, 0, NULL);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
+static int sock_send_datagrams(struct socket *sock, struct msghdr *msg,
+			       size_t size, int datagrams,
+			       struct list_head *list)
 {
 	struct kiocb iocb;
 	struct sock_iocb siocb;
@@ -588,7 +591,7 @@ int sock_sendmsg_nosec(struct socket *so
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg_nosec(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, datagrams, list);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
@@ -888,7 +891,7 @@ static ssize_t do_sock_write(struct msgh
 	if (sock->type == SOCK_SEQPACKET)
 		msg->msg_flags |= MSG_EOR;
 
-	return __sock_sendmsg(iocb, sock, msg, size);
+	return __sock_sendmsg(iocb, sock, msg, size, 0, NULL);
 }
 
 static ssize_t sock_aio_write(struct kiocb *iocb, const struct iovec *iov,
@@ -1872,7 +1875,8 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags, int datagrams,
+			 struct list_head *list)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1953,8 +1957,7 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
-							  total_len);
+	err = sock_send_datagrams(sock, msg_sys, total_len, datagrams, list);
 
 out_freectl:
 	if (ctl_buf != ctl)
@@ -1979,7 +1982,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0, NULL);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -1998,6 +2001,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
+	LIST_HEAD(list); /* List for finding duplicated destination address. */
 
 	datagrams = 0;
 
@@ -2014,18 +2018,19 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 	while (datagrams < vlen) {
 		/*
-		 * No need to ask LSM for more than the first datagram.
+		 * If datagrams == 0, LSM module will check. Otherwise, it will
+		 * check depending on its implementation.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);
@@ -2038,6 +2043,17 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	}
 
 out_put:
+#ifdef CONFIG_SECURITY_NETWORK
+	{ /* Clean up destination addresses. */
+		struct sendmmsg_dest_info *ptr;
+		struct sendmmsg_dest_info *tmp;
+
+		list_for_each_entry_safe(ptr, tmp, &list, list) {
+			list_del(&ptr->list);
+			kfree(ptr);
+		}
+	}
+#endif
 	fput_light(sock->file, fput_needed);
 
 	if (err == 0)
--- linux-3.0.orig/security/capability.c
+++ linux-3.0/security/capability.c
@@ -593,7 +593,8 @@ static int cap_socket_accept(struct sock
 	return 0;
 }
 
-static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			      int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0.orig/security/security.c
+++ linux-3.0/security/security.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 #include <linux/ima.h>
+#include <linux/socket.h>
 
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -1036,9 +1037,56 @@ int security_socket_accept(struct socket
 	return security_ops->socket_accept(sock, newsock);
 }
 
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list)
 {
-	return security_ops->socket_sendmsg(sock, msg, size);
+	return security_ops->socket_sendmsg(sock, msg, size, datagrams, list);
+}
+
+/**
+ * security_sendmsg_uniq_address - Check for duplicated address.
+ *
+ * @msg:  Pointer to "struct msg".
+ * @list: Pointer to "struct list_head".
+ *
+ * Returns true if @msg->msg_name is already in @list, false otherwise.
+ * @msg->msg_name will be duplicated and added to @list (unless out-of-memory
+ * occurs) if this function returns true. __sys_sendmmsg() provides @list and
+ * will clean up allocated memory before return.
+ *
+ * Some LSM modules check permission based on destination address at
+ * security_socket_sendmsg(). But checking for duplicated destination
+ * address at common code path is waste of time unless such LSM module is
+ * selected. Therefore, let such LSM modules call this function if they want to
+ * check permission only once for each uniq destination address.
+ */
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list)
+{
+	struct sendmmsg_dest_info *ptr;
+
+	/* If not sendmmsg(), this address is uniq. */
+	if (!list)
+		return true;
+	/* If sendmmsg(), check if this address was already used. */
+	list_for_each_entry(ptr, list, list) {
+		if (ptr->address_len != msg->msg_namelen ||
+		    memcmp(&ptr->address, msg->msg_name, ptr->address_len))
+			continue;
+		return false;
+	}
+	/*
+	 * Remember this address so that subsequent call will return false.
+	 *
+	 * Out of memory error is not fatal here because checking more than
+	 * once should be harmless other than the performance loss.
+	 */
+	ptr = kmalloc(sizeof(*ptr), GFP_KERNEL);
+	if (ptr) {
+		ptr->address_len = msg->msg_namelen;
+		memcpy(&ptr->address, msg->msg_name, ptr->address_len);
+		list_add(&ptr->list, list);
+	}
+	return true;
 }
 
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0.orig/security/selinux/hooks.c
+++ linux-3.0/security/selinux/hooks.c
@@ -3967,9 +3967,10 @@ static int selinux_socket_accept(struct 
 }
 
 static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				  int size)
+				  int size, int datagrams,
+				  struct list_head *list)
 {
-	return sock_has_perm(current, sock->sk, SOCKET__WRITE);
+	return datagrams ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
 }
 
 static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0.orig/security/smack/smack_lsm.c
+++ linux-3.0/security/smack/smack_lsm.c
@@ -2799,7 +2799,7 @@ static int smack_unix_may_send(struct so
  * label host.
  */
 static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				int size)
+				int size, int datagrams, struct list_head *list)
 {
 	struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;
 
@@ -2809,6 +2809,9 @@ static int smack_socket_sendmsg(struct s
 	if (sip == NULL || sip->sin_family != AF_INET)
 		return 0;
 
+	if (!security_sendmsg_uniq_address(msg, list))
+		return 0;
+
 	return smack_netlabel_send(sock->sk, sip);
 }
 

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: Thu, 28 Jul 2011 12:36:30 +0900

> Here is an optimized version. Only compile tested.

Anton, please test this or I'm just going to apply it before it
gets any testing :-)

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Date: Thu, 28 Jul 2011 12:36:30 +0900
> 
> > Here is an optimized version. Only compile tested.
> 
> Anton, please test this or I'm just going to apply it before it
> gets any testing :-)

Just a moment please. I found a problem (described below).

I tested on TOMOYO using below program

--- Start of test program ---
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <netdb.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <net/if.h>
#include <net/ethernet.h>
#include <linux/if_packet.h>
#include <asm/unistd.h>
#include <errno.h>

#ifndef __NR_sendmmsg
#if defined( __PPC__)
#define __NR_sendmmsg   349
#elif defined(__x86_64__)
#define __NR_sendmmsg   307
#elif defined(__i386__)
#define __NR_sendmmsg   345
#else
#error __NR_sendmmsg not defined
#endif
#endif

struct mmsghdr {
        struct msghdr msg_hdr;
        unsigned int msg_len;
};

static inline int sendmmsg(int fd, struct mmsghdr *mmsg, unsigned vlen,
                           unsigned flags)
{
        return syscall(__NR_sendmmsg, fd, mmsg, vlen, flags, NULL);
}

static unsigned long packets;
static unsigned long packets_prev;

static void sigalrm_handler(int junk)
{
        unsigned long p = packets;

        printf("%ld\n", p - packets_prev);
        packets_prev = p;
        alarm(1);
}

#define NUMDESTS 20
#define NUMBATCH 100

int main(int argc, char *argv[])
{
        const int fd = socket(AF_INET, SOCK_DGRAM, 0);
        unsigned int i;
        char b[10];
        static char buf[NUMBATCH][10];
        static struct iovec iovec[NUMBATCH][1];
        static struct mmsghdr datagrams[NUMBATCH];
        struct sockaddr_in addr[NUMDESTS];

        signal(SIGALRM, sigalrm_handler);
        alarm(1);
        memset(buf, 0, sizeof(buf));
        memset(iovec, 0, sizeof(iovec));
        memset(datagrams, 0, sizeof(datagrams));
        memset(&addr, 0, sizeof(addr));

        for (i = 0; i < sizeof(b); i++)
                b[i]= i;
        for (i = 0; i < NUMDESTS; i++) {
                addr[i].sin_family = AF_INET;
                addr[i].sin_addr.s_addr = htonl(INADDR_LOOPBACK);
                addr[i].sin_port = htons(10000 + (argc == 1 ? i : 0) * 10);
        }
        for (i = 0; i < NUMBATCH; ++i) {
                memcpy(&buf[i], b, sizeof(buf[i]));
                iovec[i][0].iov_base = buf[i];
                iovec[i][0].iov_len = sizeof(buf[i]);
                datagrams[i].msg_hdr.msg_iov = iovec[i];
                datagrams[i].msg_hdr.msg_iovlen = 1;
                datagrams[i].msg_hdr.msg_name = &addr[i % NUMDESTS];
                datagrams[i].msg_hdr.msg_namelen = sizeof(addr[0]);
        }

        while (1) {
                int ret;

                errno = 0;
                ret = sendmmsg(fd, datagrams, NUMBATCH, 0);
                if (ret < 0) {
                        perror("sendmmsg");
                        exit(1);
                }

                if (ret != NUMBATCH) {
                        fprintf(stderr,
                                "sendmmsg returned sent less than batch %d\n", ret);
                }

                packets += ret;
        }
        return 0;
}
--- End of test program ---

and realized that remembering the fact that LSM module has returned an error
code other than -EAGAIN causes subsequent sendmmsg() request to fail with that
error code.

        fput_light(sock->file, fput_needed);

        if (err == 0)
                return datagrams;

        if (datagrams != 0) {
                /*
                 * We may send less entries than requested (vlen) if the
                 * sock is non blocking...
                 */
                if (err != -EAGAIN) {
                        /*
                         * ... or if sendmsg returns an error after we
                         * send some datagrams, where we record the
                         * error to return on the next call or if the
                         * app asks about it using getsockopt(SO_ERROR).
                         */
                        sock->sk->sk_err = -err;
                }

                return datagrams;
        }

        return err;

For example, if TOMOYO returned -EPERM on the 14th datagram,
subsequent sendmmsg() fails with EPERM and the program will exit().

	sendmmsg returned sent less than batch 14
	sendmmsg: Operation not permitted

I think this behavior is not preferable.
In this case, should security_socket_sendmsg() return -EAGAIN rather than
-EPERM?
Or, should sendmmsg() not record errors after some of datagrams were sent?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: Tue, 02 Aug 2011 18:28:40 +0900

> I think this behavior is not preferable.  In this case, should
> security_socket_sendmsg() return -EAGAIN rather than -EPERM?  Or,
> should sendmmsg() not record errors after some of datagrams were
> sent?

I think you must return -EAGAIN so that the user can see how many
of the datagrams were sent successfully.

In fact, it is a requirement.  What if the sent datagrams have
side effects (f.e. money moves from one bank account to another)?

How else can the application find this out?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 02 Aug 2011 04:18:57 -0700 (PDT)

> From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Date: Tue, 02 Aug 2011 18:28:40 +0900
> 
>> I think this behavior is not preferable.  In this case, should
>> security_socket_sendmsg() return -EAGAIN rather than -EPERM?  Or,
>> should sendmmsg() not record errors after some of datagrams were
>> sent?
> 
> I think you must return -EAGAIN so that the user can see how many
> of the datagrams were sent successfully.
> 
> In fact, it is a requirement.  What if the sent datagrams have
> side effects (f.e. money moves from one bank account to another)?
> 
> How else can the application find this out?

Actually, I change my mind. :-)

I think sendmmsg() needs to unconditionally not report an error if any
datagrams were sent successfully.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

David Miller wrote:
> Actually, I change my mind. :-)
> 
> I think sendmmsg() needs to unconditionally not report an error if any
> datagrams were sent successfully.

What about adding

 #ifdef CONFIG_SECURITY_NETWORK
 static inline bool security_socket_may_send_multiple_address(void)
 {
	return security_ops->socket_may_send_multiple_address;
 }
 #else
 static inline bool security_socket_may_send_multiple_address(void)
 {
	return true;
 }
 #endif

and letting SMACK and TOMOYO return false and others return true?

The check will look like

  if (sendmmsg) {
    Record destination address of first datagram if first datagram,
    compare with recorded address and subsequent datagram otherwise.
    If same address, continue. Otherwise, call
    security_socket_may_send_multiple_address() and break if it returns false.
  }

. The side effect is that sendmmsg() will be allowed to send to single
destination if underlying  LSM module does not permit sending to multiple
address, to multiple destination otherwise. As long as sendmmsg() is used for
sending to single destination, there will be no performance loss.

It will be kmalloc()-free, fast and simple. Also, makes LSM stacking easier.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 2 Aug 2011 20:52:05 +0900

> David Miller wrote:
>> Actually, I change my mind. :-)
>> 
>> I think sendmmsg() needs to unconditionally not report an error if any
>> datagrams were sent successfully.
> 
> What about adding

I much prefer to make the error handling more correct, rather than
making sendmmsg() have fundamentally different semantics depending
upon the underlying LSM.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 2 Aug 2011 20:52:05 +0900
> 
> > David Miller wrote:
> >> Actually, I change my mind. :-)
> >> 
> >> I think sendmmsg() needs to unconditionally not report an error if any
> >> datagrams were sent successfully.
> > 
> > What about adding
> 
> I much prefer to make the error handling more correct, rather than
> making sendmmsg() have fundamentally different semantics depending
> upon the underlying LSM.
> 
I see. Here is an updated version.

Changes since V1:
  In __sys_sendmmsg(), ignore errors if one or more datagrams were
  sent successfully.

But regarding
| I think sendmmsg() needs to unconditionally not report an error if any
| datagrams were sent successfully.
what should I do if put_user() returned error?
A datagram was sent successfully but will return -EFAULT (although I doubt
when such thing can happen).

        while (datagrams < vlen) {
                /*
                 * If datagrams == 0, LSM module will check. Otherwise, it will
                 * check depending on its implementation.
                 */
                if (MSG_CMSG_COMPAT & flags) {
                        err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
                                            &msg_sys, flags, datagrams, &list);
                        if (err < 0)
                                break;
                        err = __put_user(err, &compat_entry->msg_len);
                        ++compat_entry;
                } else {
                        err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
                                            &msg_sys, flags, datagrams, &list);
                        if (err < 0)
                                break;
                        err = put_user(err, &entry->msg_len);
                        ++entry;
                }

                if (err)
                        break;
                ++datagrams;
        }

----------------------------------------
[PATCH v2] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than only the first destination address.

Fix this regression by
(1) passing "int datagrams" argument to security_socket_sendmsg() so that
    SELinux can omit sock_has_perm() checks on the 2nd or later.
(2) passing "struct list_head *list" argument to security_socket_sendmsg() so
    that SMACK can omit smack_netlabel_send() checks for duplicated destination
    address.
(3) letting __sys_sendmmsg() provide "struct list_head list" for
    security_socket_sendmsg() and clean it up before return.

Also, it is not preferable to let next sendmmsg() return previous sendmmsg()'s
error code when some of requested entries were not sent, for the caller will
get confused by next sendmmsg(). Thus, ignore errors if one or more entries
were sent successfully.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 include/linux/security.h   |   16 ++++++++--
 include/linux/socket.h     |    8 +++++
 net/socket.c               |   67 +++++++++++++++++++++++----------------------
 security/capability.c      |    3 +-
 security/security.c        |   52 +++++++++++++++++++++++++++++++++-
 security/selinux/hooks.c   |    5 ++-
 security/smack/smack_lsm.c |    5 ++-
 7 files changed, 114 insertions(+), 42 deletions(-)

--- linux-3.0.orig/include/linux/security.h
+++ linux-3.0/include/linux/security.h
@@ -93,6 +93,7 @@ struct xfrm_policy;
 struct xfrm_state;
 struct xfrm_user_sec_ctx;
 struct seq_file;
+struct list_head;
 
 extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
 extern int cap_netlink_recv(struct sk_buff *skb, int cap);
@@ -880,6 +881,10 @@ static inline void security_free_mnt_opt
  *	@sock contains the socket structure.
  *	@msg contains the message to be transmitted.
  *	@size contains the size of message.
+ *	@datagrams contains the index of messages in sendmmsg(). This is 0 if
+ *	not sendmmsg().
+ *	@list contains the list head which can be used for holding
+ *	already-checked destination address. This is NULL if not sendmmsg().
  *	Return 0 if permission is granted.
  * @socket_recvmsg:
  *	Check permission before receiving a message from a socket.
@@ -1584,8 +1589,8 @@ struct security_operations {
 			       struct sockaddr *address, int addrlen);
 	int (*socket_listen) (struct socket *sock, int backlog);
 	int (*socket_accept) (struct socket *sock, struct socket *newsock);
-	int (*socket_sendmsg) (struct socket *sock,
-			       struct msghdr *msg, int size);
+	int (*socket_sendmsg) (struct socket *sock, struct msghdr *msg,
+			       int size, int datagrams, struct list_head *list);
 	int (*socket_recvmsg) (struct socket *sock,
 			       struct msghdr *msg, int size, int flags);
 	int (*socket_getsockname) (struct socket *sock);
@@ -2551,7 +2556,9 @@ int security_socket_bind(struct socket *
 int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen);
 int security_socket_listen(struct socket *sock, int backlog);
 int security_socket_accept(struct socket *sock, struct socket *newsock);
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list);
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
 int security_socket_getsockname(struct socket *sock);
@@ -2636,7 +2643,8 @@ static inline int security_socket_accept
 }
 
 static inline int security_socket_sendmsg(struct socket *sock,
-					  struct msghdr *msg, int size)
+					  struct msghdr *msg, int size,
+					  int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0.orig/include/linux/socket.h
+++ linux-3.0/include/linux/socket.h
@@ -23,6 +23,7 @@ struct __kernel_sockaddr_storage {
 #include <linux/uio.h>			/* iovec support		*/
 #include <linux/types.h>		/* pid_t			*/
 #include <linux/compiler.h>		/* __user			*/
+#include <linux/list.h>			/* struct list_head             */
 
 struct pid;
 struct cred;
@@ -75,6 +76,13 @@ struct mmsghdr {
 	unsigned        msg_len;
 };
 
+/* For remembering destination's address passed to sendmmsg(). */
+struct sendmmsg_dest_info {
+	struct list_head list;
+	unsigned int address_len;
+	struct sockaddr_storage address;
+};
+
 /*
  *	POSIX 1003.1g - ancillary data object information
  *	Ancillary data consits of a sequence of pairs of
--- linux-3.0.orig/net/socket.c
+++ linux-3.0/net/socket.c
@@ -558,9 +558,10 @@ static inline int __sock_sendmsg_nosec(s
 }
 
 static inline int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,
-				 struct msghdr *msg, size_t size)
+				 struct msghdr *msg, size_t size,
+				 int datagrams, struct list_head *list)
 {
-	int err = security_socket_sendmsg(sock, msg, size);
+	int err = security_socket_sendmsg(sock, msg, size, datagrams, list);
 
 	return err ?: __sock_sendmsg_nosec(iocb, sock, msg, size);
 }
@@ -573,14 +574,16 @@ int sock_sendmsg(struct socket *sock, st
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, 0, NULL);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
+static int sock_send_datagrams(struct socket *sock, struct msghdr *msg,
+			       size_t size, int datagrams,
+			       struct list_head *list)
 {
 	struct kiocb iocb;
 	struct sock_iocb siocb;
@@ -588,7 +591,7 @@ int sock_sendmsg_nosec(struct socket *so
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg_nosec(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, datagrams, list);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
@@ -888,7 +891,7 @@ static ssize_t do_sock_write(struct msgh
 	if (sock->type == SOCK_SEQPACKET)
 		msg->msg_flags |= MSG_EOR;
 
-	return __sock_sendmsg(iocb, sock, msg, size);
+	return __sock_sendmsg(iocb, sock, msg, size, 0, NULL);
 }
 
 static ssize_t sock_aio_write(struct kiocb *iocb, const struct iovec *iov,
@@ -1872,7 +1875,8 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags, int datagrams,
+			 struct list_head *list)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1953,8 +1957,7 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
-							  total_len);
+	err = sock_send_datagrams(sock, msg_sys, total_len, datagrams, list);
 
 out_freectl:
 	if (ctl_buf != ctl)
@@ -1979,7 +1982,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0, NULL);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -1998,6 +2001,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
+	LIST_HEAD(list); /* List for finding duplicated destination address. */
 
 	datagrams = 0;
 
@@ -2014,18 +2018,19 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 	while (datagrams < vlen) {
 		/*
-		 * No need to ask LSM for more than the first datagram.
+		 * If datagrams == 0, LSM module will check. Otherwise, it will
+		 * check depending on its implementation.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);
@@ -2038,29 +2043,27 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	}
 
 out_put:
-	fput_light(sock->file, fput_needed);
-
-	if (err == 0)
-		return datagrams;
-
-	if (datagrams != 0) {
-		/*
-		 * We may send less entries than requested (vlen) if the
-		 * sock is non blocking...
-		 */
-		if (err != -EAGAIN) {
-			/*
-			 * ... or if sendmsg returns an error after we
-			 * send some datagrams, where we record the
-			 * error to return on the next call or if the
-			 * app asks about it using getsockopt(SO_ERROR).
-			 */
-			sock->sk->sk_err = -err;
+#ifdef CONFIG_SECURITY_NETWORK
+	{ /* Clean up destination addresses. */
+		struct sendmmsg_dest_info *ptr;
+		struct sendmmsg_dest_info *tmp;
+
+		list_for_each_entry_safe(ptr, tmp, &list, list) {
+			list_del(&ptr->list);
+			kfree(ptr);
 		}
+	}
+#endif
+	fput_light(sock->file, fput_needed);
 
+	/*
+	 * We may send less entries than requested (vlen), but we ignore errors
+	 * if one or more entries were sent successfully.
+	 */
+	if (datagrams)
 		return datagrams;
-	}
 
+	/* Report errors only if no entry was sent. */
 	return err;
 }
 
--- linux-3.0.orig/security/capability.c
+++ linux-3.0/security/capability.c
@@ -593,7 +593,8 @@ static int cap_socket_accept(struct sock
 	return 0;
 }
 
-static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			      int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0.orig/security/security.c
+++ linux-3.0/security/security.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 #include <linux/ima.h>
+#include <linux/socket.h>
 
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -1036,9 +1037,56 @@ int security_socket_accept(struct socket
 	return security_ops->socket_accept(sock, newsock);
 }
 
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list)
 {
-	return security_ops->socket_sendmsg(sock, msg, size);
+	return security_ops->socket_sendmsg(sock, msg, size, datagrams, list);
+}
+
+/**
+ * security_sendmsg_uniq_address - Check for duplicated address.
+ *
+ * @msg:  Pointer to "struct msg".
+ * @list: Pointer to "struct list_head".
+ *
+ * Returns true if @msg->msg_name is already in @list, false otherwise.
+ * @msg->msg_name will be duplicated and added to @list (unless out-of-memory
+ * occurs) if this function returns true. __sys_sendmmsg() provides @list and
+ * will clean up allocated memory before return.
+ *
+ * Some LSM modules check permission based on destination address at
+ * security_socket_sendmsg(). But checking for duplicated destination
+ * address at common code path is waste of time unless such LSM module is
+ * selected. Therefore, let such LSM modules call this function if they want to
+ * check permission only once for each uniq destination address.
+ */
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list)
+{
+	struct sendmmsg_dest_info *ptr;
+
+	/* If not sendmmsg(), this address is uniq. */
+	if (!list)
+		return true;
+	/* If sendmmsg(), check if this address was already used. */
+	list_for_each_entry(ptr, list, list) {
+		if (ptr->address_len != msg->msg_namelen ||
+		    memcmp(&ptr->address, msg->msg_name, ptr->address_len))
+			continue;
+		return false;
+	}
+	/*
+	 * Remember this address so that subsequent call will return false.
+	 *
+	 * Out of memory error is not fatal here because checking more than
+	 * once should be harmless other than the performance loss.
+	 */
+	ptr = kmalloc(sizeof(*ptr), GFP_KERNEL);
+	if (ptr) {
+		ptr->address_len = msg->msg_namelen;
+		memcpy(&ptr->address, msg->msg_name, ptr->address_len);
+		list_add(&ptr->list, list);
+	}
+	return true;
 }
 
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0.orig/security/selinux/hooks.c
+++ linux-3.0/security/selinux/hooks.c
@@ -3967,9 +3967,10 @@ static int selinux_socket_accept(struct 
 }
 
 static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				  int size)
+				  int size, int datagrams,
+				  struct list_head *list)
 {
-	return sock_has_perm(current, sock->sk, SOCKET__WRITE);
+	return datagrams ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
 }
 
 static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0.orig/security/smack/smack_lsm.c
+++ linux-3.0/security/smack/smack_lsm.c
@@ -2799,7 +2799,7 @@ static int smack_unix_may_send(struct so
  * label host.
  */
 static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				int size)
+				int size, int datagrams, struct list_head *list)
 {
 	struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;
 
@@ -2809,6 +2809,9 @@ static int smack_socket_sendmsg(struct s
 	if (sip == NULL || sip->sin_family != AF_INET)
 		return 0;
 
+	if (!security_sendmsg_uniq_address(msg, list))
+		return 0;
+
 	return smack_netlabel_send(sock->sk, sip);
 }
 

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Tetsuo Handa wrote:
> I see. Here is an updated version.
Oops, seems whitespace damaged. Resending.

Also, attaching kmalloc()-free version. If performance loss by kmalloc()-free
version is small enough, can it be a candidate?
----------------------------------------
[PATCH v2] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destinations. However,
security_socket_sendmsg() is called for only once even if multiple different
destination's addresses are passed to sendmmsg().

SMACK is using destination's address for checking sendmsg() permission.
Therefore, we need to call security_socket_sendmsg() for each destination
address rather than only the first destination address.

Fix this regression by
(1) passing "int datagrams" argument to security_socket_sendmsg() so that
    SELinux can omit sock_has_perm() checks on the 2nd or later.
(2) passing "struct list_head *list" argument to security_socket_sendmsg() so
    that SMACK can omit smack_netlabel_send() checks for duplicated destination
    address.
(3) letting __sys_sendmmsg() provide "struct list_head list" for
    security_socket_sendmsg() and clean it up before return.

Also, it is not preferable to let next sendmmsg() return previous sendmmsg()'s
error code when some of requested entries were not sent, for the caller will
get confused by next sendmmsg(). Thus, ignore errors if one or more entries
were sent successfully.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 include/linux/security.h   |   16 ++++++++--
 include/linux/socket.h     |    8 +++++
 net/socket.c               |   67 +++++++++++++++++++++++----------------------
 security/capability.c      |    3 +-
 security/security.c        |   52 +++++++++++++++++++++++++++++++++-
 security/selinux/hooks.c   |    5 ++-
 security/smack/smack_lsm.c |    5 ++-
 7 files changed, 114 insertions(+), 42 deletions(-)

--- linux-3.0-a.orig/include/linux/security.h
+++ linux-3.0-a/include/linux/security.h
@@ -93,6 +93,7 @@ struct xfrm_policy;
 struct xfrm_state;
 struct xfrm_user_sec_ctx;
 struct seq_file;
+struct list_head;
 
 extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
 extern int cap_netlink_recv(struct sk_buff *skb, int cap);
@@ -880,6 +881,10 @@ static inline void security_free_mnt_opt
  *	@sock contains the socket structure.
  *	@msg contains the message to be transmitted.
  *	@size contains the size of message.
+ *	@datagrams contains the index of messages in sendmmsg(). This is 0 if
+ *	not sendmmsg().
+ *	@list contains the list head which can be used for holding
+ *	already-checked destination address. This is NULL if not sendmmsg().
  *	Return 0 if permission is granted.
  * @socket_recvmsg:
  *	Check permission before receiving a message from a socket.
@@ -1584,8 +1589,8 @@ struct security_operations {
 			       struct sockaddr *address, int addrlen);
 	int (*socket_listen) (struct socket *sock, int backlog);
 	int (*socket_accept) (struct socket *sock, struct socket *newsock);
-	int (*socket_sendmsg) (struct socket *sock,
-			       struct msghdr *msg, int size);
+	int (*socket_sendmsg) (struct socket *sock, struct msghdr *msg,
+			       int size, int datagrams, struct list_head *list);
 	int (*socket_recvmsg) (struct socket *sock,
 			       struct msghdr *msg, int size, int flags);
 	int (*socket_getsockname) (struct socket *sock);
@@ -2551,7 +2556,9 @@ int security_socket_bind(struct socket *
 int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen);
 int security_socket_listen(struct socket *sock, int backlog);
 int security_socket_accept(struct socket *sock, struct socket *newsock);
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list);
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
 int security_socket_getsockname(struct socket *sock);
@@ -2636,7 +2643,8 @@ static inline int security_socket_accept
 }
 
 static inline int security_socket_sendmsg(struct socket *sock,
-					  struct msghdr *msg, int size)
+					  struct msghdr *msg, int size,
+					  int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0-a.orig/include/linux/socket.h
+++ linux-3.0-a/include/linux/socket.h
@@ -23,6 +23,7 @@ struct __kernel_sockaddr_storage {
 #include <linux/uio.h>			/* iovec support		*/
 #include <linux/types.h>		/* pid_t			*/
 #include <linux/compiler.h>		/* __user			*/
+#include <linux/list.h>			/* struct list_head             */
 
 struct pid;
 struct cred;
@@ -75,6 +76,13 @@ struct mmsghdr {
 	unsigned        msg_len;
 };
 
+/* For remembering destination's address passed to sendmmsg(). */
+struct sendmmsg_dest_info {
+	struct list_head list;
+	unsigned int address_len;
+	struct sockaddr_storage address;
+};
+
 /*
  *	POSIX 1003.1g - ancillary data object information
  *	Ancillary data consits of a sequence of pairs of
--- linux-3.0-a.orig/net/socket.c
+++ linux-3.0-a/net/socket.c
@@ -558,9 +558,10 @@ static inline int __sock_sendmsg_nosec(s
 }
 
 static inline int __sock_sendmsg(struct kiocb *iocb, struct socket *sock,
-				 struct msghdr *msg, size_t size)
+				 struct msghdr *msg, size_t size,
+				 int datagrams, struct list_head *list)
 {
-	int err = security_socket_sendmsg(sock, msg, size);
+	int err = security_socket_sendmsg(sock, msg, size, datagrams, list);
 
 	return err ?: __sock_sendmsg_nosec(iocb, sock, msg, size);
 }
@@ -573,14 +574,16 @@ int sock_sendmsg(struct socket *sock, st
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, 0, NULL);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
+static int sock_send_datagrams(struct socket *sock, struct msghdr *msg,
+			       size_t size, int datagrams,
+			       struct list_head *list)
 {
 	struct kiocb iocb;
 	struct sock_iocb siocb;
@@ -588,7 +591,7 @@ int sock_sendmsg_nosec(struct socket *so
 
 	init_sync_kiocb(&iocb, NULL);
 	iocb.private = &siocb;
-	ret = __sock_sendmsg_nosec(&iocb, sock, msg, size);
+	ret = __sock_sendmsg(&iocb, sock, msg, size, datagrams, list);
 	if (-EIOCBQUEUED == ret)
 		ret = wait_on_sync_kiocb(&iocb);
 	return ret;
@@ -888,7 +891,7 @@ static ssize_t do_sock_write(struct msgh
 	if (sock->type == SOCK_SEQPACKET)
 		msg->msg_flags |= MSG_EOR;
 
-	return __sock_sendmsg(iocb, sock, msg, size);
+	return __sock_sendmsg(iocb, sock, msg, size, 0, NULL);
 }
 
 static ssize_t sock_aio_write(struct kiocb *iocb, const struct iovec *iov,
@@ -1872,7 +1875,8 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags, int datagrams,
+			 struct list_head *list)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1953,8 +1957,7 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
-							  total_len);
+	err = sock_send_datagrams(sock, msg_sys, total_len, datagrams, list);
 
 out_freectl:
 	if (ctl_buf != ctl)
@@ -1979,7 +1982,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0, NULL);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -1998,6 +2001,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
+	LIST_HEAD(list); /* List for finding duplicated destination address. */
 
 	datagrams = 0;
 
@@ -2014,18 +2018,19 @@ int __sys_sendmmsg(int fd, struct mmsghd
 
 	while (datagrams < vlen) {
 		/*
-		 * No need to ask LSM for more than the first datagram.
+		 * If datagrams == 0, LSM module will check. Otherwise, it will
+		 * check depending on its implementation.
 		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, datagrams, &list);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);
@@ -2038,29 +2043,27 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	}
 
 out_put:
-	fput_light(sock->file, fput_needed);
-
-	if (err == 0)
-		return datagrams;
-
-	if (datagrams != 0) {
-		/*
-		 * We may send less entries than requested (vlen) if the
-		 * sock is non blocking...
-		 */
-		if (err != -EAGAIN) {
-			/*
-			 * ... or if sendmsg returns an error after we
-			 * send some datagrams, where we record the
-			 * error to return on the next call or if the
-			 * app asks about it using getsockopt(SO_ERROR).
-			 */
-			sock->sk->sk_err = -err;
+#ifdef CONFIG_SECURITY_NETWORK
+	{ /* Clean up destination addresses. */
+		struct sendmmsg_dest_info *ptr;
+		struct sendmmsg_dest_info *tmp;
+
+		list_for_each_entry_safe(ptr, tmp, &list, list) {
+			list_del(&ptr->list);
+			kfree(ptr);
 		}
+	}
+#endif
+	fput_light(sock->file, fput_needed);
 
+	/*
+	 * We may send less entries than requested (vlen), but we ignore errors
+	 * if one or more entries were sent successfully.
+	 */
+	if (datagrams)
 		return datagrams;
-	}
 
+	/* Report errors only if no entry was sent. */
 	return err;
 }
 
--- linux-3.0-a.orig/security/capability.c
+++ linux-3.0-a/security/capability.c
@@ -593,7 +593,8 @@ static int cap_socket_accept(struct sock
 	return 0;
 }
 
-static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			      int datagrams, struct list_head *list)
 {
 	return 0;
 }
--- linux-3.0-a.orig/security/security.c
+++ linux-3.0-a/security/security.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/security.h>
 #include <linux/ima.h>
+#include <linux/socket.h>
 
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -1036,9 +1037,56 @@ int security_socket_accept(struct socket
 	return security_ops->socket_accept(sock, newsock);
 }
 
-int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
+int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size,
+			    int datagrams, struct list_head *list)
 {
-	return security_ops->socket_sendmsg(sock, msg, size);
+	return security_ops->socket_sendmsg(sock, msg, size, datagrams, list);
+}
+
+/**
+ * security_sendmsg_uniq_address - Check for duplicated address.
+ *
+ * @msg:  Pointer to "struct msg".
+ * @list: Pointer to "struct list_head".
+ *
+ * Returns true if @msg->msg_name is already in @list, false otherwise.
+ * @msg->msg_name will be duplicated and added to @list (unless out-of-memory
+ * occurs) if this function returns true. __sys_sendmmsg() provides @list and
+ * will clean up allocated memory before return.
+ *
+ * Some LSM modules check permission based on destination address at
+ * security_socket_sendmsg(). But checking for duplicated destination
+ * address at common code path is waste of time unless such LSM module is
+ * selected. Therefore, let such LSM modules call this function if they want to
+ * check permission only once for each uniq destination address.
+ */
+bool security_sendmsg_uniq_address(struct msghdr *msg, struct list_head *list)
+{
+	struct sendmmsg_dest_info *ptr;
+
+	/* If not sendmmsg(), this address is uniq. */
+	if (!list)
+		return true;
+	/* If sendmmsg(), check if this address was already used. */
+	list_for_each_entry(ptr, list, list) {
+		if (ptr->address_len != msg->msg_namelen ||
+		    memcmp(&ptr->address, msg->msg_name, ptr->address_len))
+			continue;
+		return false;
+	}
+	/*
+	 * Remember this address so that subsequent call will return false.
+	 *
+	 * Out of memory error is not fatal here because checking more than
+	 * once should be harmless other than the performance loss.
+	 */
+	ptr = kmalloc(sizeof(*ptr), GFP_KERNEL);
+	if (ptr) {
+		ptr->address_len = msg->msg_namelen;
+		memcpy(&ptr->address, msg->msg_name, ptr->address_len);
+		list_add(&ptr->list, list);
+	}
+	return true;
 }
 
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0-a.orig/security/selinux/hooks.c
+++ linux-3.0-a/security/selinux/hooks.c
@@ -3967,9 +3967,10 @@ static int selinux_socket_accept(struct 
 }
 
 static int selinux_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				  int size)
+				  int size, int datagrams,
+				  struct list_head *list)
 {
-	return sock_has_perm(current, sock->sk, SOCKET__WRITE);
+	return datagrams ? 0 : sock_has_perm(current, sock->sk, SOCKET__WRITE);
 }
 
 static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg,
--- linux-3.0-a.orig/security/smack/smack_lsm.c
+++ linux-3.0-a/security/smack/smack_lsm.c
@@ -2799,7 +2799,7 @@ static int smack_unix_may_send(struct so
  * label host.
  */
 static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
-				int size)
+				int size, int datagrams, struct list_head *list)
 {
 	struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;
 
@@ -2809,6 +2809,9 @@ static int smack_socket_sendmsg(struct s
 	if (sip == NULL || sip->sin_family != AF_INET)
 		return 0;
 
+	if (!security_sendmsg_uniq_address(msg, list))
+		return 0;
+
 	return smack_netlabel_send(sock->sk, sip);
 }
 
----------------------------------------
[PATCH] net: Fix security_socket_sendmsg() bypass problem.

The sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
call" is capable of sending to multiple different destination addresses.

SMACK is using destination's address for checking sendmsg() permission.
However, security_socket_sendmsg() is called for only once even if multiple
different destination addresses are passed to sendmmsg().

Therefore, we need to call security_socket_sendmsg() for each destination
address rather than only the first destination address.

Since calling security_socket_sendmsg() every time when only single destination
address was passed to sendmmsg() is a waste of time, omit calling
security_socket_sendmsg() unless destination address of previous datagram and
that of current datagram differs.

Also, it is not preferable to let next sendmmsg() return previous sendmmsg()'s
error code when some of requested entries were not sent, for the caller will
get confused by next sendmmsg(). Thus, ignore errors if one or more entries
were sent successfully.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable <stable@kernel.org> [3.0+]
---
 net/socket.c |   71 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 29 deletions(-)

--- linux-3.0-b.orig/net/socket.c
+++ linux-3.0-b/net/socket.c
@@ -580,7 +580,8 @@ int sock_sendmsg(struct socket *sock, st
 }
 EXPORT_SYMBOL(sock_sendmsg);
 
-int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg, size_t size)
+static int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg,
+			      size_t size)
 {
 	struct kiocb iocb;
 	struct sock_iocb siocb;
@@ -1871,8 +1872,14 @@ SYSCALL_DEFINE2(shutdown, int, fd, int, 
 #define COMPAT_NAMELEN(msg)	COMPAT_MSG(msg, msg_namelen)
 #define COMPAT_FLAGS(msg)	COMPAT_MSG(msg, msg_flags)
 
+struct used_address {
+	struct sockaddr_storage name;
+	unsigned int name_len;
+};
+
 static int __sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
-			 struct msghdr *msg_sys, unsigned flags, int nosec)
+			 struct msghdr *msg_sys, unsigned flags,
+			 struct used_address *used_address)
 {
 	struct compat_msghdr __user *msg_compat =
 	    (struct compat_msghdr __user *)msg;
@@ -1953,8 +1960,28 @@ static int __sys_sendmsg(struct socket *
 
 	if (sock->file->f_flags & O_NONBLOCK)
 		msg_sys->msg_flags |= MSG_DONTWAIT;
-	err = (nosec ? sock_sendmsg_nosec : sock_sendmsg)(sock, msg_sys,
-							  total_len);
+	/*
+	 * If this is sendmmsg() and current destination address is same as
+	 * previously succeeded address, omit asking LSM's decision.
+	 * used_address->name_len is initialized to UINT_MAX so that the first
+	 * destination address never matches.
+	 */
+	if (used_address && used_address->name_len == msg_sys->msg_namelen &&
+	    !memcmp(&used_address->name, msg->msg_name,
+		    used_address->name_len)) {
+		err = sock_sendmsg_nosec(sock, msg_sys, total_len);
+		goto out_freectl;
+	}
+	err = sock_sendmsg(sock, msg_sys, total_len);
+	/*
+	 * If this is sendmmsg() and sending to current destination address was
+	 * successful, remember it.
+	 */
+	if (used_address && err >= 0) {
+		used_address->name_len = msg_sys->msg_namelen;
+		memcpy(&used_address->name, msg->msg_name,
+		       used_address->name_len);
+	}
 
 out_freectl:
 	if (ctl_buf != ctl)
@@ -1979,7 +2006,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct
 	if (!sock)
 		goto out;
 
-	err = __sys_sendmsg(sock, msg, &msg_sys, flags, 0);
+	err = __sys_sendmsg(sock, msg, &msg_sys, flags, NULL);
 
 	fput_light(sock->file, fput_needed);
 out:
@@ -1998,6 +2025,7 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
+	struct used_address used_address;
 
 	datagrams = 0;
 
@@ -2009,23 +2037,21 @@ int __sys_sendmmsg(int fd, struct mmsghd
 	if (err)
 		goto out_put;
 
+	used_address.name_len = UINT_MAX;
 	entry = mmsg;
 	compat_entry = (struct compat_mmsghdr __user *)mmsg;
 
 	while (datagrams < vlen) {
-		/*
-		 * No need to ask LSM for more than the first datagram.
-		 */
 		if (MSG_CMSG_COMPAT & flags) {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)compat_entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, &used_address);
 			if (err < 0)
 				break;
 			err = __put_user(err, &compat_entry->msg_len);
 			++compat_entry;
 		} else {
 			err = __sys_sendmsg(sock, (struct msghdr __user *)entry,
-					    &msg_sys, flags, datagrams);
+					    &msg_sys, flags, &used_address);
 			if (err < 0)
 				break;
 			err = put_user(err, &entry->msg_len);
@@ -2040,27 +2066,14 @@ int __sys_sendmmsg(int fd, struct mmsghd
 out_put:
 	fput_light(sock->file, fput_needed);
 
-	if (err == 0)
-		return datagrams;
-
-	if (datagrams != 0) {
-		/*
-		 * We may send less entries than requested (vlen) if the
-		 * sock is non blocking...
-		 */
-		if (err != -EAGAIN) {
-			/*
-			 * ... or if sendmsg returns an error after we
-			 * send some datagrams, where we record the
-			 * error to return on the next call or if the
-			 * app asks about it using getsockopt(SO_ERROR).
-			 */
-			sock->sk->sk_err = -err;
-		}
-
+	/*
+	 * We may send less entries than requested (vlen), but we ignore errors
+	 * if one or more entries were sent successfully.
+	 */
+	if (datagrams)
 		return datagrams;
-	}
 
+	/* Report errors only if no entry was sent. */
 	return err;
 }
 

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 03 Aug 2011 12:25:51 +0900

> Tetsuo Handa wrote:
>> I see. Here is an updated version.
> Oops, seems whitespace damaged. Resending.
> 
> Also, attaching kmalloc()-free version. If performance loss by kmalloc()-free
> version is small enough, can it be a candidate?

Anton, _PLEASE_ look into this and do some testing or at least give
some kind of feedback.

This regression came from your change so I really need you to be
active in the resolution of this regression.

Thanks.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi Dave,

> Anton, _PLEASE_ look into this and do some testing or at least give
> some kind of feedback.
> 
> This regression came from your change so I really need you to be
> active in the resolution of this regression.

Sorry, was travelling but looking at it now.

Anton 

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 2 Aug 2011 20:52:05 +0900
> 
> > David Miller wrote:
> >> Actually, I change my mind. :-)
> >> 
> >> I think sendmmsg() needs to unconditionally not report an error if any
> >> datagrams were sent successfully.
> > 
> > What about adding
> 
> I much prefer to make the error handling more correct, rather than
> making sendmmsg() have fundamentally different semantics depending
> upon the underlying LSM.
> 

Well, the way how sendmmsg() returns error code is tricky. But recvmmsg() has
been doing in this way for a while. So, for symmetry reason, maybe sendmmsg()
should do as with recvmmsg() since it is too late to change recvmmsg()'s way.

So, programmers should be warned (in the man pages) that they should always
call getsockopt(SO_ERROR) (in order to clear the error code) if sendmmsg() or
recvmmsg() returned less than requested.

It is inevitable that sendmmsg() behaves differently depending upon the
underlying LSM. If the underlying LSM is SMACK (or TOMOYO), sendmmsg() might
return before all datagrams are sent due to security_socket_sendmsg() returning
an error at the middle of the datagram array. In that case, calling senddmsg()
again (after clearing error code using getsockopt(SO_ERROR)) from (previous'
sendmmsg()'s return value)th element of the datagram array will likely fail
because security_socket_sendmsg() will return the same error again. Thus,
programmers should be warned that they must not expect that resuming sendmmsg()
 from (previous' sendmmsg()'s return value)th element will eventually succeed.

By the way, don't we want integer overflow check and/or cond_resched() here?
I don't know whether there is an arch where userspace can allocate
(1 << BITS_PER_INT) * sizeof(struct msghdr) bytes using malloc() and kernel can
allocate huge memory for the socket buffer.

#include <stdio.h>
int main(int argc, char *argv[])
{
        int datagrams = 0;
        unsigned int vlen = 4294967290U;
        while (datagrams < vlen)
                datagrams++;
        printf("%u\n", datagrams);
        return 0;
}

I think this program (on x86_32) will print an IS_ERR() value upon success.

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi,

> > I much prefer to make the error handling more correct, rather than
> > making sendmmsg() have fundamentally different semantics depending
> > upon the underlying LSM.
> 
> Well, the way how sendmmsg() returns error code is tricky. But
> recvmmsg() has been doing in this way for a while. So, for symmetry
> reason, maybe sendmmsg() should do as with recvmmsg() since it is too
> late to change recvmmsg()'s way.
> 
> So, programmers should be warned (in the man pages) that they should
> always call getsockopt(SO_ERROR) (in order to clear the error code)
> if sendmmsg() or recvmmsg() returned less than requested.

As you suggest, I wanted to mirror how recvmmsg returns errors. But I
now agree with Dave, we should not return an error if we managed to send
any datagrams.

Perhaps we need to modify recvmmsg to do the same?

> By the way, don't we want integer overflow check and/or
> cond_resched() here? I don't know whether there is an arch where
> userspace can allocate (1 << BITS_PER_INT) * sizeof(struct msghdr)
> bytes using malloc() and kernel can allocate huge memory for the
> socket buffer.
> 
> #include <stdio.h>
> int main(int argc, char *argv[])
> {
>         int datagrams = 0;
>         unsigned int vlen = 4294967290U;
>         while (datagrams < vlen)
>                 datagrams++;
>         printf("%u\n", datagrams);
>         return 0;
> }
> 
> I think this program (on x86_32) will print an IS_ERR() value upon
> success.

Good catch. I wonder if we can do something similar to read/write where
we just truncate the length. What value should we use? One option is to
reuse UIO_MAXIOV (1024).

The following patch is compiled tested only so far.

Anton
--

[PATCH] net: Cap number of elements for recvmmsg and sendmmsg 

To limit the amount of time we can spend in recvmmsg and sendmmsg,
cap the number of elements to UIO_MAXIOV (currently 1024). 
       
Signed-off-by: Anton Blanchard <anton@samba.org>
Cc: <stable@kernel.org>
---

diff --git a/net/socket.c b/net/socket.c
index b1cbbcd..ad345b1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1999,6 +1999,9 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 	struct compat_mmsghdr __user *compat_entry;
 	struct msghdr msg_sys;
 
+	if (vlen > UIO_MAXIOV)
+		vlen = UIO_MAXIOV;
+
 	datagrams = 0;
 
 	sock = sockfd_lookup_light(fd, &err, &fput_needed);
@@ -2199,6 +2202,9 @@ int __sys_recvmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 	struct msghdr msg_sys;
 	struct timespec end_time;
 
+	if (vlen > UIO_MAXIOV)
+		vlen = UIO_MAXIOV;
+
 	if (timeout &&
 	    poll_select_set_timeout(&end_time, timeout->tv_sec,
 				    timeout->tv_nsec))

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Eduard Sinelnikov]

Hi,

The scenario:
The scenario is:
* Create a bond with 3 interfaces (connect them to switch).
* Change bond's mode to active/backup.
* Physicly remove two cables form interfaces ( not the active interface ).
* Put the cables back
* Change the mode to round robin.
* Try to ping some other computer.

Now only one interface is pinging to remote computer.
Without removing the cables all three interface will ping to remote
computer periodicly.



The problem:
In the kernel 2.6.39.3 ( /drivers/net/bond/bond_main.c).
In the function  ‘bond_xmit_roundrobin’
The code check if the bond is active via
‘bond_is_active_slave(slave)’ Function call.
Which actually checks if the slave is backup or active
What is the meaning of slave being  backup in round robin mode?
Correct me if I wrong but in round robin every slave should send a
packet, regardless of being active or backup.

Thank you,
           Eduard

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Tetsuo Handa]

Anton Blanchard wrote:
> [PATCH] net: Cap number of elements for recvmmsg and sendmmsg
> 
> To limit the amount of time we can spend in recvmmsg and sendmmsg,
> cap the number of elements to UIO_MAXIOV (currently 1024).

Looks reasonable value. But it will return less than requested without setting
error code. Programmers would needlessly call getsockopt(SO_ERROR) and get 0.
Maybe -EINVAL or something is better than returning less than requested?

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi,

> > [PATCH] net: Cap number of elements for recvmmsg and sendmmsg
> > 
> > To limit the amount of time we can spend in recvmmsg and sendmmsg,
> > cap the number of elements to UIO_MAXIOV (currently 1024).
> 
> Looks reasonable value. But it will return less than requested
> without setting error code. Programmers would needlessly call
> getsockopt(SO_ERROR) and get 0. Maybe -EINVAL or something is better
> than returning less than requested?

Having to call getsockopt(SO_ERROR) at all is confusing. We should
change sendmmsg to only return an error if no messages are sent. If we
do this, then the application will have already have to handle the case
where we send less messages than requested.

Returning EINVAL adds complexity to the system call that I think we
should avoid. Will send out the patches for review after some sleep.

Anton

Re: [PATCH] net: Fix security_socket_sendmsg() bypass problem.

[Anton Blanchard]

Hi,

> Also, attaching kmalloc()-free version. If performance loss by
> kmalloc()-free version is small enough, can it be a candidate?

Thanks. Running some benchmarks across both versions, will have some
numbers later on today.

Anton

Re: Question regarding sendmmsg().

[Paul Moore]

On Friday, July 22, 2011 7:41:20 AM Tetsuo Handa wrote:
> TOMOYO was about to add support for permission checks for
> PF_INET/PF_INET6/PF_UNIX socket's bind()/listen()/connect()/send()
> operations (
> http://www.spinics.net/linux/fedora/linux-security-module/msg11496.html ).
> 
> According to http://ozlabs.org/~anton/junkcode/sendmmsg_test.c , the
> sendmmsg() introduced by commit 228e548e "net: Add sendmmsg socket system
> call" is capable of sending to multiple different destinations with single
> sendmmsg(), isn't it?

I believe so, yes.

> If yes, my plan (restricting sendmsg() based on destination's address)
> became impossible since security_socket_sendmsg() (which receives the
> destination's address) is called for only once even if there are multiple
> different destinations.

We could always change this behavior so that the sendmsg() LSM hook is called 
for each msg sent, but there would be a performance impact associated with it. 

We decided that it was unnecessary to do it this way earlier because there was 
no need: SELinux and Smack both treat the socket as an endpoint (from a 
implementation point of view only, from a high level design Smack doesn't care 
about sockets) and AppArmor really doesn't have much in the way of network 
access controls at present.

--
paul moore
linux @ hp