Message-Id: <200711181300.III52155.FtFQJOLSMOHOVF@I-love.SAKURA.ne.jp>
Date:	Sun, 18 Nov 2007 13:00:20 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	paul.moore@...com
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org, takedakn@...data.co.jp
Subject: Re: [TOMOYO #5 18/18] LSM expansion for TOMOYO Linux.

Hello.

Paul Moore wrote:
> Okay, well if that is the case I think you are going to have another problem 
> in that you could end up throwing away skbs that haven't been through your 
> security_post_recv_datagram() hook because you _always_ throw away the result 
> of the second skb_peek().  Once again, if I'm wrong please correct me.
I didn't understand what's wrong with throwing away the result of
the second skb_peek(). I'm doing similar things that udp_recvmsg() is doing.

| int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
|                 size_t len, int noblock, int flags, int *addr_len)
| {
<snipped>
| try_again:
|         skb = skb_recv_datagram(sk, flags, noblock, &err);
|         if (!skb)
|                 goto out;
<snipped>
| out_free:
|         skb_free_datagram(sk, skb);
| out:
|         return err;
| 
| csum_copy_err:
|         UDP_INC_STATS_BH(UDP_MIB_INERRORS, is_udplite);
| 
|         skb_kill_datagram(sk, skb, flags);
| 
|         if (noblock)
|                 return -EAGAIN;
|         goto try_again;
| }

The only difference is that I'm not using skb_kill_datagram()
because skb_kill_datagram() uses spin_lock_bh()
while skb_recv_datagram() needs to use spin_lock_irqsave().


> Where did the 'if (skb) return skb;' code go?  Don't you need to do you LSM 
> call before you return the skb?
Sorry, I should have explicitly inserted <snipped> rather than a blank line like:

|		error = security_post_recv_datagram(sk, skb, flags);
|		if (error)
|                       goto force_dequeue;
<snipped>
|         } while (!wait_for_packet(sk, err, &timeo));
|
|         return NULL;
| force_dequeue:
|         /* dequeue if MSG_PEEK is set. */
| no_packet:
|         *err = error;
|         return NULL;

The below is the updated patch.
Regards.
-----
Subject: LSM expansion for TOMOYO Linux.

LSM hooks for sending signal:
   * task_kill_unlocked is added in sys_kill
   * task_tkill_unlocked is added in sys_tkill
   * task_tgkill_unlocked is added in sys_tgkill
LSM hooks for network accept and recv:
   * socket_post_accept is modified to return int.
   * post_recv_datagram is added in skb_recv_datagram.

You can try TOMOYO Linux without this patch, but in that case, you
can't use access control functionality for restricting signal
transmission and incoming network data.

Signed-off-by: Kentaro Takeda <takedakn@...data.co.jp>
Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
 include/linux/security.h |   74 +++++++++++++++++++++++++++++++++++++++++++----
 kernel/signal.c          |   17 ++++++++++
 net/core/datagram.c      |   29 ++++++++++++++++--
 net/socket.c             |    7 +++-
 security/dummy.c         |   32 ++++++++++++++++++--
 security/security.c      |   25 ++++++++++++++-
 6 files changed, 169 insertions(+), 15 deletions(-)

--- linux-2.6.23.orig/include/linux/security.h	2007-11-17 00:35:44.000000000 +0900
+++ linux-2.6.23/include/linux/security.h	2007-11-17 00:37:26.000000000 +0900
@@ -657,6 +657,25 @@ struct request_sock;
  *	@sig contains the signal value.
  *	@secid contains the sid of the process where the signal originated
  *	Return 0 if permission is granted.
+ * @task_kill_unlocked:
+ *	Check permission before sending signal @sig to the process of @pid
+ *	with sys_kill.
+ *	@pid contains the pid of target process.
+ *	@sig contains the signal value.
+ *	Return 0 if permission is granted.
+ * @task_tkill_unlocked:
+ *	Check permission before sending signal @sig to the process of @pid
+ *	with sys_tkill.
+ *	@pid contains the pid of target process.
+ *	@sig contains the signal value.
+ *	Return 0 if permission is granted.
+ * @task_tgkill_unlocked:
+ *	Check permission before sending signal @sig to the process of @pid
+ *	with sys_tgkill.
+ *	@tgid contains the thread group id.
+ *	@pid contains the pid of target process.
+ *	@sig contains the signal value.
+ *	Return 0 if permission is granted.
  * @task_wait:
  *	Check permission before allowing a process to reap a child process @p
  *	and collect its status information.
@@ -778,8 +797,12 @@ struct request_sock;
  * @socket_post_accept:
  *	This hook allows a security module to copy security
  *	information into the newly created socket's inode.
+ *	This hook also allows a security module to filter connections
+ *	from unwanted peers.
+ *	The connection will be aborted if this hook returns nonzero.
  *	@sock contains the listening socket structure.
  *	@newsock contains the newly created server socket for connection.
+ *	Return 0 if permission is granted.
  * @socket_sendmsg:
  *	Check permission before transmitting a message to another socket.
  *	@sock contains the socket structure.
@@ -793,6 +816,12 @@ struct request_sock;
  *	@size contains the size of message structure.
  *	@flags contains the operational flags.
  *	Return 0 if permission is granted.  
+ * @post_recv_datagram:
+ *	Check permission after receiving a datagram.
+ *	@sk contains the socket.
+ *	@skb contains the socket buffer (may be NULL).
+ *	@flags contains the operational flags.
+ *	Return 0 if permission is granted.
  * @socket_getsockname:
  *	Check permission before the local address (name) of the socket object
  *	@sock is retrieved.
@@ -1319,6 +1348,9 @@ struct security_operations {
 	int (*task_movememory) (struct task_struct * p);
 	int (*task_kill) (struct task_struct * p,
 			  struct siginfo * info, int sig, u32 secid);
+	int (*task_kill_unlocked) (int pid, int sig);
+	int (*task_tkill_unlocked) (int pid, int sig);
+	int (*task_tgkill_unlocked) (int tgid, int pid, int sig);
 	int (*task_wait) (struct task_struct * p);
 	int (*task_prctl) (int option, unsigned long arg2,
 			   unsigned long arg3, unsigned long arg4,
@@ -1384,12 +1416,16 @@ 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);
-	void (*socket_post_accept) (struct socket * sock,
-				    struct socket * newsock);
+#define TMY_LSM_EXPANSION
+	int (*socket_post_accept) (struct socket *sock,
+				   struct socket *newsock);
 	int (*socket_sendmsg) (struct socket * sock,
 			       struct msghdr * msg, int size);
 	int (*socket_recvmsg) (struct socket * sock,
 			       struct msghdr * msg, int size, int flags);
+	int (*post_recv_datagram) (struct sock *sk,
+				   struct sk_buff *skb,
+				   unsigned int flags);
 	int (*socket_getsockname) (struct socket * sock);
 	int (*socket_getpeername) (struct socket * sock);
 	int (*socket_getsockopt) (struct socket * sock, int level, int optname);
@@ -1567,6 +1603,9 @@ int security_task_getscheduler(struct ta
 int security_task_movememory(struct task_struct *p);
 int security_task_kill(struct task_struct *p, struct siginfo *info,
 			int sig, u32 secid);
+int security_task_kill_unlocked(int pid, int sig);
+int security_task_tkill_unlocked(int pid, int sig);
+int security_task_tgkill_unlocked(int tgid, int pid, int sig);
 int security_task_wait(struct task_struct *p);
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			 unsigned long arg4, unsigned long arg5);
@@ -2112,6 +2151,21 @@ static inline int security_task_kill (st
 	return cap_task_kill(p, info, sig, secid);
 }
 
+static inline int security_task_kill_unlocked(int pid, int sig)
+{
+	return 0;
+}
+
+static inline int security_task_tkill_unlocked(int pid, int sig)
+{
+	return 0;
+}
+
+static inline int security_task_tgkill_unlocked(int tgid, int pid, int sig)
+{
+	return 0;
+}
+
 static inline int security_task_wait (struct task_struct *p)
 {
 	return 0;
@@ -2294,10 +2348,12 @@ 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);
-void security_socket_post_accept(struct socket *sock, struct socket *newsock);
+int security_socket_post_accept(struct socket *sock, struct socket *newsock);
 int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
+int security_post_recv_datagram(struct sock *sk, struct sk_buff *skb,
+				unsigned int flags);
 int security_socket_getsockname(struct socket *sock);
 int security_socket_getpeername(struct socket *sock);
 int security_socket_getsockopt(struct socket *sock, int level, int optname);
@@ -2373,9 +2429,10 @@ static inline int security_socket_accept
 	return 0;
 }
 
-static inline void security_socket_post_accept(struct socket * sock, 
-					       struct socket * newsock)
+static inline int security_socket_post_accept(struct socket *sock,
+					      struct socket *newsock)
 {
+	return 0;
 }
 
 static inline int security_socket_sendmsg(struct socket * sock, 
@@ -2391,6 +2448,13 @@ static inline int security_socket_recvms
 	return 0;
 }
 
+static inline int security_post_recv_datagram(struct sock *sk,
+					      struct sk_buff *skb,
+					      unsigned int flags)
+{
+	return 0;
+}
+
 static inline int security_socket_getsockname(struct socket * sock)
 {
 	return 0;
--- linux-2.6.23.orig/kernel/signal.c	2007-11-17 00:35:51.000000000 +0900
+++ linux-2.6.23/kernel/signal.c	2007-11-17 00:37:26.000000000 +0900
@@ -2218,6 +2218,11 @@ asmlinkage long
 sys_kill(int pid, int sig)
 {
 	struct siginfo info;
+	int ret;
+
+	ret = security_task_kill_unlocked(pid, sig);
+	if (ret)
+		return ret;
 
 	info.si_signo = sig;
 	info.si_errno = 0;
@@ -2273,10 +2278,16 @@ static int do_tkill(int tgid, int pid, i
  */
 asmlinkage long sys_tgkill(int tgid, int pid, int sig)
 {
+	int ret;
+
 	/* This is only valid for single tasks */
 	if (pid <= 0 || tgid <= 0)
 		return -EINVAL;
 
+	ret = security_task_tgkill_unlocked(tgid, pid, sig);
+	if (ret)
+		return ret;
+
 	return do_tkill(tgid, pid, sig);
 }
 
@@ -2286,10 +2297,16 @@ asmlinkage long sys_tgkill(int tgid, int
 asmlinkage long
 sys_tkill(int pid, int sig)
 {
+	int ret;
+
 	/* This is only valid for single tasks */
 	if (pid <= 0)
 		return -EINVAL;
 
+	ret = security_task_tkill_unlocked(pid, sig);
+	if (ret)
+		return ret;
+
 	return do_tkill(0, pid, sig);
 }
 
--- linux-2.6.23.orig/net/socket.c	2007-11-17 00:35:03.000000000 +0900
+++ linux-2.6.23/net/socket.c	2007-11-17 00:37:26.000000000 +0900
@@ -1442,13 +1442,16 @@ asmlinkage long sys_accept(int fd, struc
 			goto out_fd;
 	}
 
+	/* Filter connections from unwanted peers like TCP Wrapper. */
+	err = security_socket_post_accept(sock, newsock);
+	if (err)
+		goto out_fd;
+
 	/* File flags are not inherited via accept() unlike another OSes. */
 
 	fd_install(newfd, newfile);
 	err = newfd;
 
-	security_socket_post_accept(sock, newsock);
-
 out_put:
 	fput_light(sock->file, fput_needed);
 out:
--- linux-2.6.23.orig/security/dummy.c	2007-11-17 00:35:44.000000000 +0900
+++ linux-2.6.23/security/dummy.c	2007-11-17 00:37:26.000000000 +0900
@@ -576,6 +576,21 @@ static int dummy_task_kill (struct task_
 	return 0;
 }
 
+static int dummy_task_kill_unlocked(int pid, int sig)
+{
+	return 0;
+}
+
+static int dummy_task_tkill_unlocked(int pid, int sig)
+{
+	return 0;
+}
+
+static int dummy_task_tgkill_unlocked(int tgid, int pid, int sig)
+{
+	return 0;
+}
+
 static int dummy_task_prctl (int option, unsigned long arg2, unsigned long arg3,
 			     unsigned long arg4, unsigned long arg5)
 {
@@ -753,10 +768,10 @@ static int dummy_socket_accept (struct s
 	return 0;
 }
 
-static void dummy_socket_post_accept (struct socket *sock, 
-				      struct socket *newsock)
+static int dummy_socket_post_accept(struct socket *sock,
+				    struct socket *newsock)
 {
-	return;
+	return 0;
 }
 
 static int dummy_socket_sendmsg (struct socket *sock, struct msghdr *msg,
@@ -771,6 +786,13 @@ static int dummy_socket_recvmsg (struct 
 	return 0;
 }
 
+static int dummy_post_recv_datagram(struct sock *sk,
+				    struct sk_buff *skb,
+				    unsigned int flags)
+{
+	return 0;
+}
+
 static int dummy_socket_getsockname (struct socket *sock)
 {
 	return 0;
@@ -1062,6 +1084,9 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, task_movememory);
 	set_to_dummy_if_null(ops, task_wait);
 	set_to_dummy_if_null(ops, task_kill);
+	set_to_dummy_if_null(ops, task_kill_unlocked);
+	set_to_dummy_if_null(ops, task_tkill_unlocked);
+	set_to_dummy_if_null(ops, task_tgkill_unlocked);
 	set_to_dummy_if_null(ops, task_prctl);
 	set_to_dummy_if_null(ops, task_reparent_to_init);
  	set_to_dummy_if_null(ops, task_to_inode);
@@ -1104,6 +1129,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, socket_post_accept);
 	set_to_dummy_if_null(ops, socket_sendmsg);
 	set_to_dummy_if_null(ops, socket_recvmsg);
+	set_to_dummy_if_null(ops, post_recv_datagram);
 	set_to_dummy_if_null(ops, socket_getsockname);
 	set_to_dummy_if_null(ops, socket_getpeername);
 	set_to_dummy_if_null(ops, socket_setsockopt);
--- linux-2.6.23.orig/net/core/datagram.c	2007-10-10 05:31:38.000000000 +0900
+++ linux-2.6.23/net/core/datagram.c	2007-11-18 12:17:59.000000000 +0900
@@ -55,6 +55,7 @@
 #include <net/checksum.h>
 #include <net/sock.h>
 #include <net/tcp_states.h>
+#include <linux/security.h>
 
 /*
  *	Is a socket 'connection oriented' ?
@@ -148,6 +149,7 @@ struct sk_buff *skb_recv_datagram(struct
 {
 	struct sk_buff *skb;
 	long timeo;
+	unsigned long cpu_flags;
 	/*
 	 * Caller is allowed not to check sk->sk_err before skb_recv_datagram()
 	 */
@@ -166,8 +168,6 @@ struct sk_buff *skb_recv_datagram(struct
 		 * However, this function was corrent in any case. 8)
 		 */
 		if (flags & MSG_PEEK) {
-			unsigned long cpu_flags;
-
 			spin_lock_irqsave(&sk->sk_receive_queue.lock,
 					  cpu_flags);
 			skb = skb_peek(&sk->sk_receive_queue);
@@ -178,6 +178,10 @@ struct sk_buff *skb_recv_datagram(struct
 		} else
 			skb = skb_dequeue(&sk->sk_receive_queue);
 
+		error = security_post_recv_datagram(sk, skb, flags);
+		if (error)
+			goto force_dequeue;
+
 		if (skb)
 			return skb;
 
@@ -189,7 +193,26 @@ struct sk_buff *skb_recv_datagram(struct
 	} while (!wait_for_packet(sk, err, &timeo));
 
 	return NULL;
-
+force_dequeue:
+	/* Drop this packet because LSM says "Don't pass it to the caller". */
+	if (!(flags & MSG_PEEK))
+		goto no_peek;
+	/*
+	 * If this packet is MSG_PEEK'ed, dequeue it forcibly
+	 * so that this packet won't prevent the caller from picking up
+	 * next packet.
+	 * Side effect: If this socket is shared by multiple processes
+	 * who have differnt policy, the process who is permitted to pick up
+	 * this packet can't pick up this packet.
+	 */
+	spin_lock_irqsave(&sk->sk_receive_queue.lock, cpu_flags);
+	if (skb == skb_peek(&sk->sk_receive_queue)) {
+		__skb_unlink(skb, &sk->sk_receive_queue);
+		atomic_dec(&skb->users);
+	}
+	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, cpu_flags);
+no_peek:
+	kfree_skb(skb);
 no_packet:
 	*err = error;
 	return NULL;
--- linux-2.6.23.orig/security/security.c	2007-11-17 00:35:44.000000000 +0900
+++ linux-2.6.23/security/security.c	2007-11-17 00:37:26.000000000 +0900
@@ -662,6 +662,21 @@ int security_task_kill(struct task_struc
 	return security_ops->task_kill(p, info, sig, secid);
 }
 
+int security_task_kill_unlocked(int pid, int sig)
+{
+	return security_ops->task_kill_unlocked(pid, sig);
+}
+
+int security_task_tkill_unlocked(int pid, int sig)
+{
+	return security_ops->task_tkill_unlocked(pid, sig);
+}
+
+int security_task_tgkill_unlocked(int tgid, int pid, int sig)
+{
+	return security_ops->task_tgkill_unlocked(tgid, pid, sig);
+}
+
 int security_task_wait(struct task_struct *p)
 {
 	return security_ops->task_wait(p);
@@ -869,9 +884,9 @@ int security_socket_accept(struct socket
 	return security_ops->socket_accept(sock, newsock);
 }
 
-void security_socket_post_accept(struct socket *sock, struct socket *newsock)
+int security_socket_post_accept(struct socket *sock, struct socket *newsock)
 {
-	security_ops->socket_post_accept(sock, newsock);
+	return security_ops->socket_post_accept(sock, newsock);
 }
 
 int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
@@ -885,6 +900,12 @@ int security_socket_recvmsg(struct socke
 	return security_ops->socket_recvmsg(sock, msg, size, flags);
 }
 
+int security_post_recv_datagram(struct sock *sk, struct sk_buff *skb,
+				unsigned int flags)
+{
+	return security_ops->post_recv_datagram(sk, skb, flags);
+}
+
 int security_socket_getsockname(struct socket *sock)
 {
 	return security_ops->socket_getsockname(sock);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists