[RFC 0/9] snet: Security for NETwork syscalls

[RFC 0/9] snet: Security for NETwork syscalls

[Samir Bellabes]

Hello lsm and netdev people,
I would like to submit as a RFC this linux security module.

snet provides a mecanism to defer syscall security hooks and decision (verdict)
to userspace.

I believe that snet will help to get over the classical configuration
complexity of others security modules, by providing interactivity to users.
I also think that monolithic strategy is broken with snet, as we can provide
security for others syscall's categories:
 - sfs  : security for filesystem,
 - stask: security for task,
 - smem : security for memory,

In this way, and by putting abstraction on how this subsystems can talk to each
others, we may use the security combinaison we want: choose to run sfs,
stask, but not snet nor smem. Better, developpers may investigated how to build
another security subsystem for tasks, and use others existing (smem, snet..)
which they don't want to modify

I think that interactivity is very usefull for users, as they may be notify when
something is wrong and take decision, and from userspace, the decision may be
defered to another box. In this way, snet also have a advantage for mobile
devices as the policy decision will be push to a distant server, mobile device
will then wait for verdicts and as policy strategies are centralized.

snet has some subsystems :
 - core to init and exit the system
 - kernel/user communications (genetlink)
 - hashtable for events and verdict, and managing functions.
 - LSM hooks, classical security operations

Finally, and a important point: snet integration respects the LSM framework idea
of using LSM hooks

roadmap:
 * Building a cache for each task_struct using pointer (void*) security
   use the pointer (void*) security related to task_security to provides a
   verdict cache: if two identical requests are coming, ask the user for the
   first one, store the result in the cached and for the second request, just
   look in the cache
 * send data buffer of recvmsg and sendmsg to userspace
   this may provide a way to look inside the data (as a anti-virus do)
 * adding other security systems
   we can think about adding fork(), exec(), open(), close()..
   
I'm Ccing netfilter-devel, as snet may be see as a way to do filtering.

Samir Bellabes (9):
  lsm: add security_socket_closed()
  Revert "lsm: Remove the socket_post_accept() hook"
  snet: introduce security/snet, Makefile and Kconfig changes
  snet: introduce snet_core.c and snet.h
  snet: introduce snet_event.c and snet_event.h
  snet: introduce snet_hooks.c and snet_hook.h
  snet: introduce snet_netlink.c and snet_netlink.h
  snet: introduce snet_verdict.c and snet_verdict.h
  snet: introduce snet_utils.c and snet_utils.h

 include/linux/security.h             |   23 ++
 net/socket.c                         |    3 +
 security/Kconfig                     |    1 +
 security/Makefile                    |    2 +
 security/capability.c                |   10 +
 security/security.c                  |   10 +
 security/snet/Kconfig                |   22 ++
 security/snet/Makefile               |   13 +
 security/snet/include/snet.h         |   29 ++
 security/snet/include/snet_event.h   |   20 +
 security/snet/include/snet_hooks.h   |   28 ++
 security/snet/include/snet_netlink.h |  201 ++++++++++
 security/snet/include/snet_utils.h   |    9 +
 security/snet/include/snet_verdict.h |   33 ++
 security/snet/snet_core.c            |   77 ++++
 security/snet/snet_event.c           |  229 +++++++++++
 security/snet/snet_hooks.c           |  686 ++++++++++++++++++++++++++++++++++
 security/snet/snet_netlink.c         |  541 +++++++++++++++++++++++++++
 security/snet/snet_utils.c           |   40 ++
 security/snet/snet_verdict.c         |  210 +++++++++++
 20 files changed, 2187 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/Kconfig
 create mode 100644 security/snet/Makefile
 create mode 100644 security/snet/include/snet.h
 create mode 100644 security/snet/include/snet_event.h
 create mode 100644 security/snet/include/snet_hooks.h
 create mode 100644 security/snet/include/snet_netlink.h
 create mode 100644 security/snet/include/snet_utils.h
 create mode 100644 security/snet/include/snet_verdict.h
 create mode 100644 security/snet/snet_core.c
 create mode 100644 security/snet/snet_event.c
 create mode 100644 security/snet/snet_hooks.c
 create mode 100644 security/snet/snet_netlink.c
 create mode 100644 security/snet/snet_utils.c
 create mode 100644 security/snet/snet_verdict.c

[RFC 1/9] lsm: add security_socket_closed()

[Samir Bellabes]

Allow a module to update security informations when a socket is closed.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 include/linux/security.h |   10 ++++++++++
 net/socket.c             |    1 +
 security/capability.c    |    5 +++++
 security/security.c      |    5 +++++
 4 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 466cbad..275dd04 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -974,6 +974,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@sock contains the socket structure.
  *	@how contains the flag indicating how future sends and receives are handled.
  *	Return 0 if permission is granted.
+ * @socket_close:
+ *	Allow a module to update security informations when a socket is closed
+ *	@sock is closed.
  * @socket_sock_rcv_skb:
  *	Check permissions on incoming network packets.  This hook is distinct
  *	from Netfilter's IP input hooks since it is the first time that the
@@ -1673,6 +1676,7 @@ struct security_operations {
 	int (*socket_getsockopt) (struct socket *sock, int level, int optname);
 	int (*socket_setsockopt) (struct socket *sock, int level, int optname);
 	int (*socket_shutdown) (struct socket *sock, int how);
+	void (*socket_close) (struct socket *sock);
 	int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
 	int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
 	int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
@@ -2693,6 +2697,7 @@ int security_socket_getpeername(struct socket *sock);
 int security_socket_getsockopt(struct socket *sock, int level, int optname);
 int security_socket_setsockopt(struct socket *sock, int level, int optname);
 int security_socket_shutdown(struct socket *sock, int how);
+void security_socket_close(struct socket *sock);
 int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
 int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
 				      int __user *optlen, unsigned len);
@@ -2805,6 +2810,11 @@ static inline int security_socket_shutdown(struct socket *sock, int how)
 {
 	return 0;
 }
+
+static inline void security_socket_close(struct socket *sock)
+{
+}
+
 static inline int security_sock_rcv_skb(struct sock *sk,
 					struct sk_buff *skb)
 {
diff --git a/net/socket.c b/net/socket.c
index dbfdfa9..8984973 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1074,6 +1074,7 @@ static int sock_close(struct inode *inode, struct file *filp)
 		printk(KERN_DEBUG "sock_close: NULL inode\n");
 		return 0;
 	}
+	security_socket_close(SOCKET_I(inode));
 	sock_release(SOCKET_I(inode));
 	return 0;
 }
diff --git a/security/capability.c b/security/capability.c
index 5c700e1..a9810dc 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -677,6 +677,10 @@ static int cap_socket_shutdown(struct socket *sock, int how)
 	return 0;
 }
 
+static void cap_socket_close(struct socket *sock)
+{
+}
+
 static int cap_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	return 0;
@@ -1084,6 +1088,7 @@ void security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, socket_setsockopt);
 	set_to_cap_if_null(ops, socket_getsockopt);
 	set_to_cap_if_null(ops, socket_shutdown);
+	set_to_cap_if_null(ops, socket_close);
 	set_to_cap_if_null(ops, socket_sock_rcv_skb);
 	set_to_cap_if_null(ops, socket_getpeersec_stream);
 	set_to_cap_if_null(ops, socket_getpeersec_dgram);
diff --git a/security/security.c b/security/security.c
index 24e060b..7457ed5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1120,6 +1120,11 @@ int security_socket_shutdown(struct socket *sock, int how)
 	return security_ops->socket_shutdown(sock, how);
 }
 
+void security_socket_close(struct socket *sock)
+{
+	return security_ops->socket_close(sock);
+}
+
 int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	return security_ops->socket_sock_rcv_skb(sk, skb);
-- 
1.6.3.3

Re: [RFC 1/9] lsm: add security_socket_closed()

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> Allow a module to update security informations when a socket is closed.
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---
>  include/linux/security.h |   10 ++++++++++
>  net/socket.c             |    1 +
>  security/capability.c    |    5 +++++
>  security/security.c      |    5 +++++
>  4 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 466cbad..275dd04 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -974,6 +974,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	@sock contains the socket structure.
>   *	@how contains the flag indicating how future sends and receives are handled.
>   *	Return 0 if permission is granted.
> + * @socket_close:
> + *	Allow a module to update security informations when a socket is closed
> + *	@sock is closed.
>   * @socket_sock_rcv_skb:
>   *	Check permissions on incoming network packets.  This hook is distinct
>   *	from Netfilter's IP input hooks since it is the first time that the
> @@ -1673,6 +1676,7 @@ struct security_operations {
>  	int (*socket_getsockopt) (struct socket *sock, int level, int optname);
>  	int (*socket_setsockopt) (struct socket *sock, int level, int optname);
>  	int (*socket_shutdown) (struct socket *sock, int how);
> +	void (*socket_close) (struct socket *sock);
>  	int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
>  	int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
>  	int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
> @@ -2693,6 +2697,7 @@ int security_socket_getpeername(struct socket *sock);
>  int security_socket_getsockopt(struct socket *sock, int level, int optname);
>  int security_socket_setsockopt(struct socket *sock, int level, int optname);
>  int security_socket_shutdown(struct socket *sock, int how);
> +void security_socket_close(struct socket *sock);
>  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
>  int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
>  				      int __user *optlen, unsigned len);
> @@ -2805,6 +2810,11 @@ static inline int security_socket_shutdown(struct socket *sock, int how)
>  {
>  	return 0;
>  }
> +
> +static inline void security_socket_close(struct socket *sock)
> +{
> +}
> +
>  static inline int security_sock_rcv_skb(struct sock *sk,
>  					struct sk_buff *skb)
>  {
> diff --git a/net/socket.c b/net/socket.c
> index dbfdfa9..8984973 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1074,6 +1074,7 @@ static int sock_close(struct inode *inode, struct file *filp)
>  		printk(KERN_DEBUG "sock_close: NULL inode\n");
>  		return 0;
>  	}
> +	security_socket_close(SOCKET_I(inode));

Hi,

Should this also be called at other sock_release() callers, i.e.
on error paths throughout net/socket.c?  ofr instance, I assume
sock_create() will set up whatever you want released, so if
sock_map-fd() fails, do you need to call security_socket_close()
there as well?

If so, should it just be called from sock_release()?

Or do you really intend for this only to be called when userspace
purposely releases the socket?

>  	sock_release(SOCKET_I(inode));
>  	return 0;
>  }
> diff --git a/security/capability.c b/security/capability.c
> index 5c700e1..a9810dc 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -677,6 +677,10 @@ static int cap_socket_shutdown(struct socket *sock, int how)
>  	return 0;
>  }
> 
> +static void cap_socket_close(struct socket *sock)
> +{
> +}
> +
>  static int cap_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  {
>  	return 0;
> @@ -1084,6 +1088,7 @@ void security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, socket_setsockopt);
>  	set_to_cap_if_null(ops, socket_getsockopt);
>  	set_to_cap_if_null(ops, socket_shutdown);
> +	set_to_cap_if_null(ops, socket_close);
>  	set_to_cap_if_null(ops, socket_sock_rcv_skb);
>  	set_to_cap_if_null(ops, socket_getpeersec_stream);
>  	set_to_cap_if_null(ops, socket_getpeersec_dgram);
> diff --git a/security/security.c b/security/security.c
> index 24e060b..7457ed5 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1120,6 +1120,11 @@ int security_socket_shutdown(struct socket *sock, int how)
>  	return security_ops->socket_shutdown(sock, how);
>  }
> 
> +void security_socket_close(struct socket *sock)
> +{
> +	return security_ops->socket_close(sock);
> +}
> +
>  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  {
>  	return security_ops->socket_sock_rcv_skb(sk, skb);
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/9] Revert "lsm: Remove the socket_post_accept() hook"

[Samir Bellabes]

This reverts commit 8651d5c0b1f874c5b8307ae2b858bc40f9f02482.

snet needs to reintroduce this hook, as it was designed to be: a hook for
updating security informations on objects.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 include/linux/security.h |   13 +++++++++++++
 net/socket.c             |    2 ++
 security/capability.c    |    5 +++++
 security/security.c      |    5 +++++
 4 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 275dd04..c12a286 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -931,6 +931,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@sock contains the listening socket structure.
  *	@newsock contains the newly created server socket for connection.
  *	Return 0 if permission is granted.
+ * @socket_post_accept:
+ *	This hook allows a security module to copy security
+ *	information into the newly created socket's inode.
+ *	@sock contains the listening socket structure.
+ *	@newsock contains the newly created server socket for connection.
  * @socket_sendmsg:
  *	Check permission before transmitting a message to another socket.
  *	@sock contains the socket structure.
@@ -1667,6 +1672,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);
+	void (*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,
@@ -2689,6 +2696,7 @@ int security_socket_bind(struct socket *sock, struct sockaddr *address, int addr
 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_sendmsg(struct socket *sock, struct msghdr *msg, int size);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
@@ -2771,6 +2779,11 @@ static inline int security_socket_accept(struct socket *sock,
 	return 0;
 }
 
+static inline void security_socket_post_accept(struct socket *sock,
+					       struct socket *newsock)
+{
+}
+
 static inline int security_socket_sendmsg(struct socket *sock,
 					  struct msghdr *msg, int size)
 {
diff --git a/net/socket.c b/net/socket.c
index 8984973..fcd4f2b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1557,6 +1557,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	fd_install(newfd, newfile);
 	err = newfd;
 
+	security_socket_post_accept(sock, newsock);
+
 out_put:
 	fput_light(sock->file, fput_needed);
 out:
diff --git a/security/capability.c b/security/capability.c
index a9810dc..61eae40 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -641,6 +641,10 @@ static int cap_socket_accept(struct socket *sock, struct socket *newsock)
 	return 0;
 }
 
+static void cap_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+}
+
 static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
 {
 	return 0;
@@ -1081,6 +1085,7 @@ void security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, socket_connect);
 	set_to_cap_if_null(ops, socket_listen);
 	set_to_cap_if_null(ops, socket_accept);
+	set_to_cap_if_null(ops, socket_post_accept);
 	set_to_cap_if_null(ops, socket_sendmsg);
 	set_to_cap_if_null(ops, socket_recvmsg);
 	set_to_cap_if_null(ops, socket_getsockname);
diff --git a/security/security.c b/security/security.c
index 7457ed5..20ae0b8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1084,6 +1084,11 @@ int security_socket_accept(struct socket *sock, struct socket *newsock)
 	return security_ops->socket_accept(sock, newsock);
 }
 
+void security_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+	security_ops->socket_post_accept(sock, newsock);
+}
+
 int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
 {
 	return security_ops->socket_sendmsg(sock, msg, size);
-- 
1.6.3.3

Re: [RFC 2/9] Revert "lsm: Remove the socket_post_accept() hook"

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> This reverts commit 8651d5c0b1f874c5b8307ae2b858bc40f9f02482.
> 
> snet needs to reintroduce this hook, as it was designed to be: a hook for
> updating security informations on objects.
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>

Acked-by: Serge Hallyn <serue@us.ibm.com>

(contingent of course on the proposed user actually going in :)

> ---
>  include/linux/security.h |   13 +++++++++++++
>  net/socket.c             |    2 ++
>  security/capability.c    |    5 +++++
>  security/security.c      |    5 +++++
>  4 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 275dd04..c12a286 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -931,6 +931,11 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	@sock contains the listening socket structure.
>   *	@newsock contains the newly created server socket for connection.
>   *	Return 0 if permission is granted.
> + * @socket_post_accept:
> + *	This hook allows a security module to copy security
> + *	information into the newly created socket's inode.
> + *	@sock contains the listening socket structure.
> + *	@newsock contains the newly created server socket for connection.
>   * @socket_sendmsg:
>   *	Check permission before transmitting a message to another socket.
>   *	@sock contains the socket structure.
> @@ -1667,6 +1672,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);
> +	void (*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,
> @@ -2689,6 +2696,7 @@ int security_socket_bind(struct socket *sock, struct sockaddr *address, int addr
>  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_sendmsg(struct socket *sock, struct msghdr *msg, int size);
>  int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
>  			    int size, int flags);
> @@ -2771,6 +2779,11 @@ static inline int security_socket_accept(struct socket *sock,
>  	return 0;
>  }
> 
> +static inline void security_socket_post_accept(struct socket *sock,
> +					       struct socket *newsock)
> +{
> +}
> +
>  static inline int security_socket_sendmsg(struct socket *sock,
>  					  struct msghdr *msg, int size)
>  {
> diff --git a/net/socket.c b/net/socket.c
> index 8984973..fcd4f2b 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1557,6 +1557,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
>  	fd_install(newfd, newfile);
>  	err = newfd;
> 
> +	security_socket_post_accept(sock, newsock);
> +
>  out_put:
>  	fput_light(sock->file, fput_needed);
>  out:
> diff --git a/security/capability.c b/security/capability.c
> index a9810dc..61eae40 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -641,6 +641,10 @@ static int cap_socket_accept(struct socket *sock, struct socket *newsock)
>  	return 0;
>  }
> 
> +static void cap_socket_post_accept(struct socket *sock, struct socket *newsock)
> +{
> +}
> +
>  static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
>  {
>  	return 0;
> @@ -1081,6 +1085,7 @@ void security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, socket_connect);
>  	set_to_cap_if_null(ops, socket_listen);
>  	set_to_cap_if_null(ops, socket_accept);
> +	set_to_cap_if_null(ops, socket_post_accept);
>  	set_to_cap_if_null(ops, socket_sendmsg);
>  	set_to_cap_if_null(ops, socket_recvmsg);
>  	set_to_cap_if_null(ops, socket_getsockname);
> diff --git a/security/security.c b/security/security.c
> index 7457ed5..20ae0b8 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1084,6 +1084,11 @@ int security_socket_accept(struct socket *sock, struct socket *newsock)
>  	return security_ops->socket_accept(sock, newsock);
>  }
> 
> +void security_socket_post_accept(struct socket *sock, struct socket *newsock)
> +{
> +	security_ops->socket_post_accept(sock, newsock);
> +}
> +
>  int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
>  {
>  	return security_ops->socket_sendmsg(sock, msg, size);
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/9] Revert "lsm: Remove the socket_post_accept() hook"

[Tetsuo Handa]

Serge E. Hallyn wrote:
> Quoting Samir Bellabes (sam@synack.fr):
> > This reverts commit 8651d5c0b1f874c5b8307ae2b858bc40f9f02482.
> > 
> > snet needs to reintroduce this hook, as it was designed to be: a hook for
> > updating security informations on objects.
> > 
> > Signed-off-by: Samir Bellabes <sam@synack.fr>
> 
> Acked-by: Serge Hallyn <serue@us.ibm.com>
> 
> (contingent of course on the proposed user actually going in :)
> 
> > diff --git a/net/socket.c b/net/socket.c
> > index 8984973..fcd4f2b 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1557,6 +1557,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
> >  	fd_install(newfd, newfile);
> >  	err = newfd;
> > 
> > +	security_socket_post_accept(sock, newsock);
> > +
> >  out_put:
> >  	fput_light(sock->file, fput_needed);
> >  out:

I think we should call security_socket_post_accept() before fd_install().
Otherwise, other threads which share fd tables can use
security-informations-not-yet-updated accept()ed sockets.

Re: [RFC 2/9] Revert "lsm: Remove the socket_post_accept() hook"

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@I-love.SAKURA.ne.jp):
> Serge E. Hallyn wrote:
> > Quoting Samir Bellabes (sam@synack.fr):
> > > This reverts commit 8651d5c0b1f874c5b8307ae2b858bc40f9f02482.
> > > 
> > > snet needs to reintroduce this hook, as it was designed to be: a hook for
> > > updating security informations on objects.
> > > 
> > > Signed-off-by: Samir Bellabes <sam@synack.fr>
> > 
> > Acked-by: Serge Hallyn <serue@us.ibm.com>
> > 
> > (contingent of course on the proposed user actually going in :)
> > 
> > > diff --git a/net/socket.c b/net/socket.c
> > > index 8984973..fcd4f2b 100644
> > > --- a/net/socket.c
> > > +++ b/net/socket.c
> > > @@ -1557,6 +1557,8 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
> > >  	fd_install(newfd, newfile);
> > >  	err = newfd;
> > > 
> > > +	security_socket_post_accept(sock, newsock);
> > > +
> > >  out_put:
> > >  	fput_light(sock->file, fput_needed);
> > >  out:
> 
> I think we should call security_socket_post_accept() before fd_install().
> Otherwise, other threads which share fd tables can use
> security-informations-not-yet-updated accept()ed sockets.

That makes sense.

[RFC 3/9] snet: introduce security/snet, Makefile and Kconfig changes

[Samir Bellabes]

this patch creates a entry in folder security/ and adds Kconfig and Makefile

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/Kconfig       |    1 +
 security/Makefile      |    2 ++
 security/snet/Kconfig  |   22 ++++++++++++++++++++++
 security/snet/Makefile |   13 +++++++++++++
 4 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/Kconfig
 create mode 100644 security/snet/Makefile

diff --git a/security/Kconfig b/security/Kconfig
index 226b955..48e8fee 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -140,6 +140,7 @@ config LSM_MMAP_MIN_ADDR
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
+source security/snet/Kconfig
 
 source security/integrity/ima/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index bb44e35..0870dd0 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_KEYS)			+= keys/
 subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
 subdir-$(CONFIG_SECURITY_SMACK)		+= smack
 subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
+subdir-$(CONFIG_SECURITY_SNET)		+= snet
 
 # always enable default capabilities
 obj-y		+= commoncap.o min_addr.o
@@ -18,6 +19,7 @@ obj-$(CONFIG_SECURITY_SELINUX)		+= selinux/built-in.o
 obj-$(CONFIG_SECURITY_SMACK)		+= smack/built-in.o
 obj-$(CONFIG_AUDIT)			+= lsm_audit.o
 obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/built-in.o
+obj-$(CONFIG_SECURITY_SNET)		+= snet/built-in.o
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/snet/Kconfig b/security/snet/Kconfig
new file mode 100644
index 0000000..e1516a1
--- /dev/null
+++ b/security/snet/Kconfig
@@ -0,0 +1,22 @@
+#
+# snet
+#
+
+config SECURITY_SNET
+	bool "snet - Security for NETwork syscalls"
+	depends on SECURITY_NETWORK && IPV6
+	default n
+	---help---
+	Provide a generic netlink that reports networking's syscalls
+	to userspace
+
+config SECURITY_SNET_DEBUG
+       bool "snet debug messages"
+       depends on SECURITY_SNET
+       ---help---
+       Only use if you are hacking snet.
+
+       This toggles the debugging outputs, by setting the parameter snet_debug
+       to 0 or 1 at boot.
+
+       Just say N
diff --git a/security/snet/Makefile b/security/snet/Makefile
new file mode 100644
index 0000000..ee6bd83
--- /dev/null
+++ b/security/snet/Makefile
@@ -0,0 +1,13 @@
+#
+# Makefile for building the Security Network Events module.
+#
+obj-$(CONFIG_SECURITY_SNET) :=  snet.o
+
+snet-y := snet_event.o \
+	  snet_netlink.o \
+	  snet_verdict.o \
+	  snet_hooks.o \
+	  snet_core.o \
+	  snet_utils.o
+
+EXTRA_CFLAGS += -Isecurity/snet/include
-- 
1.6.3.3

Re: [RFC 3/9] snet: introduce security/snet, Makefile and Kconfig changes

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> this patch creates a entry in folder security/ and adds Kconfig and Makefile
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---
>  security/Kconfig       |    1 +
>  security/Makefile      |    2 ++
>  security/snet/Kconfig  |   22 ++++++++++++++++++++++
>  security/snet/Makefile |   13 +++++++++++++
>  4 files changed, 38 insertions(+), 0 deletions(-)
>  create mode 100644 security/snet/Kconfig
>  create mode 100644 security/snet/Makefile
> 
> diff --git a/security/Kconfig b/security/Kconfig
> index 226b955..48e8fee 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -140,6 +140,7 @@ config LSM_MMAP_MIN_ADDR
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig
> +source security/snet/Kconfig
> 
>  source security/integrity/ima/Kconfig
> 
> diff --git a/security/Makefile b/security/Makefile
> index bb44e35..0870dd0 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYS)			+= keys/
>  subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
>  subdir-$(CONFIG_SECURITY_SMACK)		+= smack
>  subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
> +subdir-$(CONFIG_SECURITY_SNET)		+= snet
> 
>  # always enable default capabilities
>  obj-y		+= commoncap.o min_addr.o
> @@ -18,6 +19,7 @@ obj-$(CONFIG_SECURITY_SELINUX)		+= selinux/built-in.o
>  obj-$(CONFIG_SECURITY_SMACK)		+= smack/built-in.o
>  obj-$(CONFIG_AUDIT)			+= lsm_audit.o
>  obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/built-in.o
> +obj-$(CONFIG_SECURITY_SNET)		+= snet/built-in.o
>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
> 
>  # Object integrity file lists
> diff --git a/security/snet/Kconfig b/security/snet/Kconfig
> new file mode 100644
> index 0000000..e1516a1
> --- /dev/null
> +++ b/security/snet/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# snet
> +#
> +
> +config SECURITY_SNET
> +	bool "snet - Security for NETwork syscalls"
> +	depends on SECURITY_NETWORK && IPV6

Why depend on IPV6?

> +	default n
> +	---help---
> +	Provide a generic netlink that reports networking's syscalls
> +	to userspace

And also wait for userspace to decide whether to authorize the
syscall, right?  'report on' is very different.

> +
> +config SECURITY_SNET_DEBUG
> +       bool "snet debug messages"
> +       depends on SECURITY_SNET
> +       ---help---
> +       Only use if you are hacking snet.
> +
> +       This toggles the debugging outputs, by setting the parameter snet_debug
> +       to 0 or 1 at boot.
> +
> +       Just say N
> diff --git a/security/snet/Makefile b/security/snet/Makefile
> new file mode 100644
> index 0000000..ee6bd83
> --- /dev/null
> +++ b/security/snet/Makefile
> @@ -0,0 +1,13 @@
> +#
> +# Makefile for building the Security Network Events module.
> +#
> +obj-$(CONFIG_SECURITY_SNET) :=  snet.o
> +
> +snet-y := snet_event.o \
> +	  snet_netlink.o \
> +	  snet_verdict.o \
> +	  snet_hooks.o \
> +	  snet_core.o \
> +	  snet_utils.o
> +
> +EXTRA_CFLAGS += -Isecurity/snet/include
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/9] snet: introduce security/snet, Makefile and Kconfig changes

[Samir Bellabes]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

>> diff --git a/security/snet/Kconfig b/security/snet/Kconfig
>> new file mode 100644
>> index 0000000..e1516a1
>> --- /dev/null
>> +++ b/security/snet/Kconfig
>> @@ -0,0 +1,22 @@
>> +#
>> +# snet
>> +#
>> +
>> +config SECURITY_SNET
>> +	bool "snet - Security for NETwork syscalls"
>> +	depends on SECURITY_NETWORK && IPV6
>
> Why depend on IPV6?

right, no need.

>
>> +	default n
>> +	---help---
>> +	Provide a generic netlink that reports networking's syscalls
>> +	to userspace
>
> And also wait for userspace to decide whether to authorize the
> syscall, right?  'report on' is very different.

I'm proposing this patch, which applies on top of previous

diff --git a/security/snet/Kconfig b/security/snet/Kconfig
index e1516a1..8ac7778 100644
--- a/security/snet/Kconfig
+++ b/security/snet/Kconfig
@@ -4,11 +4,11 @@
 
 config SECURITY_SNET
 	bool "snet - Security for NETwork syscalls"
-	depends on SECURITY_NETWORK && IPV6
+	depends on SECURITY_NETWORK
 	default n
 	---help---
-	Provide a generic netlink that reports networking's syscalls
-	to userspace
+	If this option is enabled, the kernel will include support for reporting
+	networking's syscalls to userspace and wait for a verdict
 
 config SECURITY_SNET_DEBUG
        bool "snet debug messages"

[RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

this patch introduce snet_core.c, which provides main functions to start and
stop snet's subsystems :
	- snet_hooks	: LSM hooks
	- snet_netlink	: kernel-user communication (genetlink)
	- snet_event	: manages the table of protected syscalls
	- snet_verdict	: provides a wait queue for syscalls and manage verdicts
			  from userspace

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet.h |   29 ++++++++++++++++
 security/snet/snet_core.c    |   77 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet.h
 create mode 100644 security/snet/snet_core.c

diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
new file mode 100644
index 0000000..b664a47
--- /dev/null
+++ b/security/snet/include/snet.h
@@ -0,0 +1,29 @@
+#ifndef _SNET_H
+#define _SNET_H
+
+#include "snet_hooks.h"
+
+#define SNET_VERSION	0x1
+#define SNET_NAME	"snet"
+
+#define SNET_PRINTK(enable, fmt, arg...)			\
+	do {							\
+		if (enable)					\
+			printk(KERN_INFO "%s: %s: " fmt ,	\
+				SNET_NAME , __func__ ,		\
+				## arg);			\
+	} while (0)
+
+#ifdef CONFIG_SECURITY_SNET_DEBUG
+extern unsigned int snet_debug;
+#define snet_dbg(fmt, arg...)	SNET_PRINTK(snet_debug, fmt, ##arg)
+#else
+#define snet_dbg(fmt, arg...)
+#endif
+
+struct snet_event {
+	enum snet_syscall syscall;
+	u8 protocol;
+} __attribute__ ((packed));
+
+#endif /* _SNET_H */
diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
new file mode 100644
index 0000000..34b61e9
--- /dev/null
+++ b/security/snet/snet_core.c
@@ -0,0 +1,77 @@
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <net/genetlink.h>
+
+#include "snet.h"
+#include "snet_hooks.h"
+#include "snet_netlink.h"
+#include "snet_event.h"
+#include "snet_verdict.h"
+#include "snet_utils.h"
+
+unsigned int event_hash_size = 16;
+module_param(event_hash_size, uint, 0600);
+MODULE_PARM_DESC(event_hash_size, "Set the size of the event hash table");
+
+unsigned int verdict_hash_size = 16;
+module_param(verdict_hash_size, uint, 0600);
+MODULE_PARM_DESC(verdict_hash_size, "Set the size of the verdict hash table");
+
+unsigned int snet_verdict_delay = 5;
+module_param(snet_verdict_delay, uint, 0600);
+MODULE_PARM_DESC(snet_verdict_delay, "Set the timeout for verdicts in secs");
+
+unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default */
+module_param(snet_verdict_policy, uint, 0600);
+MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
+
+#ifdef CONFIG_SECURITY_SNET_DEBUG
+unsigned int snet_debug;
+EXPORT_SYMBOL_GPL(snet_debug);
+module_param(snet_debug, bool, 0644);
+MODULE_PARM_DESC(snet_debug, "Enable debug messages");
+#endif
+
+void snet_core_exit(void)
+{
+	snet_netlink_exit();
+	snet_event_exit();
+	snet_hooks_exit();
+	snet_verdict_exit();
+	snet_dbg("stopped\n");
+}
+
+static __init int snet_init(void)
+{
+	int ret;
+
+	snet_dbg("initializing: event_hash_size=%u "
+		 "verdict_hash_size=%u verdict_delay=%usecs "
+		 "default_policy=%s\n",
+		 event_hash_size, verdict_hash_size, snet_verdict_delay,
+		 snet_verdict_name(snet_verdict_policy));
+
+	ret = snet_event_init();
+	if (ret < 0)
+		goto exit;
+
+	ret = snet_verdict_init();
+	if (ret < 0)
+		goto exit;
+
+	ret = snet_hooks_init();
+	if (ret < 0)
+		goto exit;
+
+	snet_dbg("started\n");
+	return 0;
+exit:
+	snet_core_exit();
+	return ret;
+}
+
+security_initcall(snet_init);
+
+MODULE_DESCRIPTION("snet - Security for NETwork syscalls");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samir Bellabes <sam@synack.fr>");
-- 
1.6.3.3

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Patrick McHardy]

Samir Bellabes wrote:
> this patch introduce snet_core.c, which provides main functions to start and
> stop snet's subsystems :
> 	- snet_hooks	: LSM hooks
> 	- snet_netlink	: kernel-user communication (genetlink)
> 	- snet_event	: manages the table of protected syscalls
> 	- snet_verdict	: provides a wait queue for syscalls and manage verdicts
> 			  from userspace
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---
>  security/snet/include/snet.h |   29 ++++++++++++++++
>  security/snet/snet_core.c    |   77 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+), 0 deletions(-)
>  create mode 100644 security/snet/include/snet.h
>  create mode 100644 security/snet/snet_core.c
> 
> diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
> new file mode 100644
> index 0000000..b664a47
> --- /dev/null
> +++ b/security/snet/include/snet.h
> @@ -0,0 +1,29 @@
> +#ifndef _SNET_H
> +#define _SNET_H
> +
> +#include "snet_hooks.h"
> +
> +#define SNET_VERSION	0x1
> +#define SNET_NAME	"snet"
> +
> +#define SNET_PRINTK(enable, fmt, arg...)			\
> +	do {							\
> +		if (enable)					\
> +			printk(KERN_INFO "%s: %s: " fmt ,	\
> +				SNET_NAME , __func__ ,		\
> +				## arg);			\
> +	} while (0)

How about using pr_debug()?

> +struct snet_event {
> +	enum snet_syscall syscall;
> +	u8 protocol;
> +} __attribute__ ((packed));

Does this really need to be packed? You're using it in a
struct snet_event_entry, which is padded anyways.

> +
> +#endif /* _SNET_H */
> diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
> new file mode 100644
> index 0000000..34b61e9
> --- /dev/null
> +++ b/security/snet/snet_core.c
> @@ -0,0 +1,77 @@
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <net/genetlink.h>
> +
> +#include "snet.h"
> +#include "snet_hooks.h"
> +#include "snet_netlink.h"
> +#include "snet_event.h"
> +#include "snet_verdict.h"
> +#include "snet_utils.h"
> +
> +unsigned int event_hash_size = 16;
> +module_param(event_hash_size, uint, 0600);
> +MODULE_PARM_DESC(event_hash_size, "Set the size of the event hash table");
> +
> +unsigned int verdict_hash_size = 16;
> +module_param(verdict_hash_size, uint, 0600);
> +MODULE_PARM_DESC(verdict_hash_size, "Set the size of the verdict hash table");

I can't see anything handling size changes after initialization,
so there should probably use 0400.

> +
> +unsigned int snet_verdict_delay = 5;
> +module_param(snet_verdict_delay, uint, 0600);
> +MODULE_PARM_DESC(snet_verdict_delay, "Set the timeout for verdicts in secs");
> +
> +unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default */
> +module_param(snet_verdict_policy, uint, 0600);
> +MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");

> +
> +#ifdef CONFIG_SECURITY_SNET_DEBUG
> +unsigned int snet_debug;
> +EXPORT_SYMBOL_GPL(snet_debug);
> +module_param(snet_debug, bool, 0644);
> +MODULE_PARM_DESC(snet_debug, "Enable debug messages");
> +#endif
> +
> +void snet_core_exit(void)
> +{
> +	snet_netlink_exit();
> +	snet_event_exit();
> +	snet_hooks_exit();
> +	snet_verdict_exit();
> +	snet_dbg("stopped\n");
> +}
> +
> +static __init int snet_init(void)
> +{
> +	int ret;
> +
> +	snet_dbg("initializing: event_hash_size=%u "
> +		 "verdict_hash_size=%u verdict_delay=%usecs "
> +		 "default_policy=%s\n",
> +		 event_hash_size, verdict_hash_size, snet_verdict_delay,
> +		 snet_verdict_name(snet_verdict_policy));
> +
> +	ret = snet_event_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	ret = snet_verdict_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	ret = snet_hooks_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	snet_dbg("started\n");
> +	return 0;
> +exit:
> +	snet_core_exit();
> +	return ret;

By cleaning up only those parts that were successfully initialized,
you could avoid things like the verdict_hash check below:

> +int snet_verdict_exit(void)
> +{
> +	write_lock_bh(&verdict_hash_lock);
> +	if (verdict_hash) {
> +		__snet_verdict_flush();
> +		kfree(verdict_hash);
> +		verdict_hash = NULL;
> +	}
> +	write_unlock_bh(&verdict_hash_lock);
> +
> +	return 0;

Also the exit() functions should return void, there shouldn't
be any error conditions since there's no way to handle them.

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +#define SNET_PRINTK(enable, fmt, arg...)			\
>> +	do {							\
>> +		if (enable)					\
>> +			printk(KERN_INFO "%s: %s: " fmt ,	\
>> +				SNET_NAME , __func__ ,		\
>> +				## arg);			\
>> +	} while (0)
>
> How about using pr_debug()?

right. I moved the code to use pr_debug()
here is the patch

commit 2ca568b34357c8f744a75e3c8191054e23bf5ff2
Author: Samir Bellabes <sam@synack.fr>
Date:   Wed Jan 6 19:09:33 2010 +0100

    snet: use pr_debug() for debug logging
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/Kconfig b/security/snet/Kconfig
index 547a524..6dabd7d 100644
--- a/security/snet/Kconfig
+++ b/security/snet/Kconfig
@@ -9,14 +9,3 @@ config SECURITY_SNET
 	---help---
 	If this option is enabled, the kernel will include support for reporting
 	networking's syscalls to userspace and wait for a verdict
-
-config SECURITY_SNET_DEBUG
-       bool "snet debug messages"
-       depends on SECURITY_SNET
-       ---help---
-       Only use if you are hacking snet.
-
-       This toggles the debugging outputs, by setting the parameter snet_debug
-       to 0 or 1 at boot.
-
-       Just say N
diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
index b664a47..47da614 100644
--- a/security/snet/include/snet.h
+++ b/security/snet/include/snet.h
@@ -6,21 +6,6 @@
 #define SNET_VERSION	0x1
 #define SNET_NAME	"snet"
 
-#define SNET_PRINTK(enable, fmt, arg...)			\
-	do {							\
-		if (enable)					\
-			printk(KERN_INFO "%s: %s: " fmt ,	\
-				SNET_NAME , __func__ ,		\
-				## arg);			\
-	} while (0)
-
-#ifdef CONFIG_SECURITY_SNET_DEBUG
-extern unsigned int snet_debug;
-#define snet_dbg(fmt, arg...)	SNET_PRINTK(snet_debug, fmt, ##arg)
-#else
-#define snet_dbg(fmt, arg...)
-#endif
-
 struct snet_event {
 	enum snet_syscall syscall;
 	u8 protocol;
diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
index 1ef1313..35f0c71 100644
--- a/security/snet/snet_core.c
+++ b/security/snet/snet_core.c
@@ -25,27 +25,20 @@ unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default
 module_param(snet_verdict_policy, uint, 0400);
 MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
 
-#ifdef CONFIG_SECURITY_SNET_DEBUG
-unsigned int snet_debug;
-EXPORT_SYMBOL_GPL(snet_debug);
-module_param(snet_debug, bool, 0644);
-MODULE_PARM_DESC(snet_debug, "Enable debug messages");
-#endif
-
 void snet_core_exit(void)
 {
 	snet_netlink_exit();
 	snet_event_exit();
 	snet_hooks_exit();
 	snet_verdict_exit();
-	snet_dbg("stopped\n");
+	pr_debug("stopped\n");
 }
 
 static __init int snet_init(void)
 {
 	int ret;
 
-	snet_dbg("initializing: event_hash_size=%u "
+	pr_debug("initializing: event_hash_size=%u "
 		 "verdict_hash_size=%u verdict_delay=%usecs "
 		 "default_policy=%s\n",
 		 snet_evh_size, snet_vdh_size, snet_verdict_delay,
@@ -63,7 +56,7 @@ static __init int snet_init(void)
 	if (ret < 0)
 		goto exit;
 
-	snet_dbg("started\n");
+	pr_debug("started\n");
 	return 0;
 exit:
 	snet_core_exit();
diff --git a/security/snet/snet_hooks.c b/security/snet/snet_hooks.c
index 7c0e990..3980350 100644
--- a/security/snet/snet_hooks.c
+++ b/security/snet/snet_hooks.c
@@ -38,19 +38,19 @@
 #include "snet_event.h"
 
 #define SNET_DBG_V4(info)						\
-	snet_dbg("%pI4:%u->%pI4:%u\n",					\
+	pr_debug("%pI4:%u->%pI4:%u\n",					\
 		 &info.src.u3.ip, info.src.u.port,			\
 		 &info.dst.u3.ip, info.dst.u.port)
 
 #define SNET_DBG_V6(info)						\
-	snet_dbg("%pI6:%u->%pI6:%u\n",					\
+	pr_debug("%pI6:%u->%pI6:%u\n",					\
 		 &info.src.u3.ip, info.src.u.port,			\
 		 &info.dst.u3.ip, info.dst.u.port)
 
 #define SNET_CHECK_LISTENERS()						\
 do {									\
 	if (atomic_read(&snet_num_listeners) < 0) {			\
-		snet_dbg("number of listeners is negative\n");		\
+		pr_debug("number of listeners is negative\n");		\
 		verdict = SNET_VERDICT_GRANT;				\
 		goto out;						\
 	} else if (atomic_read(&snet_num_listeners) == 0) {		\
@@ -74,7 +74,7 @@ do {									\
 #define SNET_CHECK_LISTENERS_NOVERDICT()				\
 do {									\
 	if (atomic_read(&snet_num_listeners) < 0) {			\
-		snet_dbg("number of listeners is negative\n");		\
+		pr_debug("number of listeners is negative\n");		\
 		goto out;						\
 	} else if (atomic_read(&snet_num_listeners) == 0) {		\
 		goto out;						\
@@ -116,7 +116,7 @@ static int snet_socket_create(int family, int type, int protocol, int kern)
 		info.family = family;
 		info.type = type;
 
-		snet_dbg("family=%u type=%u protocol=%u kern=%u\n",
+		pr_debug("family=%u type=%u protocol=%u kern=%u\n",
 			 family, type, protocol, kern);
 
 		SNET_DO_VERDICT(info);
diff --git a/security/snet/snet_netlink.c b/security/snet/snet_netlink.c
index 51a4fd7..0b2405b 100644
--- a/security/snet/snet_netlink.c
+++ b/security/snet/snet_netlink.c
@@ -33,7 +33,7 @@ int snet_nl_send_event(struct snet_info *info)
 	if (msg_head == NULL)
 		goto send_event_failure;
 
-	snet_dbg("verdict_id=0x%x syscall=%s protocol=%u "
+	pr_debug("verdict_id=0x%x syscall=%s protocol=%u "
 		 "family=%u uid=%u pid=%u\n",
 		 info->verdict_id, snet_syscall_name(info->syscall),
 		 info->protocol, info->family, current_uid(), current->pid);
@@ -230,7 +230,7 @@ static int snet_nl_register(struct sk_buff *skb, struct genl_info *info)
 				    set_resp_flag);
 
 	snet_nl_pid = info->snd_pid;
-	snet_dbg("pid=%u num_listeners=%d\n",
+	pr_debug("pid=%u num_listeners=%d\n",
 		 snet_nl_pid, atomic_read(&snet_num_listeners));
 	return ret;
 }
@@ -254,7 +254,7 @@ static int snet_nl_unregister(struct sk_buff *skb, struct genl_info *info)
 		atomic_dec(&snet_num_listeners);
 	ret = snet_nl_response_flag(info, &snet_genl_family,
 				    SNET_C_UNREGISTER, SNET_A_UNREGISTERED, 1);
-	snet_dbg("pid=%u num_listeners=%d\n",
+	pr_debug("pid=%u num_listeners=%d\n",
 		 snet_nl_pid, atomic_read(&snet_num_listeners));
 	return ret;
 }
@@ -286,7 +286,7 @@ static int snet_nl_insert(struct sk_buff *skb, struct genl_info *info)
 	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
 	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
 	ret_event = snet_event_insert(syscall, protocol);
-	snet_dbg("syscall=%s protocol=%u insert=%s\n",
+	pr_debug("syscall=%s protocol=%u insert=%s\n",
 		 snet_syscall_name(syscall), protocol,
 		 (ret_event == 0) ? "success" : "failed");
 
@@ -326,7 +326,7 @@ static int snet_nl_remove(struct sk_buff *skb, struct genl_info *info)
 	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
 	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
 	ret_event = snet_event_remove(syscall, protocol);
-	snet_dbg("syscall=%s protocol=%u remove=%s\n",
+	pr_debug("syscall=%s protocol=%u remove=%s\n",
 		 snet_syscall_name(syscall), protocol,
 		 (ret_event == 0) ? "success" : "failed");
 

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +struct snet_event {
>> +	enum snet_syscall syscall;
>> +	u8 protocol;
>> +} __attribute__ ((packed));
>
> Does this really need to be packed? You're using it in a
> struct snet_event_entry, which is padded anyways.

I think that that members needs to be aligned, because this struct is
used for the jhash() computation.
when testing on x86_64, I discovered that jhash value wasn't the same,
for a same struct snet_event. Then I thougth about misaligned data, and
possible 'hole' between syscall and protocol.

anyway, I patched the code to use jhash_2words() or jhash_1word().
here is a patch, which apply on top of initial serie

thank you Patrick,
sam

commit bacdb6b62549b58370298f89f908d66c5a3cab66
Author: Samir Bellabes <sam@synack.fr>
Date:   Wed Jan 6 20:36:06 2010 +0100

    snet : delete attribute packed for snet_event and use proper jash functions
    
    the structure snet_event was using the attribute packed, which seems to be
    unnecessary.
    this patch also fix the good compute of hashes, by moving jash() to
    jash_1word() and jash_2words()
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
index 47da614..75b32d5 100644
--- a/security/snet/include/snet.h
+++ b/security/snet/include/snet.h
@@ -9,6 +9,6 @@
 struct snet_event {
 	enum snet_syscall syscall;
 	u8 protocol;
-} __attribute__ ((packed));
+};
 
 #endif /* _SNET_H */
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index 9f0b0f3..cc3b6a2 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -24,17 +24,12 @@ static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall sysc
 	unsigned int h = 0;
 	struct list_head *l;
 	struct snet_event_entry *s;
-	struct snet_event t;
 
 	if (!snet_evh)
 		return NULL;
 
-	/* building the element to look for */
-	t.syscall = syscall;
-	t.protocol = protocol;
-
 	/* computing its hash value */
-	h = jhash(&t, sizeof(struct snet_event), 0) % snet_evh_size;
+	h = jhash_2words(syscall, protocol, 0) % snet_evh_size;
 	l = &snet_evh[h];
 
 	list_for_each_entry(s, l, list) {
@@ -127,7 +122,7 @@ int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
 	data->se.syscall = syscall;
 	data->se.protocol = protocol;
 	INIT_LIST_HEAD(&(data->list));
-	h = jhash(&(data->se), sizeof(struct snet_event), 0) % snet_evh_size;
+	h = jhash_2words(data->se.syscall, data->se.protocol, 0) % snet_evh_size;
 	list_add_tail(&data->list, &snet_evh[h]);
 	write_unlock_bh(&snet_evh_lock);
 	pr_debug("[%u]=(syscall=%s, protocol=%u)\n",
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
index 0cad06b..477af3b 100644
--- a/security/snet/snet_verdict.c
+++ b/security/snet/snet_verdict.c
@@ -29,18 +29,16 @@ static struct snet_verdict_entry *__snet_verdict_lookup(const u32 verdict_id)
 	unsigned int h = 0;
 	struct list_head *l = NULL;
 	struct snet_verdict_entry *s = NULL;
-	u32 vid = 0;
 
 	if (!snet_vdh)
 		return NULL;
 
-	vid = verdict_id;
 	/* computing its hash value */
-	h = jhash(&vid, sizeof(u32), 0) % snet_vdh_size;
+	h = jhash_1word(verdict_id, 0) % snet_vdh_size;
 	l = &snet_vdh[h];
 
 	list_for_each_entry(s, l, list) {
-		if (s->verdict_id == vid) {
+		if (s->verdict_id == verdict_id) {
 			return s;
 		}
 	}
@@ -134,7 +132,7 @@ int snet_verdict_insert(void)
 	data->verdict_id = verdict_id;
 	data->verdict = SNET_VERDICT_PENDING;
 	INIT_LIST_HEAD(&(data->list));
-	h = jhash(&(data->verdict_id), sizeof(u32), 0) % snet_vdh_size;
+	h = jhash_1word(data->verdict_id, 0) % snet_vdh_size;
 
 	write_lock_bh(&snet_vdh_lock);
 	if (snet_vdh) {

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Evgeniy Polyakov]

On Wed, Jan 06, 2010 at 08:46:35PM +0100, Samir Bellabes (sam@synack.fr) wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
> >> +struct snet_event {
> >> +	enum snet_syscall syscall;
> >> +	u8 protocol;
> >> +} __attribute__ ((packed));
> >
> > Does this really need to be packed? You're using it in a
> > struct snet_event_entry, which is padded anyways.
> 
> I think that that members needs to be aligned, because this struct is
> used for the jhash() computation.
> when testing on x86_64, I discovered that jhash value wasn't the same,
> for a same struct snet_event. Then I thougth about misaligned data, and
> possible 'hole' between syscall and protocol.

Without padding it will eat additional bytes after 'protocol' field,
since enum is 32-bit long.

> anyway, I patched the code to use jhash_2words() or jhash_1word().
> here is a patch, which apply on top of initial serie

What's the purpose of hashing verdict_id?

-- 
	Evgeniy Polyakov

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

I'm sorry for the delay I missed this one.

Evgeniy Polyakov <zbr@ioremap.net> writes:
> On Wed, Jan 06, 2010 at 08:46:35PM +0100, Samir Bellabes (sam@synack.fr) wrote:
>> Patrick McHardy <kaber@trash.net> writes:
>> 
>> >> +struct snet_event {
>> >> +	enum snet_syscall syscall;
>> >> +	u8 protocol;
>> >> +} __attribute__ ((packed));
>> >
>> > Does this really need to be packed? You're using it in a
>> > struct snet_event_entry, which is padded anyways.
>> 
>> I think that that members needs to be aligned, because this struct is
>> used for the jhash() computation.
>> when testing on x86_64, I discovered that jhash value wasn't the same,
>> for a same struct snet_event. Then I thougth about misaligned data, and
>> possible 'hole' between syscall and protocol.
>
> Without padding it will eat additional bytes after 'protocol' field,
> since enum is 32-bit long.

thanks for this help.


>> anyway, I patched the code to use jhash_2words() or jhash_1word().
>> here is a patch, which apply on top of initial serie
>
> What's the purpose of hashing verdict_id?

indeed, there is no specific purpose, I will fix this now.

thanks, and again sorry for the delay.
sam

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Evgeniy Polyakov]

On Sat, Jan 23, 2010 at 03:07:36AM +0100, Samir Bellabes (sam@synack.fr) wrote:
> I'm sorry for the delay I missed this one.

:)

-- 
	Evgeniy Polyakov

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +unsigned int event_hash_size = 16;
>> +module_param(event_hash_size, uint, 0600);
>> +MODULE_PARM_DESC(event_hash_size, "Set the size of the event hash table");
>> +
>> +unsigned int verdict_hash_size = 16;
>> +module_param(verdict_hash_size, uint, 0600);
>> +MODULE_PARM_DESC(verdict_hash_size, "Set the size of the verdict hash table");
>
> I can't see anything handling size changes after initialization,
> so there should probably use 0400.

right, here is a patch

thanks Patrick,
sam

commit af9c2157ecb130c1d08bcbeb121e4f50b3e40ab0
Author: Samir Bellabes <sam@synack.fr>
Date:   Tue Jan 5 17:58:42 2010 +0100

    snet: fixing permission of snet module's parameters
    
    the values of parameters are not changing after initialisation.
    So permissions should be 0400
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
index 6e2befc..bf55758 100644
--- a/security/snet/snet_core.c
+++ b/security/snet/snet_core.c
@@ -10,11 +10,11 @@
 #include "snet_utils.h"
 
 unsigned int snet_evh_size = 16;
-module_param(snet_evh_size, uint, 0600);
+module_param(snet_evh_size, uint, 0400);
 MODULE_PARM_DESC(snet_evh_size, "Set the size of the event hash table");
 
 unsigned int snet_vdh_size = 16;
-module_param(snet_vdh_size, uint, 0600);
+module_param(snet_vdh_size, uint, 0400);
 MODULE_PARM_DESC(snet_vdh_size, "Set the size of the verdict hash table");
 
 unsigned int snet_verdict_delay = 5;
@@ -22,7 +22,7 @@ module_param(snet_verdict_delay, uint, 0600);
 MODULE_PARM_DESC(snet_verdict_delay, "Set the timeout for verdicts in secs");
 
 unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default */
-module_param(snet_verdict_policy, uint, 0600);
+module_param(snet_verdict_policy, uint, 0400);
 MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
 
 #ifdef CONFIG_SECURITY_SNET_DEBUG

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +static __init int snet_init(void)
>> +{
>> +	int ret;
>> +
>> +	snet_dbg("initializing: event_hash_size=%u "
>> +		 "verdict_hash_size=%u verdict_delay=%usecs "
>> +		 "default_policy=%s\n",
>> +		 event_hash_size, verdict_hash_size, snet_verdict_delay,
>> +		 snet_verdict_name(snet_verdict_policy));
>> +
>> +	ret = snet_event_init();
>> +	if (ret < 0)
>> +		goto exit;
>> +
>> +	ret = snet_verdict_init();
>> +	if (ret < 0)
>> +		goto exit;
>> +
>> +	ret = snet_hooks_init();
>> +	if (ret < 0)
>> +		goto exit;
>> +
>> +	snet_dbg("started\n");
>> +	return 0;
>> +exit:
>> +	snet_core_exit();
>> +	return ret;
>
> By cleaning up only those parts that were successfully initialized,
> you could avoid things like the verdict_hash check below:
>
>> +int snet_verdict_exit(void)
>> +{
>> +	write_lock_bh(&verdict_hash_lock);
>> +	if (verdict_hash) {
>> +		__snet_verdict_flush();
>> +		kfree(verdict_hash);
>> +		verdict_hash = NULL;
>> +	}
>> +	write_unlock_bh(&verdict_hash_lock);
>> +
>> +	return 0;

true. here is the patch I made to fix this.

Patrick, thanks
sam

commit 76c53bb2e6ac2ff6b8426c9b22c67cde1ae5ac07
Author: Samir Bellabes <sam@synack.fr>
Date:   Thu Jan 7 23:09:17 2010 +0100

    snet: avoid unnecessary checks by fixing initialisation for verdict and event
    
    checking if snet_evh and snet_vdh are NULL is unnecessary if the initalisations
    are sucessfull.
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
index 562d986..54fad08 100644
--- a/security/snet/snet_core.c
+++ b/security/snet/snet_core.c
@@ -25,15 +25,6 @@ unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default
 module_param(snet_verdict_policy, uint, 0400);
 MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
 
-void snet_core_exit(void)
-{
-	snet_netlink_exit();
-	snet_event_exit();
-	snet_hooks_exit();
-	snet_verdict_exit();
-	pr_debug("stopped\n");
-}
-
 static __init int snet_init(void)
 {
 	int ret;
@@ -46,20 +37,25 @@ static __init int snet_init(void)
 
 	ret = snet_event_init();
 	if (ret < 0)
-		goto exit;
+		goto event_failed;
 
 	ret = snet_verdict_init();
 	if (ret < 0)
-		goto exit;
+		goto verdict_failed;
 
 	ret = snet_hooks_init();
 	if (ret < 0)
-		goto exit;
+		goto hooks_failed;
 
 	pr_debug("started\n");
 	return 0;
-exit:
-	snet_core_exit();
+
+hooks_failed:
+	snet_verdict_exit();
+verdict_failed:
+	snet_event_exit();
+event_failed:
+	pr_debug("stopped\n");
 	return ret;
 }
 
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index cc3b6a2..44155fb 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -25,9 +25,6 @@ static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall sysc
 	struct list_head *l;
 	struct snet_event_entry *s;
 
-	if (!snet_evh)
-		return NULL;
-
 	/* computing its hash value */
 	h = jhash_2words(syscall, protocol, 0) % snet_evh_size;
 	l = &snet_evh[h];
@@ -52,9 +49,6 @@ int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
 
 	read_lock_bh(&snet_evh_lock);
 
-	if (!snet_evh)
-		goto errout;
-
 	for (i = 0; i < snet_evh_size; i++) {
 		if (i < hashs_to_skip)
 			continue;
@@ -151,11 +145,12 @@ int snet_event_remove(const enum snet_syscall syscall, const u8 protocol)
 }
 
 /* flushing all events */
-void __snet_event_flush(void)
+void snet_event_flush(void)
 {
 	struct snet_event_entry *data = NULL;
 	unsigned int i = 0;
 
+	write_lock_bh(&snet_evh_lock);
 	for (i = 0; i < snet_evh_size; i++) {
 		while (!list_empty(&snet_evh[i])) {
 			data = list_entry(snet_evh[i].next,
@@ -164,14 +159,6 @@ void __snet_event_flush(void)
 			kfree(data);
 		}
 	}
-	return;
-}
-
-void snet_event_flush(void)
-{
-	write_lock_bh(&snet_evh_lock);
-	if (snet_evh)
-		__snet_event_flush();
 	write_unlock_bh(&snet_evh_lock);
 	return;
 }
@@ -200,13 +187,7 @@ out:
 /* exit function */
 int snet_event_exit(void)
 {
-	write_lock_bh(&snet_evh_lock);
-	if (snet_evh) {
-		__snet_event_flush();
-		kfree(snet_evh);
-		snet_evh = NULL;
-	}
-	write_unlock_bh(&snet_evh_lock);
-
+	kfree(snet_evh);
+	snet_evh = NULL;
 	return 0;
 }
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
index 477af3b..78bc882 100644
--- a/security/snet/snet_verdict.c
+++ b/security/snet/snet_verdict.c
@@ -30,9 +30,6 @@ static struct snet_verdict_entry *__snet_verdict_lookup(const u32 verdict_id)
 	struct list_head *l = NULL;
 	struct snet_verdict_entry *s = NULL;
 
-	if (!snet_vdh)
-		return NULL;
-
 	/* computing its hash value */
 	h = jhash_1word(verdict_id, 0) % snet_vdh_size;
 	l = &snet_vdh[h];
@@ -135,24 +132,19 @@ int snet_verdict_insert(void)
 	h = jhash_1word(data->verdict_id, 0) % snet_vdh_size;
 
 	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh) {
-		list_add_tail(&data->list, &snet_vdh[h]);
-		pr_debug("[%u]=(verdict_id=%u)\n", h, data->verdict_id);
-		write_unlock_bh(&snet_vdh_lock);
-	} else {
-		write_unlock_bh(&snet_vdh_lock);
-		kfree(data);
-		verdict_id = 0;
-	}
+	list_add_tail(&data->list, &snet_vdh[h]);
+	pr_debug("[%u]=(verdict_id=%u)\n", h, data->verdict_id);
+	write_unlock_bh(&snet_vdh_lock);
 
 	return verdict_id;
 }
 
-void __snet_verdict_flush(void)
+void snet_verdict_flush(void)
 {
 	struct snet_verdict_entry *data = NULL;
 	unsigned int i = 0;
 
+	write_lock_bh(&snet_vdh_lock);
 	for (i = 0; i < snet_vdh_size; i++) {
 		while (!list_empty(&snet_vdh[i])) {
 			data = list_entry(snet_vdh[i].next,
@@ -161,15 +153,7 @@ void __snet_verdict_flush(void)
 			kfree(data);
 		}
 	}
-	return;
-}
-
-void snet_verdict_flush(void)
-{
-	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh)
-		__snet_verdict_flush();
-	write_unlock_bh(&snet_vdh_lock);
+	write_untlock_bh(&snet_vdh_lock);
 	return;
 }
 
@@ -197,13 +181,7 @@ out:
 /* exit function */
 int snet_verdict_exit(void)
 {
-	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh) {
-		__snet_verdict_flush();
-		kfree(snet_vdh);
-		snet_vdh = NULL;
-	}
-	write_unlock_bh(&snet_vdh_lock);
-
+	kfree(snet_vdh);
+	snet_vdh = NULL;
 	return 0;
 }

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Samir Bellabes <sam@synack.fr> writes:

> -void snet_verdict_flush(void)
> -{
> -	write_lock_bh(&snet_vdh_lock);
> -	if (snet_vdh)
> -		__snet_verdict_flush();
> -	write_unlock_bh(&snet_vdh_lock);
> +	write_untlock_bh(&snet_vdh_lock);

I introduce a typo here, (untblock)
this is fixed now.
I'm sorry for not getting this at first time.

sam

>  	return;
>  }
>  
> @@ -197,13 +181,7 @@ out:
>  /* exit function */
>  int snet_verdict_exit(void)
>  {
> -	write_lock_bh(&snet_vdh_lock);
> -	if (snet_vdh) {
> -		__snet_verdict_flush();
> -		kfree(snet_vdh);
> -		snet_vdh = NULL;
> -	}
> -	write_unlock_bh(&snet_vdh_lock);
> -
> +	kfree(snet_vdh);
> +	snet_vdh = NULL;
>  	return 0;
>  }

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +int snet_verdict_exit(void)
>> +{
>> +	write_lock_bh(&verdict_hash_lock);
>> +	if (verdict_hash) {
>> +		__snet_verdict_flush();
>> +		kfree(verdict_hash);
>> +		verdict_hash = NULL;
>> +	}
>> +	write_unlock_bh(&verdict_hash_lock);
>> +
>> +	return 0;
>
> Also the exit() functions should return void, there shouldn't
> be any error conditions since there's no way to handle them.

right. I fixed this with this patch

Patrick, thank you for your time and your comments
sam

commit ca287efd0099b67340dd6c61fbe18fb7fda33872
Author: Samir Bellabes <sam@synack.fr>
Date:   Thu Jan 7 23:09:17 2010 +0100

    snet: avoid unnecessary checks by fixing initialisation for verdict and event
    
    checking if snet_evh and snet_vdh are NULL is unnecessary if the initalisations
    are sucessfull.
    
    Noticed by Patrick McHardy <kaber@trash.net>
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
index 562d986..54fad08 100644
--- a/security/snet/snet_core.c
+++ b/security/snet/snet_core.c
@@ -25,15 +25,6 @@ unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default
 module_param(snet_verdict_policy, uint, 0400);
 MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
 
-void snet_core_exit(void)
-{
-	snet_netlink_exit();
-	snet_event_exit();
-	snet_hooks_exit();
-	snet_verdict_exit();
-	pr_debug("stopped\n");
-}
-
 static __init int snet_init(void)
 {
 	int ret;
@@ -46,20 +37,25 @@ static __init int snet_init(void)
 
 	ret = snet_event_init();
 	if (ret < 0)
-		goto exit;
+		goto event_failed;
 
 	ret = snet_verdict_init();
 	if (ret < 0)
-		goto exit;
+		goto verdict_failed;
 
 	ret = snet_hooks_init();
 	if (ret < 0)
-		goto exit;
+		goto hooks_failed;
 
 	pr_debug("started\n");
 	return 0;
-exit:
-	snet_core_exit();
+
+hooks_failed:
+	snet_verdict_exit();
+verdict_failed:
+	snet_event_exit();
+event_failed:
+	pr_debug("stopped\n");
 	return ret;
 }
 
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
index cc3b6a2..44155fb 100644
--- a/security/snet/snet_event.c
+++ b/security/snet/snet_event.c
@@ -25,9 +25,6 @@ static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall sysc
 	struct list_head *l;
 	struct snet_event_entry *s;
 
-	if (!snet_evh)
-		return NULL;
-
 	/* computing its hash value */
 	h = jhash_2words(syscall, protocol, 0) % snet_evh_size;
 	l = &snet_evh[h];
@@ -52,9 +49,6 @@ int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
 
 	read_lock_bh(&snet_evh_lock);
 
-	if (!snet_evh)
-		goto errout;
-
 	for (i = 0; i < snet_evh_size; i++) {
 		if (i < hashs_to_skip)
 			continue;
@@ -151,11 +145,12 @@ int snet_event_remove(const enum snet_syscall syscall, const u8 protocol)
 }
 
 /* flushing all events */
-void __snet_event_flush(void)
+void snet_event_flush(void)
 {
 	struct snet_event_entry *data = NULL;
 	unsigned int i = 0;
 
+	write_lock_bh(&snet_evh_lock);
 	for (i = 0; i < snet_evh_size; i++) {
 		while (!list_empty(&snet_evh[i])) {
 			data = list_entry(snet_evh[i].next,
@@ -164,14 +159,6 @@ void __snet_event_flush(void)
 			kfree(data);
 		}
 	}
-	return;
-}
-
-void snet_event_flush(void)
-{
-	write_lock_bh(&snet_evh_lock);
-	if (snet_evh)
-		__snet_event_flush();
 	write_unlock_bh(&snet_evh_lock);
 	return;
 }
@@ -200,13 +187,7 @@ out:
 /* exit function */
 int snet_event_exit(void)
 {
-	write_lock_bh(&snet_evh_lock);
-	if (snet_evh) {
-		__snet_event_flush();
-		kfree(snet_evh);
-		snet_evh = NULL;
-	}
-	write_unlock_bh(&snet_evh_lock);
-
+	kfree(snet_evh);
+	snet_evh = NULL;
 	return 0;
 }
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
index 477af3b..d9f17f5 100644
--- a/security/snet/snet_verdict.c
+++ b/security/snet/snet_verdict.c
@@ -30,9 +30,6 @@ static struct snet_verdict_entry *__snet_verdict_lookup(const u32 verdict_id)
 	struct list_head *l = NULL;
 	struct snet_verdict_entry *s = NULL;
 
-	if (!snet_vdh)
-		return NULL;
-
 	/* computing its hash value */
 	h = jhash_1word(verdict_id, 0) % snet_vdh_size;
 	l = &snet_vdh[h];
@@ -135,24 +132,19 @@ int snet_verdict_insert(void)
 	h = jhash_1word(data->verdict_id, 0) % snet_vdh_size;
 
 	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh) {
-		list_add_tail(&data->list, &snet_vdh[h]);
-		pr_debug("[%u]=(verdict_id=%u)\n", h, data->verdict_id);
-		write_unlock_bh(&snet_vdh_lock);
-	} else {
-		write_unlock_bh(&snet_vdh_lock);
-		kfree(data);
-		verdict_id = 0;
-	}
+	list_add_tail(&data->list, &snet_vdh[h]);
+	pr_debug("[%u]=(verdict_id=%u)\n", h, data->verdict_id);
+	write_unlock_bh(&snet_vdh_lock);
 
 	return verdict_id;
 }
 
-void __snet_verdict_flush(void)
+void snet_verdict_flush(void)
 {
 	struct snet_verdict_entry *data = NULL;
 	unsigned int i = 0;
 
+	write_lock_bh(&snet_vdh_lock);
 	for (i = 0; i < snet_vdh_size; i++) {
 		while (!list_empty(&snet_vdh[i])) {
 			data = list_entry(snet_vdh[i].next,
@@ -161,14 +153,6 @@ void __snet_verdict_flush(void)
 			kfree(data);
 		}
 	}
-	return;
-}
-
-void snet_verdict_flush(void)
-{
-	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh)
-		__snet_verdict_flush();
 	write_unlock_bh(&snet_vdh_lock);
 	return;
 }
@@ -197,13 +181,7 @@ out:
 /* exit function */
 int snet_verdict_exit(void)
 {
-	write_lock_bh(&snet_vdh_lock);
-	if (snet_vdh) {
-		__snet_verdict_flush();
-		kfree(snet_vdh);
-		snet_vdh = NULL;
-	}
-	write_unlock_bh(&snet_vdh_lock);
-
+	kfree(snet_vdh);
+	snet_vdh = NULL;
 	return 0;
 }

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> this patch introduce snet_core.c, which provides main functions to start and
> stop snet's subsystems :
> 	- snet_hooks	: LSM hooks
> 	- snet_netlink	: kernel-user communication (genetlink)
> 	- snet_event	: manages the table of protected syscalls
> 	- snet_verdict	: provides a wait queue for syscalls and manage verdicts
> 			  from userspace
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---
>  security/snet/include/snet.h |   29 ++++++++++++++++
>  security/snet/snet_core.c    |   77 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+), 0 deletions(-)
>  create mode 100644 security/snet/include/snet.h
>  create mode 100644 security/snet/snet_core.c
> 
> diff --git a/security/snet/include/snet.h b/security/snet/include/snet.h
> new file mode 100644
> index 0000000..b664a47
> --- /dev/null
> +++ b/security/snet/include/snet.h
> @@ -0,0 +1,29 @@
> +#ifndef _SNET_H
> +#define _SNET_H
> +
> +#include "snet_hooks.h"
> +
> +#define SNET_VERSION	0x1
> +#define SNET_NAME	"snet"
> +
> +#define SNET_PRINTK(enable, fmt, arg...)			\
> +	do {							\
> +		if (enable)					\
> +			printk(KERN_INFO "%s: %s: " fmt ,	\
> +				SNET_NAME , __func__ ,		\
> +				## arg);			\
> +	} while (0)
> +
> +#ifdef CONFIG_SECURITY_SNET_DEBUG
> +extern unsigned int snet_debug;
> +#define snet_dbg(fmt, arg...)	SNET_PRINTK(snet_debug, fmt, ##arg)
> +#else
> +#define snet_dbg(fmt, arg...)
> +#endif
> +
> +struct snet_event {
> +	enum snet_syscall syscall;
> +	u8 protocol;
> +} __attribute__ ((packed));
> +
> +#endif /* _SNET_H */
> diff --git a/security/snet/snet_core.c b/security/snet/snet_core.c
> new file mode 100644
> index 0000000..34b61e9
> --- /dev/null
> +++ b/security/snet/snet_core.c
> @@ -0,0 +1,77 @@
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <net/genetlink.h>
> +
> +#include "snet.h"
> +#include "snet_hooks.h"
> +#include "snet_netlink.h"
> +#include "snet_event.h"
> +#include "snet_verdict.h"
> +#include "snet_utils.h"
> +
> +unsigned int event_hash_size = 16;
> +module_param(event_hash_size, uint, 0600);
> +MODULE_PARM_DESC(event_hash_size, "Set the size of the event hash table");
> +
> +unsigned int verdict_hash_size = 16;
> +module_param(verdict_hash_size, uint, 0600);
> +MODULE_PARM_DESC(verdict_hash_size, "Set the size of the verdict hash table");
> +
> +unsigned int snet_verdict_delay = 5;
> +module_param(snet_verdict_delay, uint, 0600);
> +MODULE_PARM_DESC(snet_verdict_delay, "Set the timeout for verdicts in secs");
> +
> +unsigned int snet_verdict_policy = SNET_VERDICT_GRANT;	/* permissive by default */
> +module_param(snet_verdict_policy, uint, 0600);
> +MODULE_PARM_DESC(snet_verdict_policy, "Set the default verdict");
> +
> +#ifdef CONFIG_SECURITY_SNET_DEBUG
> +unsigned int snet_debug;
> +EXPORT_SYMBOL_GPL(snet_debug);
> +module_param(snet_debug, bool, 0644);
> +MODULE_PARM_DESC(snet_debug, "Enable debug messages");
> +#endif
> +
> +void snet_core_exit(void)
> +{
> +	snet_netlink_exit();
> +	snet_event_exit();
> +	snet_hooks_exit();
> +	snet_verdict_exit();
> +	snet_dbg("stopped\n");
> +}
> +
> +static __init int snet_init(void)
> +{
> +	int ret;
> +
> +	snet_dbg("initializing: event_hash_size=%u "
> +		 "verdict_hash_size=%u verdict_delay=%usecs "
> +		 "default_policy=%s\n",
> +		 event_hash_size, verdict_hash_size, snet_verdict_delay,
> +		 snet_verdict_name(snet_verdict_policy));
> +
> +	ret = snet_event_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	ret = snet_verdict_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	ret = snet_hooks_init();
> +	if (ret < 0)
> +		goto exit;
> +
> +	snet_dbg("started\n");
> +	return 0;
> +exit:
> +	snet_core_exit();
> +	return ret;
> +}
> +
> +security_initcall(snet_init);
> +
> +MODULE_DESCRIPTION("snet - Security for NETwork syscalls");

Would 'usnet' or 'ucsnet' - userspace-controlled security for
network syscalls - be more informative?

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Samir Bellabes <sam@synack.fr>");
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/9] snet: introduce snet_core.c and snet.h

[Samir Bellabes]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Samir Bellabes (sam@synack.fr):
>> +
>> +MODULE_DESCRIPTION("snet - Security for NETwork syscalls");
>
> Would 'usnet' or 'ucsnet' - userspace-controlled security for
> network syscalls - be more informative?

maybe a better suitable name is required. discussion is opened :)
'snet' was short.

[RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes]

This patch adds the snet's subsystem responsive of managing events

snet is using the word 'event' for a couple of values [syscall, protocol]. For
example, [listen, tcp] or [sendmsg, dccp] are events.
This patch introduces a hastable 'event_hash' and operations (add/remove/search..)
in order to manage which events have to be protected.
With the help of the communication's subsystem, managing orders are coming from
userspace.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet_event.h |   20 +++
 security/snet/snet_event.c         |  229 ++++++++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet_event.h
 create mode 100644 security/snet/snet_event.c

diff --git a/security/snet/include/snet_event.h b/security/snet/include/snet_event.h
new file mode 100644
index 0000000..2c71ca7
--- /dev/null
+++ b/security/snet/include/snet_event.h
@@ -0,0 +1,20 @@
+#ifndef _SNET_EVENT_H
+#define _SNET_EVENT_H
+#include <linux/skbuff.h>
+
+extern unsigned int event_hash_size;
+
+/* manipulate the events hash table */
+int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb);
+int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol);
+int snet_event_insert(const enum snet_syscall syscall, const u8 protocol);
+int snet_event_remove(const enum snet_syscall syscall, const u8 protocol);
+void snet_event_flush(void);
+void snet_event_dumpall(void);
+
+/* init function */
+int snet_event_init(void);
+/* exit funtion */
+int snet_event_exit(void);
+
+#endif /* _SNET_EVENT_H */
diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
new file mode 100644
index 0000000..6ac5646
--- /dev/null
+++ b/security/snet/snet_event.c
@@ -0,0 +1,229 @@
+#include <linux/spinlock.h>
+#include <linux/list.h>
+#include <linux/jhash.h>
+#include <linux/slab.h>
+#include <linux/netlink.h>
+
+#include "snet.h"
+#include "snet_event.h"
+#include "snet_netlink.h"
+
+static struct list_head *event_hash;
+static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
+
+struct snet_event_entry {
+	struct list_head list;
+	struct snet_event se;
+};
+
+/* lookup for a event_hash - before using this function, lock event_hash_lock */
+static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
+						   const u8 protocol)
+{
+	unsigned int h = 0;
+	struct list_head *l;
+	struct snet_event_entry *s;
+	struct snet_event t;
+
+	if (!event_hash)
+		return NULL;
+
+	/* building the element to look for */
+	t.syscall = syscall;
+	t.protocol = protocol;
+
+	/* computing its hash value */
+	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;
+	l = &event_hash[h];
+
+	list_for_each_entry(s, l, list) {
+		if ((s->se.protocol == protocol) &&
+		    (s->se.syscall == syscall)) {
+			return s;
+		}
+	}
+	return NULL;
+}
+
+int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	unsigned int i = 0, n = 0;
+	int ret = -1;
+	unsigned hashs_to_skip = cb->args[0];
+	unsigned events_to_skip = cb->args[1];
+	struct list_head *l;
+	struct snet_event_entry *s;
+
+	read_lock_bh(&event_hash_lock);
+
+	if (!event_hash)
+		goto errout;
+
+	for (i = 0; i < event_hash_size; i++) {
+		if (i < hashs_to_skip)
+			continue;
+		l = &event_hash[i];
+		n = 0;
+		list_for_each_entry(s, l, list) {
+			if (++n < events_to_skip)
+				continue;
+			ret = snet_nl_list_fill_info(skb,
+						     NETLINK_CB(cb->skb).pid,
+						     cb->nlh->nlmsg_seq,
+						     NLM_F_MULTI,
+						     s->se.protocol,
+						     s->se.syscall);
+			if (ret < 0)
+				goto errout;
+		}
+	}
+
+errout:
+	read_unlock_bh(&event_hash_lock);
+
+	cb->args[0] = i;
+	cb->args[1] = n;
+	return skb->len;
+}
+
+/* void snet_event_dumpall() */
+/* { */
+/*	unsigned int i = 0; */
+/*	struct list_head *l; */
+/*	struct snet_event_entry *s; */
+
+/*	snet_dbg("entering\n"); */
+/*	read_lock_bh(&event_hash_lock); */
+/*	for (i = 0; i < (event_hash_size - 1); i++) { */
+/*		l = &hash[i]; */
+/*		list_for_each_entry(s, l, list) { */
+/*			snet_dbg("[%d, %d, %d]\n", i, */
+/*				 s->se.protocol, s->se.syscall); */
+/*		} */
+/*	} */
+/*	read_unlock_bh(&event_hash_lock); */
+/*	snet_dbg("exiting\n"); */
+/*	return; */
+/* } */
+
+/*
+ * check if a event is registered or not
+ * return 1 if event is registered, 0 if not
+ */
+int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol)
+{
+	int ret = 0;
+
+	read_lock_bh(&event_hash_lock);
+	if (__snet_event_lookup(syscall, protocol) != NULL)
+		ret = 1;
+	read_unlock_bh(&event_hash_lock);
+	return ret;
+}
+
+/* adding a event */
+int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
+{
+	struct snet_event_entry *data = NULL;
+	unsigned int h = 0;
+
+	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	write_lock_bh(&event_hash_lock);
+	/* check if event is already registered */
+	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
+		write_unlock_bh(&event_hash_lock);
+		kfree(data);
+		return -EINVAL;
+	}
+
+	data->se.syscall = syscall;
+	data->se.protocol = protocol;
+	INIT_LIST_HEAD(&(data->list));
+	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
+	list_add_tail(&data->list, &event_hash[h]);
+	write_unlock_bh(&event_hash_lock);
+
+	return 0;
+}
+
+/* removing a event */
+int snet_event_remove(const enum snet_syscall syscall, const u8 protocol)
+{
+	struct snet_event_entry *data = NULL;
+
+	write_lock_bh(&event_hash_lock);
+	data = __snet_event_lookup(syscall, protocol);
+	if (data == NULL) {
+		write_unlock_bh(&event_hash_lock);
+		return -EINVAL;
+	}
+
+	list_del(&data->list);
+	write_unlock_bh(&event_hash_lock);
+	kfree(data);
+	return 0;
+}
+
+/* flushing all events */
+void __snet_event_flush(void)
+{
+	struct snet_event_entry *data = NULL;
+	unsigned int i = 0;
+
+	for (i = 0; i < event_hash_size; i++) {
+		while (!list_empty(&event_hash[i])) {
+			data = list_entry(event_hash[i].next,
+					  struct snet_event_entry, list);
+			list_del(&data->list);
+			kfree(data);
+		}
+	}
+	return;
+}
+
+void snet_event_flush(void)
+{
+	write_lock_bh(&event_hash_lock);
+	if (event_hash)
+		__snet_event_flush();
+	write_unlock_bh(&event_hash_lock);
+	return;
+}
+
+/* init function */
+int snet_event_init(void)
+{
+	int err = 0, i = 0;
+
+	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
+			     GFP_KERNEL);
+	if (!event_hash) {
+		printk(KERN_WARNING
+		       "snet: can't alloc memory for event_hash\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < event_hash_size; i++)
+		INIT_LIST_HEAD(&(event_hash[i]));
+
+out:
+	return err;
+}
+
+/* exit function */
+int snet_event_exit(void)
+{
+	write_lock_bh(&event_hash_lock);
+	if (event_hash) {
+		__snet_event_flush();
+		kfree(event_hash);
+		event_hash = NULL;
+	}
+	write_unlock_bh(&event_hash_lock);
+
+	return 0;
+}
-- 
1.6.3.3

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Evgeniy Polyakov]

Hi.

On Sat, Jan 02, 2010 at 02:04:12PM +0100, Samir Bellabes (sam@synack.fr) wrote:
> This patch adds the snet's subsystem responsive of managing events
> 
> snet is using the word 'event' for a couple of values [syscall, protocol]. For
> example, [listen, tcp] or [sendmsg, dccp] are events.
> This patch introduces a hastable 'event_hash' and operations (add/remove/search..)
> in order to manage which events have to be protected.
> With the help of the communication's subsystem, managing orders are coming from
> userspace.
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---

> +
> +extern unsigned int event_hash_size;
> +

> +
> +static struct list_head *event_hash;
> +static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
> +

Those are way too generic names, please use snet_ prefix as you did in
other places.


I believe snet should be converted to RCU instead of rw locks.

> +/* lookup for a event_hash - before using this function, lock event_hash_lock */
> +static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
> +						   const u8 protocol)
> +{
> +	unsigned int h = 0;
> +	struct list_head *l;
> +	struct snet_event_entry *s;
> +	struct snet_event t;
> +
> +	if (!event_hash)
> +		return NULL;
> +
> +	/* building the element to look for */
> +	t.syscall = syscall;
> +	t.protocol = protocol;
> +
> +	/* computing its hash value */
> +	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;

You can have better distribution if not simply cut off the remainder.
Also it is a rather expensive operation.

> +	l = &event_hash[h];
> +
> +	list_for_each_entry(s, l, list) {
> +		if ((s->se.protocol == protocol) &&
> +		    (s->se.syscall == syscall)) {
> +			return s;
> +		}
> +	}
> +	return NULL;
> +}

Below comments looks a little bit weird, was it generated automatically
by editor?

> +/* void snet_event_dumpall() */
> +/* { */
> +/*	unsigned int i = 0; */
> +/*	struct list_head *l; */
> +/*	struct snet_event_entry *s; */
> +
> +/*	snet_dbg("entering\n"); */
> +/*	read_lock_bh(&event_hash_lock); */
> +/*	for (i = 0; i < (event_hash_size - 1); i++) { */
> +/*		l = &hash[i]; */
> +/*		list_for_each_entry(s, l, list) { */
> +/*			snet_dbg("[%d, %d, %d]\n", i, */
> +/*				 s->se.protocol, s->se.syscall); */
> +/*		} */
> +/*	} */
> +/*	read_unlock_bh(&event_hash_lock); */
> +/*	snet_dbg("exiting\n"); */
> +/*	return; */
> +/* } */

> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
> +{
> +	struct snet_event_entry *data = NULL;
> +	unsigned int h = 0;
> +
> +	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	write_lock_bh(&event_hash_lock);
> +	/* check if event is already registered */
> +	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
> +		write_unlock_bh(&event_hash_lock);
> +		kfree(data);
> +		return -EINVAL;
> +	}

What about single error exiting point?

> +
> +	data->se.syscall = syscall;
> +	data->se.protocol = protocol;
> +	INIT_LIST_HEAD(&(data->list));
> +	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
> +	list_add_tail(&data->list, &event_hash[h]);
> +	write_unlock_bh(&event_hash_lock);
> +
> +	return 0;
> +}

> +/* init function */
> +int snet_event_init(void)
> +{
> +	int err = 0, i = 0;
> +
> +	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
> +			     GFP_KERNEL);
> +	if (!event_hash) {
> +		printk(KERN_WARNING
> +		       "snet: can't alloc memory for event_hash\n");
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < event_hash_size; i++)
> +		INIT_LIST_HEAD(&(event_hash[i]));

No need for double braces.

-- 
	Evgeniy Polyakov

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes]

Hello Evgeniy,

Evgeniy Polyakov <zbr@ioremap.net> writes:
>> +
>> +extern unsigned int event_hash_size;
>> +
>
>> +
>> +static struct list_head *event_hash;
>> +static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
>> +
>
> Those are way too generic names, please use snet_ prefix as you did in
> other places.

done. thank you

> I believe snet should be converted to RCU instead of rw locks.

I see, I will investigated this idea.

>> +/* lookup for a event_hash - before using this function, lock event_hash_lock */
>> +static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
>> +						   const u8 protocol)
>> +{
>> +	unsigned int h = 0;
>> +	struct list_head *l;
>> +	struct snet_event_entry *s;
>> +	struct snet_event t;
>> +
>> +	if (!event_hash)
>> +		return NULL;
>> +
>> +	/* building the element to look for */
>> +	t.syscall = syscall;
>> +	t.protocol = protocol;
>> +
>> +	/* computing its hash value */
>> +	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;
>
> You can have better distribution if not simply cut off the remainder.
> Also it is a rather expensive operation.

true, I replaced the modulo operation to a bitmask operations.

>> +	l = &event_hash[h];
>> +
>> +	list_for_each_entry(s, l, list) {
>> +		if ((s->se.protocol == protocol) &&
>> +		    (s->se.syscall == syscall)) {
>> +			return s;
>> +		}
>> +	}
>> +	return NULL;
>> +}
>
> Below comments looks a little bit weird, was it generated automatically
> by editor?

no, it's was a unused debug function. I deleted this comment.

>> +/* void snet_event_dumpall() */
>> +/* { */
>> +/*	unsigned int i = 0; */
>> +/*	struct list_head *l; */
>> +/*	struct snet_event_entry *s; */
>> +
>> +/*	snet_dbg("entering\n"); */
>> +/*	read_lock_bh(&event_hash_lock); */
>> +/*	for (i = 0; i < (event_hash_size - 1); i++) { */
>> +/*		l = &hash[i]; */
>> +/*		list_for_each_entry(s, l, list) { */
>> +/*			snet_dbg("[%d, %d, %d]\n", i, */
>> +/*				 s->se.protocol, s->se.syscall); */
>> +/*		} */
>> +/*	} */
>> +/*	read_unlock_bh(&event_hash_lock); */
>> +/*	snet_dbg("exiting\n"); */
>> +/*	return; */
>> +/* } */
>
>> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
>> +{
>> +	struct snet_event_entry *data = NULL;
>> +	unsigned int h = 0;
>> +
>> +	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	write_lock_bh(&event_hash_lock);
>> +	/* check if event is already registered */
>> +	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
>> +		write_unlock_bh(&event_hash_lock);
>> +		kfree(data);
>> +		return -EINVAL;
>> +	}
>
> What about single error exiting point?

done, thanks

>> +
>> +	data->se.syscall = syscall;
>> +	data->se.protocol = protocol;
>> +	INIT_LIST_HEAD(&(data->list));
>> +	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
>> +	list_add_tail(&data->list, &event_hash[h]);
>> +	write_unlock_bh(&event_hash_lock);
>> +
>> +	return 0;
>> +}
>
>> +/* init function */
>> +int snet_event_init(void)
>> +{
>> +	int err = 0, i = 0;
>> +
>> +	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
>> +			     GFP_KERNEL);
>> +	if (!event_hash) {
>> +		printk(KERN_WARNING
>> +		       "snet: can't alloc memory for event_hash\n");
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	for (i = 0; i < event_hash_size; i++)
>> +		INIT_LIST_HEAD(&(event_hash[i]));
>
> No need for double braces.

again fixed.

thanks for your time Evgeniy,
sam

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> This patch adds the snet's subsystem responsive of managing events
> 
> snet is using the word 'event' for a couple of values [syscall, protocol]. For
> example, [listen, tcp] or [sendmsg, dccp] are events.
> This patch introduces a hastable 'event_hash' and operations (add/remove/search..)
> in order to manage which events have to be protected.
> With the help of the communication's subsystem, managing orders are coming from
> userspace.
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---
>  security/snet/include/snet_event.h |   20 +++
>  security/snet/snet_event.c         |  229 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 249 insertions(+), 0 deletions(-)
>  create mode 100644 security/snet/include/snet_event.h
>  create mode 100644 security/snet/snet_event.c
> 
> diff --git a/security/snet/include/snet_event.h b/security/snet/include/snet_event.h
> new file mode 100644
> index 0000000..2c71ca7
> --- /dev/null
> +++ b/security/snet/include/snet_event.h
> @@ -0,0 +1,20 @@
> +#ifndef _SNET_EVENT_H
> +#define _SNET_EVENT_H
> +#include <linux/skbuff.h>
> +
> +extern unsigned int event_hash_size;
> +
> +/* manipulate the events hash table */
> +int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb);
> +int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol);
> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol);
> +int snet_event_remove(const enum snet_syscall syscall, const u8 protocol);
> +void snet_event_flush(void);
> +void snet_event_dumpall(void);
> +
> +/* init function */
> +int snet_event_init(void);
> +/* exit funtion */
> +int snet_event_exit(void);
> +
> +#endif /* _SNET_EVENT_H */
> diff --git a/security/snet/snet_event.c b/security/snet/snet_event.c
> new file mode 100644
> index 0000000..6ac5646
> --- /dev/null
> +++ b/security/snet/snet_event.c
> @@ -0,0 +1,229 @@
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/jhash.h>
> +#include <linux/slab.h>
> +#include <linux/netlink.h>
> +
> +#include "snet.h"
> +#include "snet_event.h"
> +#include "snet_netlink.h"
> +
> +static struct list_head *event_hash;
> +static rwlock_t event_hash_lock = __RW_LOCK_UNLOCKED();
> +
> +struct snet_event_entry {
> +	struct list_head list;
> +	struct snet_event se;
> +};
> +
> +/* lookup for a event_hash - before using this function, lock event_hash_lock */
> +static struct snet_event_entry *__snet_event_lookup(const enum snet_syscall syscall,
> +						   const u8 protocol)
> +{
> +	unsigned int h = 0;
> +	struct list_head *l;
> +	struct snet_event_entry *s;
> +	struct snet_event t;
> +
> +	if (!event_hash)
> +		return NULL;
> +
> +	/* building the element to look for */
> +	t.syscall = syscall;
> +	t.protocol = protocol;
> +
> +	/* computing its hash value */
> +	h = jhash(&t, sizeof(struct snet_event), 0) % event_hash_size;
> +	l = &event_hash[h];
> +
> +	list_for_each_entry(s, l, list) {
> +		if ((s->se.protocol == protocol) &&
> +		    (s->se.syscall == syscall)) {
> +			return s;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	unsigned int i = 0, n = 0;
> +	int ret = -1;
> +	unsigned hashs_to_skip = cb->args[0];
> +	unsigned events_to_skip = cb->args[1];
> +	struct list_head *l;
> +	struct snet_event_entry *s;
> +
> +	read_lock_bh(&event_hash_lock);
> +
> +	if (!event_hash)
> +		goto errout;
> +
> +	for (i = 0; i < event_hash_size; i++) {
> +		if (i < hashs_to_skip)
> +			continue;

What is this?

> +		l = &event_hash[i];
> +		n = 0;
> +		list_for_each_entry(s, l, list) {
> +			if (++n < events_to_skip)
> +				continue;
> +			ret = snet_nl_list_fill_info(skb,
> +						     NETLINK_CB(cb->skb).pid,
> +						     cb->nlh->nlmsg_seq,
> +						     NLM_F_MULTI,
> +						     s->se.protocol,
> +						     s->se.syscall);
> +			if (ret < 0)
> +				goto errout;

So if it returns 0, presumably meaning successfully handled, you
want to go on processing any duplicates?

> +		}
> +	}
> +
> +errout:
> +	read_unlock_bh(&event_hash_lock);
> +
> +	cb->args[0] = i;
> +	cb->args[1] = n;
> +	return skb->len;
> +}
> +
> +/* void snet_event_dumpall() */
> +/* { */
> +/*	unsigned int i = 0; */
> +/*	struct list_head *l; */
> +/*	struct snet_event_entry *s; */
> +
> +/*	snet_dbg("entering\n"); */
> +/*	read_lock_bh(&event_hash_lock); */
> +/*	for (i = 0; i < (event_hash_size - 1); i++) { */
> +/*		l = &hash[i]; */
> +/*		list_for_each_entry(s, l, list) { */
> +/*			snet_dbg("[%d, %d, %d]\n", i, */
> +/*				 s->se.protocol, s->se.syscall); */
> +/*		} */
> +/*	} */
> +/*	read_unlock_bh(&event_hash_lock); */
> +/*	snet_dbg("exiting\n"); */
> +/*	return; */
> +/* } */
> +
> +/*
> + * check if a event is registered or not
> + * return 1 if event is registered, 0 if not
> + */
> +int snet_event_is_registered(const enum snet_syscall syscall, const u8 protocol)
> +{
> +	int ret = 0;
> +
> +	read_lock_bh(&event_hash_lock);
> +	if (__snet_event_lookup(syscall, protocol) != NULL)
> +		ret = 1;
> +	read_unlock_bh(&event_hash_lock);
> +	return ret;
> +}
> +
> +/* adding a event */
> +int snet_event_insert(const enum snet_syscall syscall, const u8 protocol)
> +{
> +	struct snet_event_entry *data = NULL;
> +	unsigned int h = 0;
> +
> +	data = kzalloc(sizeof(struct snet_event_entry), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	write_lock_bh(&event_hash_lock);
> +	/* check if event is already registered */
> +	if (!event_hash || __snet_event_lookup(syscall, protocol) != NULL) {
> +		write_unlock_bh(&event_hash_lock);
> +		kfree(data);
> +		return -EINVAL;

-EBUSY to help debugging?

> +	}
> +
> +	data->se.syscall = syscall;
> +	data->se.protocol = protocol;
> +	INIT_LIST_HEAD(&(data->list));
> +	h = jhash(&(data->se), sizeof(struct snet_event), 0) % event_hash_size;
> +	list_add_tail(&data->list, &event_hash[h]);
> +	write_unlock_bh(&event_hash_lock);
> +
> +	return 0;
> +}
> +
> +/* removing a event */
> +int snet_event_remove(const enum snet_syscall syscall, const u8 protocol)
> +{
> +	struct snet_event_entry *data = NULL;
> +
> +	write_lock_bh(&event_hash_lock);
> +	data = __snet_event_lookup(syscall, protocol);
> +	if (data == NULL) {
> +		write_unlock_bh(&event_hash_lock);
> +		return -EINVAL;
> +	}
> +
> +	list_del(&data->list);
> +	write_unlock_bh(&event_hash_lock);
> +	kfree(data);
> +	return 0;
> +}
> +
> +/* flushing all events */
> +void __snet_event_flush(void)
> +{
> +	struct snet_event_entry *data = NULL;
> +	unsigned int i = 0;
> +
> +	for (i = 0; i < event_hash_size; i++) {
> +		while (!list_empty(&event_hash[i])) {
> +			data = list_entry(event_hash[i].next,
> +					  struct snet_event_entry, list);
> +			list_del(&data->list);
> +			kfree(data);
> +		}
> +	}
> +	return;
> +}
> +
> +void snet_event_flush(void)
> +{
> +	write_lock_bh(&event_hash_lock);
> +	if (event_hash)
> +		__snet_event_flush();
> +	write_unlock_bh(&event_hash_lock);
> +	return;
> +}
> +
> +/* init function */
> +int snet_event_init(void)
> +{
> +	int err = 0, i = 0;
> +
> +	event_hash = kzalloc(sizeof(struct list_head) * event_hash_size,
> +			     GFP_KERNEL);
> +	if (!event_hash) {
> +		printk(KERN_WARNING
> +		       "snet: can't alloc memory for event_hash\n");
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < event_hash_size; i++)
> +		INIT_LIST_HEAD(&(event_hash[i]));
> +
> +out:
> +	return err;
> +}
> +
> +/* exit function */
> +int snet_event_exit(void)
> +{
> +	write_lock_bh(&event_hash_lock);
> +	if (event_hash) {
> +		__snet_event_flush();
> +		kfree(event_hash);
> +		event_hash = NULL;
> +	}
> +	write_unlock_bh(&event_hash_lock);
> +
> +	return 0;
> +}
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Samir Bellabes (sam@synack.fr):
>> +int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
>> +{
>> +	unsigned int i = 0, n = 0;
>> +	int ret = -1;
>> +	unsigned hashs_to_skip = cb->args[0];
>> +	unsigned events_to_skip = cb->args[1];
>> +	struct list_head *l;
>> +	struct snet_event_entry *s;
>> +
>> +	read_lock_bh(&event_hash_lock);
>> +
>> +	if (!event_hash)
>> +		goto errout;
>> +
>> +	for (i = 0; i < event_hash_size; i++) {
>> +		if (i < hashs_to_skip)
>> +			continue;
>
> What is this?

code was duplicated from ctrl_dumpfamily() at net/netlink/genetlink.c
this can be optimized by:
        for (i = hashs_to_skip; i < event_hash_size; i++) {

I will made a patch for ctrl_dumpfamily() right now.

>> +		l = &event_hash[i];
>> +		n = 0;
>> +		list_for_each_entry(s, l, list) {
>> +			if (++n < events_to_skip)
>> +				continue;
>> +			ret = snet_nl_list_fill_info(skb,
>> +						     NETLINK_CB(cb->skb).pid,
>> +						     cb->nlh->nlmsg_seq,
>> +						     NLM_F_MULTI,
>> +						     s->se.protocol,
>> +						     s->se.syscall);
>> +			if (ret < 0)
>> +				goto errout;
>
> So if it returns 0, presumably meaning successfully handled, you
> want to go on processing any duplicates?

first, I found a bug in snet_nl_list_fill_info() which was returning 0
instead of -EMSGSIZE in case there was not enough space to put data.

I'm not sure to understand what may have duplicates, but if you are
talking about the events (struct snet_event_entry), that is not possible
as the insert function checks if the event is already in the hashtable
snet_evh before insertion.

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> > Quoting Samir Bellabes (sam@synack.fr):
> >> +int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
> >> +{
> >> +	unsigned int i = 0, n = 0;
> >> +	int ret = -1;
> >> +	unsigned hashs_to_skip = cb->args[0];
> >> +	unsigned events_to_skip = cb->args[1];
> >> +	struct list_head *l;
> >> +	struct snet_event_entry *s;
> >> +
> >> +	read_lock_bh(&event_hash_lock);
> >> +
> >> +	if (!event_hash)
> >> +		goto errout;
> >> +
> >> +	for (i = 0; i < event_hash_size; i++) {
> >> +		if (i < hashs_to_skip)
> >> +			continue;
> >
> > What is this?
> 
> code was duplicated from ctrl_dumpfamily() at net/netlink/genetlink.c
> this can be optimized by:
>         for (i = hashs_to_skip; i < event_hash_size; i++) {

Sure, but my question was more general (more naive?) - what are the
hashs_to_skip?

sounds like i should be able to go read the genetlink code for an
answer, thanks.

> I will made a patch for ctrl_dumpfamily() right now.
> 
> >> +		l = &event_hash[i];
> >> +		n = 0;
> >> +		list_for_each_entry(s, l, list) {
> >> +			if (++n < events_to_skip)
> >> +				continue;
> >> +			ret = snet_nl_list_fill_info(skb,
> >> +						     NETLINK_CB(cb->skb).pid,
> >> +						     cb->nlh->nlmsg_seq,
> >> +						     NLM_F_MULTI,
> >> +						     s->se.protocol,
> >> +						     s->se.syscall);
> >> +			if (ret < 0)
> >> +				goto errout;
> >
> > So if it returns 0, presumably meaning successfully handled, you
> > want to go on processing any duplicates?
> 
> first, I found a bug in snet_nl_list_fill_info() which was returning 0
> instead of -EMSGSIZE in case there was not enough space to put data.
> 
> I'm not sure to understand what may have duplicates, but if you are
> talking about the events (struct snet_event_entry), that is not possible
> as the insert function checks if the event is already in the hashtable
> snet_evh before insertion.

Ok, but the way your loop is constructed, if snet_nl_list_fill_info()
returns 0 (success, presumably) you won't break.  Sounds like you want
to.

-serge

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes]

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Samir Bellabes (sam@synack.fr):
>> "Serge E. Hallyn" <serue@us.ibm.com> writes:
>> 
>> > Quoting Samir Bellabes (sam@synack.fr):
>> >> +int snet_event_fill_info(struct sk_buff *skb, struct netlink_callback *cb)
>> >> +{
>> >> +	unsigned int i = 0, n = 0;
>> >> +	int ret = -1;
>> >> +	unsigned hashs_to_skip = cb->args[0];
>> >> +	unsigned events_to_skip = cb->args[1];
>> >> +	struct list_head *l;
>> >> +	struct snet_event_entry *s;
>> >> +
>> >> +	read_lock_bh(&event_hash_lock);
>> >> +
>> >> +	if (!event_hash)
>> >> +		goto errout;
>> >> +
>> >> +	for (i = 0; i < event_hash_size; i++) {
>> >> +		if (i < hashs_to_skip)
>> >> +			continue;
>> >
>> > What is this?
>> 
>> code was duplicated from ctrl_dumpfamily() at net/netlink/genetlink.c
>> this can be optimized by:
>>         for (i = hashs_to_skip; i < event_hash_size; i++) {
>
> Sure, but my question was more general (more naive?) - what are the
> hashs_to_skip?
>
> sounds like i should be able to go read the genetlink code for an
> answer, thanks.

if it's not too late.
snet_nl_list(), in snet_netlink.c, is defined as a dumpit() genetlink
operation. A dumpit() operations is called when NLM_DUMP message flag is
set.

the genetlink dumpit() operation receives, as a argument, a
pre-allocated sk_buff buffer, so the purpose of snet_nl_list() is to
fill this buffer. snet_nl_list() is calling snet_event_fill_info() for
this job.

as long as snet_event_fill_info() is not returning 0, it means that it
fills data in the sk_buff, and the dumpit() operations is called again.

the behaviour of sending event list informations is to send a netlink
message by event - remember a event is [syscall, protocol]. so we are
iterate other the hashtable snet_evh:

  [0]               -> list_head: [sys0, proto1], [sys1, proto1]
  [1]               -> list_head: .. 
  [2]               -> list_head: ..
  ...
  [snet_evh_size-1] -> list_head: ..

in order to fill each the pre-allocated sk_buff.

so at each time the snet_event_fill_info() is called again, because of
returning values different from 0, we need to skip previous events which
were already filled in the buffer.
this is the purpose of 

	unsigned hashs_to_skip = cb->args[0];
	unsigned events_to_skip = cb->args[1];

to get the last event filled, and the purpose of :

        cb->args[0] = i;
        cb->args[1] = n;

to store the last event filled, at each call of dumpit()

at this point you may understand the purpose to skip the previous values
in the hashtables, and after the events values at each list.

>> I will made a patch for ctrl_dumpfamily() right now.
>> 
>> >> +		l = &event_hash[i];
>> >> +		n = 0;
>> >> +		list_for_each_entry(s, l, list) {
>> >> +			if (++n < events_to_skip)
>> >> +				continue;
>> >> +			ret = snet_nl_list_fill_info(skb,
>> >> +						     NETLINK_CB(cb->skb).pid,
>> >> +						     cb->nlh->nlmsg_seq,
>> >> +						     NLM_F_MULTI,
>> >> +						     s->se.protocol,
>> >> +						     s->se.syscall);
>> >> +			if (ret < 0)
>> >> +				goto errout;
>> >
>> > So if it returns 0, presumably meaning successfully handled, you
>> > want to go on processing any duplicates?
>> 
>> first, I found a bug in snet_nl_list_fill_info() which was returning 0
>> instead of -EMSGSIZE in case there was not enough space to put data.
>> 
>> I'm not sure to understand what may have duplicates, but if you are
>> talking about the events (struct snet_event_entry), that is not possible
>> as the insert function checks if the event is already in the hashtable
>> snet_evh before insertion.
>
> Ok, but the way your loop is constructed, if snet_nl_list_fill_info()
> returns 0 (success, presumably) you won't break.  Sounds like you want
> to.

No I don't, if it returns 0, we proceed the next event in the current
list.
once the list is empty, we proceed the next hashtable entry.
once the index running other the hashtable is egal to the hashtable size,
it's done.

sam

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Samir Bellabes]

Samir Bellabes <sam@synack.fr> writes:


> No I don't, if it returns 0, we proceed the next event in the current
> list.

To be sure you shall understand: No I don't want to break.

Re: [RFC 5/9] snet: introduce snet_event.c and snet_event.h

[Serge E. Hallyn]

Quoting Samir Bellabes (sam@synack.fr):
> Samir Bellabes <sam@synack.fr> writes:
> 
> 
> > No I don't, if it returns 0, we proceed the next event in the current
> > list.
> 
> To be sure you shall understand: No I don't want to break.

Thanks :)

-serge

[RFC 6/9] snet: introduce snet_hooks.c and snet_hook.h

[Samir Bellabes]

This patch adds the snet LSM's subsystem

snet_hooks provides the security hook's functions and the security_operations
structure. Currently hook functions are only related to network stack.

For each hook function, there is a generic mecanism:
 0. check if the event [syscall, protocol] is registered
 1. prepare informations for userspace
 2. send informations to userspace (snet_netlink)
 3. wait for verdict from userspace (snet_verdict)
 4. apply verdict for the syscall

steps 3 and 4 are only valid for LSM hooks which are returning a value (a way to
'filter' the syscall). For hooks returning 'void', steps 3 and 4 don't exist,
but snet sends security informations to userspace (step 2) to update the global
security policy.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet_hooks.h |   28 ++
 security/snet/snet_hooks.c         |  686 ++++++++++++++++++++++++++++++++++++
 2 files changed, 714 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet_hooks.h
 create mode 100644 security/snet/snet_hooks.c

diff --git a/security/snet/include/snet_hooks.h b/security/snet/include/snet_hooks.h
new file mode 100644
index 0000000..fbda5ed
--- /dev/null
+++ b/security/snet/include/snet_hooks.h
@@ -0,0 +1,28 @@
+#ifndef _SNET_HOOKS_H
+#define _SNET_HOOKS_H
+
+extern atomic_t snet_num_listeners;
+extern unsigned int snet_verdict_policy;
+
+enum snet_syscall {
+	SNET_SOCKET_CREATE = 0,
+	SNET_SOCKET_BIND,
+	SNET_SOCKET_CONNECT,
+	SNET_SOCKET_LISTEN,
+	SNET_SOCKET_ACCEPT,
+	SNET_SOCKET_POST_ACCEPT,
+	SNET_SOCKET_SENDMSG,
+	SNET_SOCKET_RECVMSG,
+	SNET_SOCKET_SOCK_RCV_SKB,
+	SNET_SOCKET_CLOSE,
+	SNET_SOCKET_INVALID,
+};
+
+#define SNET_NR_SOCKET_TYPES SNET_SOCKET_INVALID
+
+/* init function */
+int snet_hooks_init(void);
+/* exit function */
+int snet_hooks_exit(void);
+
+#endif /* _SNET_HOOK_H */
diff --git a/security/snet/snet_hooks.c b/security/snet/snet_hooks.c
new file mode 100644
index 0000000..9c249ab
--- /dev/null
+++ b/security/snet/snet_hooks.c
@@ -0,0 +1,686 @@
+/*
+ * snet_hook.c
+ *
+ * here are interesting informations which can be picked up from hooks.
+ *
+ *
+ * SOCKET_CREATE:
+ *	family, type, protocol
+ * SOCKET_BIND:
+ *	family, protocol, saddr, sport
+ * SOCKET_CONNECT:
+ *	family, protocol, saddr, sport, daddr, dport
+ * SOCKET_LISTEN:
+ *	family, protocol, saddr, sport
+ * SOCKET_ACCEPT:
+ *	family, protocol, saddr, sport
+ *
+ * SOCKET_SENDMSG:
+ * SOCKET_RECVMSG:
+ * SOCKET_SOCK_RCV_SKB:
+ *
+ *
+ */
+
+#include <linux/skbuff.h>
+#include <linux/security.h>
+#include <linux/net.h>
+#include <net/sock.h>
+#include <linux/in.h>
+#include <linux/in6.h>
+#include <net/inet_sock.h>
+#include <linux/ipv6.h>
+
+#include "snet.h"
+#include "snet_hooks.h"
+#include "snet_verdict.h"
+#include "snet_netlink.h"
+#include "snet_event.h"
+
+#define SNET_DBG_V4()							\
+	snet_dbg("%u.%u.%u.%u:%u->%u.%u.%u.%u:%u\n",			\
+		 NIPQUAD(info.src.u3.ip), info.src.u.port,		\
+		 NIPQUAD(info.dst.u3.ip), info.dst.u.port)
+
+#define SNET_DBG_V6()							\
+	snet_dbg("%pI6:%u->%pI6:%u\n",					\
+		 &info.src.u3.ip, info.src.u.port,			\
+		 &info.dst.u3.ip, info.dst.u.port)
+
+#define SNET_CHECK_LISTENERS()						\
+do {									\
+	if (atomic_read(&snet_num_listeners) < 0) {			\
+		snet_dbg("number of listeners is negative\n");		\
+		verdict = SNET_VERDICT_GRANT;				\
+		goto out;						\
+	} else if (atomic_read(&snet_num_listeners) == 0) {		\
+		verdict = SNET_VERDICT_GRANT;				\
+		goto out;						\
+	}								\
+} while (0)
+
+#define SNET_DO_VERDICT(sys, family)					\
+do {									\
+	if (verdict_id == 0)						\
+		goto skip_send_wait;					\
+	/* sending networking informations to userspace */		\
+	snet_nl_send_event(verdict_id, sys, protocol,			\
+			   family, (void *)&info,			\
+			   sizeof(struct snet_sock_info));		\
+	/* waiting for userspace reply or timeout */			\
+	verdict = snet_verdict_wait(verdict_id);			\
+		/* removing verdict */					\
+	snet_verdict_remove(verdict_id);				\
+} while (0)
+
+#define SNET_CHECK_LISTENERS_NOVERDICT()				\
+do {									\
+	if (atomic_read(&snet_num_listeners) < 0) {			\
+		snet_dbg("number of listeners is negative\n");		\
+		goto out;						\
+	} else if (atomic_read(&snet_num_listeners) == 0) {		\
+		goto out;						\
+	}								\
+} while (0)
+
+#define SNET_DO_SEND_EVENT(sys, family)					\
+do {									\
+	/* sending networking informations to userspace */		\
+	snet_nl_send_event(0, sys, protocol,				\
+			   family, (void *)&info,			\
+			   sizeof(struct snet_sock_info));		\
+} while (0)
+
+/*
+ * security operations helper functions
+ */
+
+/*
+ * security operations functions members
+ */
+
+static int snet_socket_create(int family, int type, int protocol, int kern)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+
+	/* if (kern) */
+	/*	;		/\* do something smart *\/ */
+
+	SNET_CHECK_LISTENERS();
+
+	if (snet_event_is_registered(SNET_SOCKET_CREATE, protocol)) {
+		struct snet_sock_info info;
+
+		/* inserting verdict PENDING */
+		verdict_id = snet_verdict_insert();
+
+		/* prepare networking informations for userspace */
+		info.type = type;
+
+		snet_dbg("family=%u type=%u protocol=%u kern=%u\n",
+			 family, type, protocol, kern);
+
+		SNET_DO_VERDICT(SNET_SOCKET_CREATE, family);
+
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+
+skip_send_wait:
+out:
+	return verdict;
+}
+
+static int snet_socket_bind(struct socket *sock,
+			    struct sockaddr *address, int addrlen)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_BIND, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+		struct sockaddr_in *a = (struct sockaddr_in *) address;
+		struct sockaddr_in6 *a6 = (struct sockaddr_in6 *) address;
+
+		/* prepare networking informations for userspace */
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = a->sin_addr.s_addr;
+			info.dst.u3.ip = inet->inet_daddr;
+			info.src.u.port = ntohs(a->sin_port);
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&a6->sin6_addr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+			info.src.u.port = ntohs(a6->sin6_port);
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_BIND, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static int snet_socket_connect(struct socket *sock,
+			       struct sockaddr *address, int addrlen)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_CONNECT, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+		struct sockaddr_in *a = (struct sockaddr_in *) address;
+		struct sockaddr_in6 *a6 = (struct sockaddr_in6 *) address;
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = a->sin_addr.s_addr;
+			info.dst.u.port = ntohs(a->sin_port);
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&a6->sin6_addr,
+			       sizeof(info.dst.u3.ip6));
+			info.dst.u.port = ntohs(a6->sin6_port);
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_CONNECT, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static int snet_socket_listen(struct socket *sock, int backlog)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_LISTEN, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_LISTEN, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static int snet_socket_accept(struct socket *sock, struct socket *newsock)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_ACCEPT, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_ACCEPT, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static void snet_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS_NOVERDICT();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_POST_ACCEPT, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(newsock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			goto out;
+			break;
+		}
+
+		SNET_DO_SEND_EVENT(SNET_SOCKET_POST_ACCEPT, sock->sk->sk_family);
+	}
+out:
+	return;
+}
+
+static int snet_socket_sendmsg(struct socket *sock,
+			       struct msghdr *msg, int size)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_SENDMSG, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_SENDMSG, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static int snet_socket_recvmsg(struct socket *sock,
+			       struct msghdr *msg, int size, int flags)
+{
+	u32 verdict_id = 0;
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_RECVMSG, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING  */
+			verdict_id = snet_verdict_insert();
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		SNET_DO_VERDICT(SNET_SOCKET_RECVMSG, sock->sk->sk_family);
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static int snet_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
+{
+	/* u32 verdict_id = 0; */
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	u8 protocol = 0;
+
+	SNET_CHECK_LISTENERS();
+
+	protocol = sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_SOCK_RCV_SKB, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sk->sk_family) {
+		case PF_INET:
+			/* inserting verdict PENDING  */
+			/* verdict_id = snet_verdict_insert(); */
+
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			/* inserting verdict PENDING  */
+			/* verdict_id = snet_verdict_insert(); */
+
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			verdict = SNET_VERDICT_NONE;
+			goto skip_send_wait;
+			break;
+		}
+
+		/* SNET_DOC_VERDICT(SNET_SOCKET_SOCK_RCV_SKB, sk->sk_family); */
+	} else {
+		verdict = SNET_VERDICT_GRANT;
+	}
+
+skip_send_wait:
+	if (verdict == SNET_VERDICT_NONE)
+		verdict = snet_verdict_policy;
+out:
+	return verdict;
+}
+
+static void snet_socket_close(struct socket *sock)
+{
+	u8 protocol = 0;
+
+	if (sock == NULL || sock->sk == NULL) {
+		goto out;
+	}
+
+	SNET_CHECK_LISTENERS_NOVERDICT();
+
+	protocol = sock->sk->sk_protocol;
+
+	if (snet_event_is_registered(SNET_SOCKET_CLOSE, protocol)) {
+		struct snet_sock_info info;
+		struct inet_sock *inet = inet_sk(sock->sk);
+
+		/* prepare networking informations for userspace */
+		info.src.u.port = ntohs(inet->inet_sport);
+		info.dst.u.port = ntohs(inet->inet_dport);
+
+		switch (sock->sk->sk_family) {
+		case PF_INET:
+			info.src.u3.ip = inet->inet_saddr;
+			info.dst.u3.ip = inet->inet_daddr;
+
+			SNET_DBG_V4();
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case PF_INET6:
+			memcpy(&info.src.u3.ip6, (void *)&inet->pinet6->saddr,
+			       sizeof(info.src.u3.ip6));
+			memcpy(&info.dst.u3.ip6, (void *)&inet->pinet6->daddr,
+			       sizeof(info.dst.u3.ip6));
+
+			SNET_DBG_V6();
+			break;
+#endif
+		default:
+			goto out;
+			break;
+		}
+
+		SNET_DO_SEND_EVENT(SNET_SOCKET_CLOSE, sock->sk->sk_family);
+	}
+out:
+	return;
+}
+
+#undef SNET_DBG_V4
+#undef SNET_DBG_V6
+#undef SNET_CHECK_LISTENERS
+
+static struct security_operations snet_security_ops = {
+	.name			= "snet",
+
+	.socket_create		= snet_socket_create,
+	.socket_bind		= snet_socket_bind,
+	.socket_connect		= snet_socket_connect,
+	.socket_listen		= snet_socket_listen,
+	.socket_accept		= snet_socket_accept,
+	.socket_post_accept	= snet_socket_post_accept,
+	.socket_sendmsg		= snet_socket_sendmsg,
+	.socket_recvmsg		= snet_socket_recvmsg,
+	.socket_sock_rcv_skb	= snet_socket_sock_rcv_skb,
+	.socket_close		= snet_socket_close,
+};
+
+int snet_hooks_init(void)
+{
+	if (!security_module_enable(&snet_security_ops))
+		return 0;
+
+	if (register_security(&snet_security_ops))
+		panic("snet: failed to register security_ops\n");
+
+	return 0;
+}
+
+int snet_hooks_exit(void)
+{
+	return 0;
+}
-- 
1.6.3.3

Re: [RFC 6/9] snet: introduce snet_hooks.c and snet_hook.h

[Evgeniy Polyakov]

Hi.

On Sat, Jan 02, 2010 at 02:04:13PM +0100, Samir Bellabes (sam@synack.fr) wrote:
> This patch adds the snet LSM's subsystem
> 
> snet_hooks provides the security hook's functions and the security_operations
> structure. Currently hook functions are only related to network stack.
> 
> For each hook function, there is a generic mecanism:
>  0. check if the event [syscall, protocol] is registered
>  1. prepare informations for userspace
>  2. send informations to userspace (snet_netlink)
>  3. wait for verdict from userspace (snet_verdict)
>  4. apply verdict for the syscall
> 
> steps 3 and 4 are only valid for LSM hooks which are returning a value (a way to
> 'filter' the syscall). For hooks returning 'void', steps 3 and 4 don't exist,
> but snet sends security informations to userspace (step 2) to update the global
> security policy.
> 
> Signed-off-by: Samir Bellabes <sam@synack.fr>
> ---

> +#define SNET_DBG_V4()							\
> +	snet_dbg("%u.%u.%u.%u:%u->%u.%u.%u.%u:%u\n",			\
> +		 NIPQUAD(info.src.u3.ip), info.src.u.port,		\
> +		 NIPQUAD(info.dst.u3.ip), info.dst.u.port)
> +
> +#define SNET_DBG_V6()							\
> +	snet_dbg("%pI6:%u->%pI6:%u\n",					\
> +		 &info.src.u3.ip, info.src.u.port,			\
> +		 &info.dst.u3.ip, info.dst.u.port)
> +

There are cool %pi4 and %pi6 format modifiers for IP addresses.

> +#define SNET_CHECK_LISTENERS()						\
> +do {									\
> +	if (atomic_read(&snet_num_listeners) < 0) {			\
> +		snet_dbg("number of listeners is negative\n");		\
> +		verdict = SNET_VERDICT_GRANT;				\
> +		goto out;						\
> +	} else if (atomic_read(&snet_num_listeners) == 0) {		\
> +		verdict = SNET_VERDICT_GRANT;				\
> +		goto out;						\
> +	}								\
> +} while (0)
> +
> +#define SNET_DO_VERDICT(sys, family)					\
> +do {									\
> +	if (verdict_id == 0)						\
> +		goto skip_send_wait;					\
> +	/* sending networking informations to userspace */		\
> +	snet_nl_send_event(verdict_id, sys, protocol,			\
> +			   family, (void *)&info,			\
> +			   sizeof(struct snet_sock_info));		\
> +	/* waiting for userspace reply or timeout */			\
> +	verdict = snet_verdict_wait(verdict_id);			\
> +		/* removing verdict */					\
> +	snet_verdict_remove(verdict_id);				\
> +} while (0)
> +
> +#define SNET_CHECK_LISTENERS_NOVERDICT()				\
> +do {									\
> +	if (atomic_read(&snet_num_listeners) < 0) {			\
> +		snet_dbg("number of listeners is negative\n");		\
> +		goto out;						\
> +	} else if (atomic_read(&snet_num_listeners) == 0) {		\
> +		goto out;						\
> +	}								\
> +} while (0)
> +
> +#define SNET_DO_SEND_EVENT(sys, family)					\
> +do {									\
> +	/* sending networking informations to userspace */		\
> +	snet_nl_send_event(0, sys, protocol,				\
> +			   family, (void *)&info,			\
> +			   sizeof(struct snet_sock_info));		\
> +} while (0)
> +

Why do you need those ugly macroses which reference external objects?
Can it be refactored into some inline function with common error value
instead?


-- 
	Evgeniy Polyakov

Re: [RFC 6/9] snet: introduce snet_hooks.c and snet_hook.h

[Samir Bellabes]

hello Evgeniy

Evgeniy Polyakov <zbr@ioremap.net> writes:

> Hi.
>
> On Sat, Jan 02, 2010 at 02:04:13PM +0100, Samir Bellabes (sam@synack.fr) wrote:
>> This patch adds the snet LSM's subsystem
>> 
>> snet_hooks provides the security hook's functions and the security_operations
>> structure. Currently hook functions are only related to network stack.
>> 
>> For each hook function, there is a generic mecanism:
>>  0. check if the event [syscall, protocol] is registered
>>  1. prepare informations for userspace
>>  2. send informations to userspace (snet_netlink)
>>  3. wait for verdict from userspace (snet_verdict)
>>  4. apply verdict for the syscall
>> 
>> steps 3 and 4 are only valid for LSM hooks which are returning a value (a way to
>> 'filter' the syscall). For hooks returning 'void', steps 3 and 4 don't exist,
>> but snet sends security informations to userspace (step 2) to update the global
>> security policy.
>> 
>> Signed-off-by: Samir Bellabes <sam@synack.fr>
>> ---
>
>> +#define SNET_DBG_V4()							\
>> +	snet_dbg("%u.%u.%u.%u:%u->%u.%u.%u.%u:%u\n",			\
>> +		 NIPQUAD(info.src.u3.ip), info.src.u.port,		\
>> +		 NIPQUAD(info.dst.u3.ip), info.dst.u.port)
>> +
>> +#define SNET_DBG_V6()							\
>> +	snet_dbg("%pI6:%u->%pI6:%u\n",					\
>> +		 &info.src.u3.ip, info.src.u.port,			\
>> +		 &info.dst.u3.ip, info.dst.u.port)
>> +
>
> There are cool %pi4 and %pi6 format modifiers for IP addresses.

I moved %u.%u.%u.%u to %pI4
thanks

>> +#define SNET_CHECK_LISTENERS()						\
>> +do {									\
>> +	if (atomic_read(&snet_num_listeners) < 0) {			\
>> +		snet_dbg("number of listeners is negative\n");		\
>> +		verdict = SNET_VERDICT_GRANT;				\
>> +		goto out;						\
>> +	} else if (atomic_read(&snet_num_listeners) == 0) {		\
>> +		verdict = SNET_VERDICT_GRANT;				\
>> +		goto out;						\
>> +	}								\
>> +} while (0)
>> +
>> +#define SNET_DO_VERDICT(sys, family)					\
>> +do {									\
>> +	if (verdict_id == 0)						\
>> +		goto skip_send_wait;					\
>> +	/* sending networking informations to userspace */		\
>> +	snet_nl_send_event(verdict_id, sys, protocol,			\
>> +			   family, (void *)&info,			\
>> +			   sizeof(struct snet_sock_info));		\
>> +	/* waiting for userspace reply or timeout */			\
>> +	verdict = snet_verdict_wait(verdict_id);			\
>> +		/* removing verdict */					\
>> +	snet_verdict_remove(verdict_id);				\
>> +} while (0)
>> +
>> +#define SNET_CHECK_LISTENERS_NOVERDICT()				\
>> +do {									\
>> +	if (atomic_read(&snet_num_listeners) < 0) {			\
>> +		snet_dbg("number of listeners is negative\n");		\
>> +		goto out;						\
>> +	} else if (atomic_read(&snet_num_listeners) == 0) {		\
>> +		goto out;						\
>> +	}								\
>> +} while (0)
>> +
>> +#define SNET_DO_SEND_EVENT(sys, family)					\
>> +do {									\
>> +	/* sending networking informations to userspace */		\
>> +	snet_nl_send_event(0, sys, protocol,				\
>> +			   family, (void *)&info,			\
>> +			   sizeof(struct snet_sock_info));		\
>> +} while (0)
>> +
>
> Why do you need those ugly macroses which reference external objects?
> Can it be refactored into some inline function with common error value
> instead?

I know it's ugly, but code in security hooks are duplicated.
functions (inline or not), which replace this macros, will resulte of
having lots of parameters. macros with external object seems to be the
most simple at this point.

thanks for your time Evgeniy.
sam

Re: [RFC 6/9] snet: introduce snet_hooks.c and snet_hook.h

[Stephen Hemminger]

On Sun, 03 Jan 2010 12:10:53 +0100
Samir Bellabes <sam@synack.fr> wrote:

> I know it's ugly, but code in security hooks are duplicated.
> functions (inline or not), which replace this macros, will resulte of
> having lots of parameters. macros with external object seems to be the
> most simple at this point.

Macro's with references to thing outside of the argument are just
plain wrong. They introduce bugs that humans don't see.

-- 

Re: [RFC 6/9] snet: introduce snet_hooks.c and snet_hook.h

[Samir Bellabes]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Sun, 03 Jan 2010 12:10:53 +0100
> Samir Bellabes <sam@synack.fr> wrote:
>
>> I know it's ugly, but code in security hooks are duplicated.
>> functions (inline or not), which replace this macros, will resulte of
>> having lots of parameters. macros with external object seems to be the
>> most simple at this point.
>
> Macro's with references to thing outside of the argument are just
> plain wrong. They introduce bugs that humans don't see.

right.
I will move this macros to inline functions.

thanks,
sam

[RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

this patch adds the snet communication's subsystem.

snet_netlink is using genetlink for sending/receiving messages to/from userspace.
the genetlink operations permit to receive orders to manage the table of events
- events are values [syscall, protocol] - which is used to know which syscall
and protocol have to be protected. genl operations are also used to manage
communication of events to userspace, and to receive the related verdict.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet_netlink.h |  201 +++++++++++++
 security/snet/snet_netlink.c         |  541 ++++++++++++++++++++++++++++++++++
 2 files changed, 742 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet_netlink.h
 create mode 100644 security/snet/snet_netlink.c

diff --git a/security/snet/include/snet_netlink.h b/security/snet/include/snet_netlink.h
new file mode 100644
index 0000000..d739f66
--- /dev/null
+++ b/security/snet/include/snet_netlink.h
@@ -0,0 +1,201 @@
+#ifndef _SNET_NETLINK_H
+#define _SNET_NETLINK_H
+
+/*
+ * The following payloads are supported.
+ *
+ * o VERSION:
+ *   Sent by an application to verify the snet version.
+ *   When sent by an application, there is no payload.
+ *   When sent by the kernel, it's a response to an VERSION request.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_VERSION
+ *
+ * o REGISTER:
+ *   Sent by an application to notify listening for events.
+ *
+ * o UNREGISTER:
+ *   Sent by an application to notify unlistening for events.
+ *
+ * o INSERT:
+ *   Sent by an application to insert a new event.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_SYSCALL
+ *     SNET_A_PROTOCOL
+ *
+ * o REMOVE
+ *   Sent by an application to remove a event.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_SYSCALL
+ *     SNET_A_PROTOCOL
+ *
+ * o FLUSH
+ *   Sent by an application to flush the events' hashtable
+ *
+ * o LIST
+ *   Sent by an application to list all events on the hashtable.
+ *   When sent by an application there is no payload and the NLM_F_DUMP
+ *   flag should be set. The kernel should respond with a series of
+ *   the following messages.
+ *
+ * o VERDICT
+ *   kernel -> userspace
+ *   Sent by the kernel to notify for a syscall is pending for a
+ *   verdict or to notify for a network event.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_VERDICT_ID
+ *     SNET_A_SYSCALL
+ *     SNET_A_PROTOCOL
+ *     SNET_A_FAMILY
+ *     SNET_A_UID
+ *     SNET_A_PID
+ *
+ *   If using SNET_SOCKET_CREATE
+ *   the following attributes are required:
+ *
+ *     SNET_A_TYPE
+ *
+ *   If using SNET_SOCKET_LISTEN or SNET_SOCKET_BIND or SNET_SOCKET_ACCEPT
+ *   the following attributes are required:
+ *
+ *     SNET_A_SADDR or SNET_A_SADDR6 depending on sk->family
+ *     SNET_A_SPORT
+ *
+ *   If using SNET_SOCKET_CONNECT
+ *   the following attributes are required:
+ *
+ *     SNET_A_SADDR or SNET_A_SADDR6 depending on sk->family
+ *     SNET_A_DADDR or SNET_A_DADDR6 depending on sk->family
+ *     SNET_A_SPORT
+ *     SNET_A_DPORT
+ *
+ *   If using SNET_SOCKET_SENDMSG
+ *   the following attributes are required:
+ *
+ *     SNET_A_SADDR or SNET_A_SADDR6 depending on sk->family
+ *     SNET_A_DADDR or SNET_A_DADDR6 depending on sk->family
+ *     SNET_A_SPORT
+ *     SNET_A_DPORT
+ *     SNET_A_BUFFER_LEN (msg->msg_iov->iov_len)
+ *     SNET_A_BUFFER     (msg->msg_iov->iov_base)
+ *
+ *   If using SNET_SOCKET_RECVMSG
+ *   the following attributes are required:
+ *
+ *     SNET_A_SADDR or SNET_A_SADDR6 depending on sk->family
+ *     SNET_A_DADDR or SNET_A_DADDR6 depending on sk->family
+ *     SNET_A_SPORT
+ *     SNET_A_DPORT
+ *
+ *   If using SNET_SOCKET_SOCK_RCV_SKB
+ *   the following attributes are required:
+ *
+ *     SNET_A_SADDR or SNET_A_SADDR6 depending on sk->family
+ *     SNET_A_DADDR or SNET_A_DADDR6 depending on sk->family
+ *     SNET_A_SPORT
+ *     SNET_A_DPORT
+ *     SNET_A_BUFFER_LEN ()
+ *     SNET_A_BUFFER     ()
+ *
+ *   userspace -> kernel
+ *   Sent by a application to set the verdict for a pending event.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_VERDICT_ID
+ *     SNET_A_VERDICT
+ *
+ * o VERDICT_DELAY
+ *   Sent by an application to set the timeout value for verdicts.
+ *
+ *   Required attributes:
+ *
+ *     SNET_A_VERDICT_DELAY
+ *
+ */
+
+#include <linux/in6.h>
+#include "snet_hooks.h"
+
+extern unsigned int snet_verdict_delay;
+
+/* commands */
+enum {
+	SNET_C_UNSPEC,
+	SNET_C_VERSION,
+	SNET_C_REGISTER,
+	SNET_C_UNREGISTER,
+	SNET_C_INSERT,
+	SNET_C_REMOVE,
+	SNET_C_FLUSH,
+	SNET_C_LIST,
+	SNET_C_VERDICT,
+	SNET_C_VERDICT_DELAY,
+	__SNET_C_MAX,
+};
+
+#define SNET_C_MAX (__SNET_C_MAX - 1)
+
+/* attributes */
+enum {
+	SNET_A_UNSPEC,
+	SNET_A_VERSION,		/* (NLA_U32) the snet protocol version	*/
+	SNET_A_SYSCALL,		/* (NLA_U8)  a syscall identifier	*/
+	SNET_A_PROTOCOL,	/* (NLA_U8)  a protocol identifier	*/
+	SNET_A_INSERTED,
+	SNET_A_REMOVED,
+	SNET_A_FLUSHED,
+	SNET_A_REGISTERED,
+	SNET_A_UNREGISTERED,
+	SNET_A_VERDICT_ID,
+	SNET_A_FAMILY,
+	SNET_A_UID,
+	SNET_A_PID,
+	SNET_A_VERDICT,
+	SNET_A_DATA_EXT,
+	SNET_A_VERDICT_DELAY,
+	SNET_A_VERDICT_DELAYED,
+	__SNET_A_MAX,
+};
+
+#define SNET_A_MAX (__SNET_A_MAX - 1)
+
+#define SNET_GENL_NAME		"SNET"
+#define SNET_GENL_VERSION	SNET_VERSION
+
+int snet_nl_send_event(const u32 verdict_id, const enum snet_syscall syscall,
+		       const u8 protocol, const u8 family, void *data,
+		       const unsigned int len);
+
+int snet_nl_list_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
+			   u32 flags, u8 protocol, enum snet_syscall syscall);
+
+void snet_netlink_exit(void);
+
+struct snet_sock_half {
+	struct {
+		union {
+			__be32 ip;
+			struct in6_addr ip6;
+		};
+	} u3;
+	struct {
+		__be16 port;
+	} u;
+};
+
+struct snet_sock_info {
+	struct snet_sock_half src;
+	struct snet_sock_half dst;
+	int type;
+};
+
+#endif /* _SNET_NETLINK_H */
diff --git a/security/snet/snet_netlink.c b/security/snet/snet_netlink.c
new file mode 100644
index 0000000..cc21d6c
--- /dev/null
+++ b/security/snet/snet_netlink.c
@@ -0,0 +1,541 @@
+#include <linux/sched.h>
+#include <net/genetlink.h>
+#include <linux/in6.h>
+
+#include "snet.h"
+#include "snet_netlink.h"
+#include "snet_verdict.h"
+#include "snet_event.h"
+#include <snet_utils.h>
+
+atomic_t snet_nl_seq = ATOMIC_INIT(0);
+static uint32_t snet_nl_pid;
+static struct genl_family snet_genl_family;
+atomic_t snet_num_listeners = ATOMIC_INIT(0);
+
+/*
+ * snet genetlink
+ */
+int snet_nl_send_event(const u32 verdict_id, const enum snet_syscall syscall,
+		       const u8 protocol, const u8 family, void *data,
+		       const unsigned int len)
+{
+	struct sk_buff *skb_rsp;
+	void *msg_head;
+	int ret = -ENOMEM;
+
+	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb_rsp == NULL)
+		return 0;
+
+	msg_head = genlmsg_put(skb_rsp, snet_nl_pid,
+			       atomic_inc_return(&snet_nl_seq),
+			       &snet_genl_family, 0, SNET_C_VERDICT);
+	if (msg_head == NULL)
+		goto send_event_failure;
+
+	snet_dbg("verdict_id=0x%x syscall=%s protocol=%u "
+		 "family=%u uid=%u pid=%u\n",
+		 verdict_id, snet_syscall_name(syscall),
+		 protocol, family, current_uid(), current->pid);
+
+	if (verdict_id) {
+		ret = nla_put_u32(skb_rsp, SNET_A_VERDICT_ID, verdict_id);
+		if (ret != 0)
+			goto send_event_failure;
+	}
+	ret = nla_put_u8(skb_rsp, SNET_A_SYSCALL, syscall);
+	if (ret != 0)
+		goto send_event_failure;
+	ret = nla_put_u8(skb_rsp, SNET_A_PROTOCOL, protocol);
+	if (ret != 0)
+		goto send_event_failure;
+	ret = nla_put_u8(skb_rsp, SNET_A_FAMILY, family);
+	if (ret != 0)
+		goto send_event_failure;
+	ret = nla_put_u32(skb_rsp, SNET_A_UID, current_uid());
+	if (ret != 0)
+		goto send_event_failure;
+	ret = nla_put_u32(skb_rsp, SNET_A_PID, current->pid);
+	if (ret != 0)
+		goto send_event_failure;
+	ret = nla_put(skb_rsp, SNET_A_DATA_EXT, len, data);
+	if (ret != 0)
+		goto send_event_failure;
+
+	ret = genlmsg_end(skb_rsp, msg_head);
+	if (ret < 0)
+		goto send_event_failure;
+
+	genlmsg_unicast(&init_net, skb_rsp, snet_nl_pid);
+	return 0;
+
+send_event_failure:
+	kfree_skb(skb_rsp);
+	return 0;
+}
+
+/*
+ * snet genetlink helper functions
+ */
+static int snet_nl_response_flag(struct genl_info *info,
+				 struct genl_family *family,
+				 u8 cmd, int attrtype, u8 set_resp_flag)
+{
+	int ret = -EINVAL;
+	struct sk_buff *skb_rsp = NULL;
+	void *msg_head;
+
+	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb_rsp == NULL)
+		return -ENOMEM;
+	msg_head = genlmsg_put_reply(skb_rsp, info, family, 0, cmd);
+	if (msg_head == NULL)
+		goto response_failure;
+
+	/* we put flag only if it is asked */
+	if (set_resp_flag) {
+		ret = nla_put_flag(skb_rsp, attrtype);
+		if (ret != 0)
+			goto response_failure;
+	}
+
+	genlmsg_end(skb_rsp, msg_head);
+	ret = genlmsg_reply(skb_rsp, info);
+	if (ret != 0)
+		goto response_failure;
+	return 0;
+
+response_failure:
+	kfree_skb(skb_rsp);
+	return ret;
+}
+
+/*
+ * snet genetlink functions
+ */
+
+static struct genl_family snet_genl_family = {
+	.id		= GENL_ID_GENERATE,
+	.hdrsize	= 0,
+	.name		= SNET_GENL_NAME,
+	.version	= SNET_GENL_VERSION,
+	.maxattr	= SNET_A_MAX,
+};
+
+static const struct nla_policy snet_genl_policy[SNET_A_MAX + 1]
+__read_mostly = {
+	[SNET_A_VERSION]		= { .type = NLA_U32 },
+	[SNET_A_SYSCALL]		= { .type = NLA_U8 },
+	[SNET_A_PROTOCOL]		= { .type = NLA_U8 },
+	[SNET_A_INSERTED]		= { .type = NLA_FLAG },
+	[SNET_A_REMOVED]		= { .type = NLA_FLAG },
+	[SNET_A_FLUSHED]		= { .type = NLA_FLAG },
+	[SNET_A_REGISTERED]		= { .type = NLA_FLAG },
+	[SNET_A_UNREGISTERED]		= { .type = NLA_FLAG },
+	[SNET_A_VERDICT_ID]		= { .type = NLA_U32 },
+	[SNET_A_FAMILY]			= { .type = NLA_U8 },
+	[SNET_A_UID]			= { .type = NLA_U32 },
+	[SNET_A_PID]			= { .type = NLA_U32 },
+	[SNET_A_VERDICT]		= { .type = NLA_U8 },
+	[SNET_A_DATA_EXT]		= { .type = NLA_BINARY,
+					    .len = sizeof(struct snet_sock_info) },
+	[SNET_A_VERDICT_DELAY]		= { .type = NLA_U32 },
+	[SNET_A_VERDICT_DELAYED]	= { .type = NLA_FLAG },
+};
+
+/**
+ * snet_nl_version - Handle a VERSION message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Process a user generated VERSION message and respond accordingly.
+ * Returns zero on success, negative values on failure.
+ */
+static int snet_nl_version(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret = -ENOMEM;
+	struct sk_buff *skb_rsp = NULL;
+	void *msg_head;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb_rsp == NULL)
+		return -ENOMEM;
+	msg_head = genlmsg_put_reply(skb_rsp, info, &snet_genl_family,
+				     0, SNET_C_VERSION);
+	if (msg_head == NULL)
+		goto version_failure;
+
+	ret = nla_put_u32(skb_rsp, SNET_A_VERSION, SNET_VERSION);
+	if (ret != 0)
+		goto version_failure;
+
+	genlmsg_end(skb_rsp, msg_head);
+
+	ret = genlmsg_reply(skb_rsp, info);
+	if (ret != 0)
+		goto version_failure;
+	return 0;
+
+version_failure:
+	kfree_skb(skb_rsp);
+	return ret;
+}
+
+/**
+ * snet_nl_register - Handle a REGISTER message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Notify the kernel that an application is listening for events.
+ * Returns zero on success, negative values on failure.
+ */
+static int snet_nl_register(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret = -EINVAL;
+	u32 version = 0;
+	u8 set_resp_flag = 0;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (!info->attrs[SNET_A_VERSION])
+		return -EINVAL;
+	version = nla_get_u32(info->attrs[SNET_A_VERSION]);
+
+	if (version == SNET_VERSION) {	/* version is compliant */
+		atomic_inc(&snet_num_listeners);
+		set_resp_flag = 1;
+	}
+
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_REGISTER, SNET_A_REGISTERED,
+				    set_resp_flag);
+
+	snet_nl_pid = info->snd_pid;
+	snet_dbg("pid=%u num_listeners=%d\n",
+		 snet_nl_pid, atomic_read(&snet_num_listeners));
+	return ret;
+}
+
+/**
+ * snet_nl_unregister - Handle a UNREGISTER message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Notify the kernel that the application is no more listening for events.
+ * Returns zero on success, negative values on failure.
+ */
+static int snet_nl_unregister(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret = -EINVAL;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners))
+		atomic_dec(&snet_num_listeners);
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_UNREGISTER, SNET_A_UNREGISTERED, 1);
+	snet_dbg("pid=%u num_listeners=%d\n",
+		 snet_nl_pid, atomic_read(&snet_num_listeners));
+	return ret;
+}
+
+/**
+ * snet_nl_insert - Handle a INSERT message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Insert a new event to the events' hashtable. Returns zero on success,
+ * negative values on failure.
+ */
+static int snet_nl_insert(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret_event = -EINVAL, ret = -EINVAL;
+	enum snet_syscall syscall;
+	u8 protocol;
+	u8 set_resp_flag = 0;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	if (!info->attrs[SNET_A_SYSCALL] || !info->attrs[SNET_A_PROTOCOL])
+		return -EINVAL;
+
+	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
+	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
+	ret_event = snet_event_insert(syscall, protocol);
+	snet_dbg("syscall=%s protocol=%u insert=%s\n",
+		 snet_syscall_name(syscall), protocol,
+		 (ret_event == 0) ? "success" : "failed");
+
+	if (ret_event == 0)
+		set_resp_flag = 1;
+
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_INSERT, SNET_A_INSERTED,
+				    set_resp_flag);
+	return ret;
+}
+
+/**
+ * snet_nl_remove - Handle a REMOVE message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Remove a event from the events' hastable. Returns zero on success,
+ * negative values on failure.
+ */
+static int snet_nl_remove(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret_event = -EINVAL, ret = -EINVAL;
+	enum snet_syscall syscall;
+	u8 protocol;
+	u8 set_resp_flag = 0;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	if (!info->attrs[SNET_A_SYSCALL] || !info->attrs[SNET_A_PROTOCOL])
+		return -EINVAL;
+
+	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
+	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
+	ret_event = snet_event_remove(syscall, protocol);
+	snet_dbg("syscall=%s protocol=%u remove=%s\n",
+		 snet_syscall_name(syscall), protocol,
+		 (ret_event == 0) ? "success" : "failed");
+
+	if (ret_event == 0)
+		set_resp_flag = 1;
+
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_REMOVE, SNET_A_REMOVED,
+				    set_resp_flag);
+	return ret;
+}
+
+/**
+ * snet_nl_flush - Handle a FLUSH message
+ * @skb: the NETLINK buffer
+ * @info: the Generic NETLINK info block
+ *
+ * Description:
+ * Remove all events from the hashtable. Returns zero on success,
+ * negative values on failure.
+ */
+static int snet_nl_flush(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret = -EINVAL;
+	u8 set_resp_flag = 0;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	snet_event_flush();
+
+	set_resp_flag = 1;
+
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_FLUSH, SNET_A_FLUSHED,
+				    set_resp_flag);
+	return ret;
+}
+
+int snet_nl_list_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
+			   u32 flags, u8 protocol, enum snet_syscall syscall)
+{
+	void *hdr;
+	int ret = -1;
+
+	hdr = genlmsg_put(skb, pid, seq, &snet_genl_family, flags, SNET_C_LIST);
+	if (hdr == NULL)
+		return -1;
+
+	ret = nla_put_u8(skb, SNET_A_SYSCALL, syscall);
+	if (ret != 0)
+		goto list_failure;
+
+	ret = nla_put_u8(skb, SNET_A_PROTOCOL, protocol);
+	if (ret != 0)
+		goto list_failure;
+
+	return genlmsg_end(skb, hdr);
+
+list_failure:
+	genlmsg_cancel(skb, hdr);
+	return 0;
+}
+/**
+ * snet_nl_list - Handle a LIST message
+ * @skb: the NETLINK buffer
+ * @cb:
+ *
+ * Description:
+ * Process a user LIST message and respond. Returns zero on success,
+ * and negative values on error.
+ */
+static int snet_nl_list(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	unsigned int len = 0;
+
+	atomic_set(&snet_nl_seq, cb->nlh->nlmsg_seq);
+	len = snet_event_fill_info(skb, cb);
+	return len;
+}
+
+/**
+ * snet_nl_verdict - Handle a VERDICT message
+ * @skb: the NETLINK buffer
+ * @info the Generic NETLINK info block
+ *
+ * Description:
+ * Provides userspace with a VERDICT message, ie we are sending informations
+ * with this command. Userspace is sending the appropriate verdict for the
+ * event. Returns zero on success,and negative values on error.
+ */
+static int snet_nl_verdict(struct sk_buff *skb,
+			   struct genl_info *info)
+{
+	u32 verdict_id;
+	enum snet_verdict verdict;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	if (!info->attrs[SNET_A_VERDICT_ID] || !info->attrs[SNET_A_VERDICT])
+		return -EINVAL;
+
+	verdict_id = nla_get_u32(info->attrs[SNET_A_VERDICT_ID]);
+	verdict = nla_get_u8(info->attrs[SNET_A_VERDICT]);
+	snet_verdict_set(verdict_id, verdict);
+	return 0;
+}
+
+/**
+ * snet_nl_verdict_delay - Handle a VERDICT_DELAY message
+ * @skb: the NETLINK buffer
+ * @info the Generic NETLINK info block
+ *
+ * Description:
+ * Provides userspace with a VERDICT_DELAY message, ie userspace application
+ * is able to set the value of the timeout for verdicts
+ * Returns zero on success, and negative values on error.
+ */
+static int snet_nl_verdict_delay(struct sk_buff *skb,
+				 struct genl_info *info)
+{
+	int ret = -EINVAL;
+
+	atomic_set(&snet_nl_seq, info->snd_seq);
+
+	if (atomic_read(&snet_num_listeners) <= 0)
+		return 0;
+
+	if (!info->attrs[SNET_A_VERDICT_DELAY])
+		return -EINVAL;
+
+	snet_verdict_delay = nla_get_u32(info->attrs[SNET_A_VERDICT_DELAY]);
+	/* FIXME: do something */
+	ret = snet_nl_response_flag(info, &snet_genl_family,
+				    SNET_C_VERDICT_DELAY, SNET_A_VERDICT_DELAYED,
+				    1);
+	return ret;
+}
+
+static struct genl_ops snet_genl_ops[] = {
+	{
+		.cmd		= SNET_C_VERSION,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_version,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_REGISTER,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_register,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_UNREGISTER,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_unregister,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_INSERT,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_insert,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_REMOVE,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_remove,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_FLUSH,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_flush,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_LIST,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= NULL,
+		.dumpit		= snet_nl_list,
+	},
+	{
+		.cmd		= SNET_C_VERDICT,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_verdict,
+		.dumpit		= NULL,
+	},
+	{
+		.cmd		= SNET_C_VERDICT_DELAY,
+		.flags		= GENL_ADMIN_PERM,
+		.policy		= snet_genl_policy,
+		.doit		= snet_nl_verdict_delay,
+		.dumpit		= NULL,
+	},
+};
+
+static __init int snet_netlink_init(void)
+{
+	return genl_register_family_with_ops(&snet_genl_family,
+					     snet_genl_ops,
+					     ARRAY_SIZE(snet_genl_ops));
+}
+
+void snet_netlink_exit(void)
+{
+	genl_unregister_family(&snet_genl_family);
+}
+
+__initcall(snet_netlink_init);
-- 
1.6.3.3

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Patrick McHardy]

Samir Bellabes wrote:
> +++ b/security/snet/include/snet_netlink.h
> @@ -0,0 +1,201 @@
> +#ifndef _SNET_NETLINK_H
> +#define _SNET_NETLINK_H
> +
> +#include <linux/in6.h>
> +#include "snet_hooks.h"
> +
> +extern unsigned int snet_verdict_delay;

As this file defines the userspace interface, it probably shouldn't
contain declarations of kernel-internal variables (same for
snet_hooks.h). It would also be better placed in include/linux as
the other netlink API definitions.

> +
> +/* commands */
> +enum {
> +	SNET_C_UNSPEC,
> +	SNET_C_VERSION,
> +	SNET_C_REGISTER,
> +	SNET_C_UNREGISTER,
> +	SNET_C_INSERT,
> +	SNET_C_REMOVE,
> +	SNET_C_FLUSH,
> +	SNET_C_LIST,
> +	SNET_C_VERDICT,
> +	SNET_C_VERDICT_DELAY,
> +	__SNET_C_MAX,
> +};
> +
> +#define SNET_C_MAX (__SNET_C_MAX - 1)
> +
> +/* attributes */
> +enum {
> +	SNET_A_UNSPEC,
> +	SNET_A_VERSION,		/* (NLA_U32) the snet protocol version	*/

You're using this to check for a "compliant protocol version" below.
This shouldn't be needed as any protocol changes need to be done
in a compatible fashion.

> +	SNET_A_SYSCALL,		/* (NLA_U8)  a syscall identifier	*/

We're already using 299 syscalls on x86, so perhaps a larger type
would be better suited.

> +	SNET_A_PROTOCOL,	/* (NLA_U8)  a protocol identifier	*/
> +	SNET_A_INSERTED,
> +	SNET_A_REMOVED,
> +	SNET_A_FLUSHED,
> +	SNET_A_REGISTERED,
> +	SNET_A_UNREGISTERED,
> +	SNET_A_VERDICT_ID,
> +	SNET_A_FAMILY,
> +	SNET_A_UID,
> +	SNET_A_PID,
> +	SNET_A_VERDICT,
> +	SNET_A_DATA_EXT,
> +	SNET_A_VERDICT_DELAY,
> +	SNET_A_VERDICT_DELAYED,
> +	__SNET_A_MAX,
> +};
> +
> +#define SNET_A_MAX (__SNET_A_MAX - 1)
> +
> +#define SNET_GENL_NAME		"SNET"
> +#define SNET_GENL_VERSION	SNET_VERSION
> +
> +int snet_nl_send_event(const u32 verdict_id, const enum snet_syscall syscall,
> +		       const u8 protocol, const u8 family, void *data,
> +		       const unsigned int len);
> +
> +int snet_nl_list_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
> +			   u32 flags, u8 protocol, enum snet_syscall syscall);
> +
> +void snet_netlink_exit(void);
> +
> +struct snet_sock_half {
> +	struct {
> +		union {
> +			__be32 ip;
> +			struct in6_addr ip6;
> +		};
> +	} u3;
> +	struct {
> +		__be16 port;
> +	} u;
> +};
> +
> +struct snet_sock_info {
> +	struct snet_sock_half src;
> +	struct snet_sock_half dst;
> +	int type;
> +};

How about using a struct sockaddr or encoding the values within
netlink attributes? That would provide a bit more flexibility in
case you want to support more protocols in the future.

> +
> +#endif /* _SNET_NETLINK_H */
> diff --git a/security/snet/snet_netlink.c b/security/snet/snet_netlink.c
> new file mode 100644
> index 0000000..cc21d6c
> --- /dev/null
> +++ b/security/snet/snet_netlink.c
> @@ -0,0 +1,541 @@
> +#include <linux/sched.h>
> +#include <net/genetlink.h>
> +#include <linux/in6.h>
> +
> +#include "snet.h"
> +#include "snet_netlink.h"
> +#include "snet_verdict.h"
> +#include "snet_event.h"
> +#include <snet_utils.h>
> +
> +atomic_t snet_nl_seq = ATOMIC_INIT(0);
> +static uint32_t snet_nl_pid;
> +static struct genl_family snet_genl_family;
> +atomic_t snet_num_listeners = ATOMIC_INIT(0);

The num_listeners seem to be redundant as you only support a
single listener anyways, whose presence is indicated by
snet_nl_pid != 0.

> +
> +/*
> + * snet genetlink
> + */
> +int snet_nl_send_event(const u32 verdict_id, const enum snet_syscall syscall,
> +		       const u8 protocol, const u8 family, void *data,
> +		       const unsigned int len)
> +{
> +	struct sk_buff *skb_rsp;
> +	void *msg_head;
> +	int ret = -ENOMEM;
> +
> +	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (skb_rsp == NULL)
> +		return 0;

You could decrease the chance of rcvqueue overflow by using a smaller
allocation size.

> +
> +	msg_head = genlmsg_put(skb_rsp, snet_nl_pid,
> +			       atomic_inc_return(&snet_nl_seq),
> +			       &snet_genl_family, 0, SNET_C_VERDICT);
> +	if (msg_head == NULL)
> +		goto send_event_failure;
> +
> +	snet_dbg("verdict_id=0x%x syscall=%s protocol=%u "
> +		 "family=%u uid=%u pid=%u\n",
> +		 verdict_id, snet_syscall_name(syscall),
> +		 protocol, family, current_uid(), current->pid);
> +
> +	if (verdict_id) {
> +		ret = nla_put_u32(skb_rsp, SNET_A_VERDICT_ID, verdict_id);
> +		if (ret != 0)
> +			goto send_event_failure;
> +	}
> +	ret = nla_put_u8(skb_rsp, SNET_A_SYSCALL, syscall);
> +	if (ret != 0)
> +		goto send_event_failure;
> +	ret = nla_put_u8(skb_rsp, SNET_A_PROTOCOL, protocol);
> +	if (ret != 0)
> +		goto send_event_failure;
> +	ret = nla_put_u8(skb_rsp, SNET_A_FAMILY, family);
> +	if (ret != 0)
> +		goto send_event_failure;
> +	ret = nla_put_u32(skb_rsp, SNET_A_UID, current_uid());
> +	if (ret != 0)
> +		goto send_event_failure;
> +	ret = nla_put_u32(skb_rsp, SNET_A_PID, current->pid);
> +	if (ret != 0)
> +		goto send_event_failure;
> +	ret = nla_put(skb_rsp, SNET_A_DATA_EXT, len, data);
> +	if (ret != 0)
> +		goto send_event_failure;

I guess its a matter of taste, but the NLA_PUT macros already take
care of exception handling and keep the code smaller.

> +
> +	ret = genlmsg_end(skb_rsp, msg_head);
> +	if (ret < 0)
> +		goto send_event_failure;
> +
> +	genlmsg_unicast(&init_net, skb_rsp, snet_nl_pid);
> +	return 0;
> +
> +send_event_failure:
> +	kfree_skb(skb_rsp);
> +	return 0;

Shouldn't this error be returned to the caller to avoid waiting
for the timeout to occur (same question for the return value of
genlmsg_unicast, which can also fail).

> +}
> +
> +/*
> + * snet genetlink helper functions
> + */
> +static int snet_nl_response_flag(struct genl_info *info,
> +				 struct genl_family *family,
> +				 u8 cmd, int attrtype, u8 set_resp_flag)
> +{
> +	int ret = -EINVAL;
> +	struct sk_buff *skb_rsp = NULL;
> +	void *msg_head;
> +
> +	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (skb_rsp == NULL)
> +		return -ENOMEM;
> +	msg_head = genlmsg_put_reply(skb_rsp, info, family, 0, cmd);
> +	if (msg_head == NULL)
> +		goto response_failure;
> +
> +	/* we put flag only if it is asked */
> +	if (set_resp_flag) {
> +		ret = nla_put_flag(skb_rsp, attrtype);
> +		if (ret != 0)
> +			goto response_failure;
> +	}
> +
> +	genlmsg_end(skb_rsp, msg_head);
> +	ret = genlmsg_reply(skb_rsp, info);
> +	if (ret != 0)
> +		goto response_failure;
> +	return 0;
> +
> +response_failure:
> +	kfree_skb(skb_rsp);
> +	return ret;
> +}
> +
> +/*
> + * snet genetlink functions
> + */
> +
> +static struct genl_family snet_genl_family = {
> +	.id		= GENL_ID_GENERATE,
> +	.hdrsize	= 0,
> +	.name		= SNET_GENL_NAME,
> +	.version	= SNET_GENL_VERSION,
> +	.maxattr	= SNET_A_MAX,
> +};
> +
> +static const struct nla_policy snet_genl_policy[SNET_A_MAX + 1]
> +__read_mostly = {

You don't need __read_mostly for const. If I recall correctly, it even
causes an error with certain compiler or linker versions.

> +	[SNET_A_VERSION]		= { .type = NLA_U32 },
> +	[SNET_A_SYSCALL]		= { .type = NLA_U8 },
> +	[SNET_A_PROTOCOL]		= { .type = NLA_U8 },
> +	[SNET_A_INSERTED]		= { .type = NLA_FLAG },
> +	[SNET_A_REMOVED]		= { .type = NLA_FLAG },
> +	[SNET_A_FLUSHED]		= { .type = NLA_FLAG },
> +	[SNET_A_REGISTERED]		= { .type = NLA_FLAG },
> +	[SNET_A_UNREGISTERED]		= { .type = NLA_FLAG },
> +	[SNET_A_VERDICT_ID]		= { .type = NLA_U32 },
> +	[SNET_A_FAMILY]			= { .type = NLA_U8 },
> +	[SNET_A_UID]			= { .type = NLA_U32 },
> +	[SNET_A_PID]			= { .type = NLA_U32 },
> +	[SNET_A_VERDICT]		= { .type = NLA_U8 },
> +	[SNET_A_DATA_EXT]		= { .type = NLA_BINARY,
> +					    .len = sizeof(struct snet_sock_info) },
> +	[SNET_A_VERDICT_DELAY]		= { .type = NLA_U32 },
> +	[SNET_A_VERDICT_DELAYED]	= { .type = NLA_FLAG },
> +};
> +
> +/**
> + * snet_nl_version - Handle a VERSION message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Process a user generated VERSION message and respond accordingly.
> + * Returns zero on success, negative values on failure.
> + */
> +static int snet_nl_version(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret = -ENOMEM;
> +	struct sk_buff *skb_rsp = NULL;
> +	void *msg_head;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;

In all these checks for listeners, I think it would make sense to
provide an error to userspace if it hasn't registered properly,
perhaps ENOTCONN or something like that.

> +
> +	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (skb_rsp == NULL)
> +		return -ENOMEM;
> +	msg_head = genlmsg_put_reply(skb_rsp, info, &snet_genl_family,
> +				     0, SNET_C_VERSION);
> +	if (msg_head == NULL)
> +		goto version_failure;
> +
> +	ret = nla_put_u32(skb_rsp, SNET_A_VERSION, SNET_VERSION);
> +	if (ret != 0)
> +		goto version_failure;
> +
> +	genlmsg_end(skb_rsp, msg_head);
> +
> +	ret = genlmsg_reply(skb_rsp, info);
> +	if (ret != 0)
> +		goto version_failure;
> +	return 0;
> +
> +version_failure:
> +	kfree_skb(skb_rsp);
> +	return ret;
> +}
> +
> +/**
> + * snet_nl_register - Handle a REGISTER message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Notify the kernel that an application is listening for events.
> + * Returns zero on success, negative values on failure.
> + */
> +static int snet_nl_register(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret = -EINVAL;
> +	u32 version = 0;
> +	u8 set_resp_flag = 0;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (!info->attrs[SNET_A_VERSION])
> +		return -EINVAL;
> +	version = nla_get_u32(info->attrs[SNET_A_VERSION]);
> +
> +	if (version == SNET_VERSION) {	/* version is compliant */
> +		atomic_inc(&snet_num_listeners);
> +		set_resp_flag = 1;
> +	}
> +
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_REGISTER, SNET_A_REGISTERED,
> +				    set_resp_flag);

Is this really needed? A return value of 0 should already tell userspace
that the command was successful. If it really wants a seperate success
message, it can use NLM_F_ACK. This will also automatically take care
of using the proper sequence number, so the snet_nl_seq handling isn't
required anymore. Same for all similar cases below.

> +
> +	snet_nl_pid = info->snd_pid;
> +	snet_dbg("pid=%u num_listeners=%d\n",
> +		 snet_nl_pid, atomic_read(&snet_num_listeners));
> +	return ret;
> +}
> +
> +/**
> + * snet_nl_unregister - Handle a UNREGISTER message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Notify the kernel that the application is no more listening for events.
> + * Returns zero on success, negative values on failure.
> + */
> +static int snet_nl_unregister(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret = -EINVAL;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners))
> +		atomic_dec(&snet_num_listeners);
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_UNREGISTER, SNET_A_UNREGISTERED, 1);
> +	snet_dbg("pid=%u num_listeners=%d\n",
> +		 snet_nl_pid, atomic_read(&snet_num_listeners));
> +	return ret;
> +}
> +
> +/**
> + * snet_nl_insert - Handle a INSERT message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Insert a new event to the events' hashtable. Returns zero on success,
> + * negative values on failure.
> + */
> +static int snet_nl_insert(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret_event = -EINVAL, ret = -EINVAL;
> +	enum snet_syscall syscall;
> +	u8 protocol;
> +	u8 set_resp_flag = 0;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;
> +
> +	if (!info->attrs[SNET_A_SYSCALL] || !info->attrs[SNET_A_PROTOCOL])
> +		return -EINVAL;
> +
> +	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
> +	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
> +	ret_event = snet_event_insert(syscall, protocol);
> +	snet_dbg("syscall=%s protocol=%u insert=%s\n",
> +		 snet_syscall_name(syscall), protocol,
> +		 (ret_event == 0) ? "success" : "failed");
> +
> +	if (ret_event == 0)
> +		set_resp_flag = 1;
> +
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_INSERT, SNET_A_INSERTED,
> +				    set_resp_flag);
> +	return ret;
> +}
> +
> +/**
> + * snet_nl_remove - Handle a REMOVE message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Remove a event from the events' hastable. Returns zero on success,
> + * negative values on failure.
> + */
> +static int snet_nl_remove(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret_event = -EINVAL, ret = -EINVAL;
> +	enum snet_syscall syscall;
> +	u8 protocol;
> +	u8 set_resp_flag = 0;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;
> +
> +	if (!info->attrs[SNET_A_SYSCALL] || !info->attrs[SNET_A_PROTOCOL])
> +		return -EINVAL;
> +
> +	syscall = nla_get_u8(info->attrs[SNET_A_SYSCALL]);
> +	protocol = nla_get_u8(info->attrs[SNET_A_PROTOCOL]);
> +	ret_event = snet_event_remove(syscall, protocol);
> +	snet_dbg("syscall=%s protocol=%u remove=%s\n",
> +		 snet_syscall_name(syscall), protocol,
> +		 (ret_event == 0) ? "success" : "failed");
> +
> +	if (ret_event == 0)
> +		set_resp_flag = 1;
> +
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_REMOVE, SNET_A_REMOVED,
> +				    set_resp_flag);
> +	return ret;
> +}
> +
> +/**
> + * snet_nl_flush - Handle a FLUSH message
> + * @skb: the NETLINK buffer
> + * @info: the Generic NETLINK info block
> + *
> + * Description:
> + * Remove all events from the hashtable. Returns zero on success,
> + * negative values on failure.
> + */
> +static int snet_nl_flush(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret = -EINVAL;
> +	u8 set_resp_flag = 0;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;
> +
> +	snet_event_flush();
> +
> +	set_resp_flag = 1;
> +
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_FLUSH, SNET_A_FLUSHED,
> +				    set_resp_flag);
> +	return ret;
> +}
> +
> +int snet_nl_list_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
> +			   u32 flags, u8 protocol, enum snet_syscall syscall)
> +{
> +	void *hdr;
> +	int ret = -1;
> +
> +	hdr = genlmsg_put(skb, pid, seq, &snet_genl_family, flags, SNET_C_LIST);
> +	if (hdr == NULL)
> +		return -1;
> +
> +	ret = nla_put_u8(skb, SNET_A_SYSCALL, syscall);
> +	if (ret != 0)
> +		goto list_failure;
> +
> +	ret = nla_put_u8(skb, SNET_A_PROTOCOL, protocol);
> +	if (ret != 0)
> +		goto list_failure;
> +
> +	return genlmsg_end(skb, hdr);
> +
> +list_failure:
> +	genlmsg_cancel(skb, hdr);
> +	return 0;
> +}
> +/**
> + * snet_nl_list - Handle a LIST message
> + * @skb: the NETLINK buffer
> + * @cb:
> + *
> + * Description:
> + * Process a user LIST message and respond. Returns zero on success,
> + * and negative values on error.
> + */
> +static int snet_nl_list(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	unsigned int len = 0;
> +
> +	atomic_set(&snet_nl_seq, cb->nlh->nlmsg_seq);
> +	len = snet_event_fill_info(skb, cb);
> +	return len;
> +}
> +
> +/**
> + * snet_nl_verdict - Handle a VERDICT message
> + * @skb: the NETLINK buffer
> + * @info the Generic NETLINK info block
> + *
> + * Description:
> + * Provides userspace with a VERDICT message, ie we are sending informations
> + * with this command. Userspace is sending the appropriate verdict for the
> + * event. Returns zero on success,and negative values on error.
> + */
> +static int snet_nl_verdict(struct sk_buff *skb,
> +			   struct genl_info *info)
> +{
> +	u32 verdict_id;
> +	enum snet_verdict verdict;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;
> +
> +	if (!info->attrs[SNET_A_VERDICT_ID] || !info->attrs[SNET_A_VERDICT])
> +		return -EINVAL;
> +
> +	verdict_id = nla_get_u32(info->attrs[SNET_A_VERDICT_ID]);
> +	verdict = nla_get_u8(info->attrs[SNET_A_VERDICT]);
> +	snet_verdict_set(verdict_id, verdict);
> +	return 0;
> +}
> +
> +/**
> + * snet_nl_verdict_delay - Handle a VERDICT_DELAY message
> + * @skb: the NETLINK buffer
> + * @info the Generic NETLINK info block
> + *
> + * Description:
> + * Provides userspace with a VERDICT_DELAY message, ie userspace application
> + * is able to set the value of the timeout for verdicts
> + * Returns zero on success, and negative values on error.
> + */
> +static int snet_nl_verdict_delay(struct sk_buff *skb,
> +				 struct genl_info *info)
> +{
> +	int ret = -EINVAL;
> +
> +	atomic_set(&snet_nl_seq, info->snd_seq);
> +
> +	if (atomic_read(&snet_num_listeners) <= 0)
> +		return 0;
> +
> +	if (!info->attrs[SNET_A_VERDICT_DELAY])
> +		return -EINVAL;
> +
> +	snet_verdict_delay = nla_get_u32(info->attrs[SNET_A_VERDICT_DELAY]);
> +	/* FIXME: do something */
> +	ret = snet_nl_response_flag(info, &snet_genl_family,
> +				    SNET_C_VERDICT_DELAY, SNET_A_VERDICT_DELAYED,
> +				    1);
> +	return ret;
> +}
> +
> +static struct genl_ops snet_genl_ops[] = {
> +	{
> +		.cmd		= SNET_C_VERSION,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_version,
> +		.dumpit		= NULL,

The NULL initializations aren't neccessary.

> +	},
> +	{
> +		.cmd		= SNET_C_REGISTER,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_register,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_UNREGISTER,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_unregister,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_INSERT,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_insert,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_REMOVE,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_remove,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_FLUSH,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_flush,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_LIST,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= NULL,
> +		.dumpit		= snet_nl_list,
> +	},
> +	{
> +		.cmd		= SNET_C_VERDICT,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_verdict,
> +		.dumpit		= NULL,
> +	},
> +	{
> +		.cmd		= SNET_C_VERDICT_DELAY,
> +		.flags		= GENL_ADMIN_PERM,
> +		.policy		= snet_genl_policy,
> +		.doit		= snet_nl_verdict_delay,
> +		.dumpit		= NULL,
> +	},
> +};
> +
> +static __init int snet_netlink_init(void)
> +{
> +	return genl_register_family_with_ops(&snet_genl_family,
> +					     snet_genl_ops,
> +					     ARRAY_SIZE(snet_genl_ops));
> +}
> +
> +void snet_netlink_exit(void)
> +{
> +	genl_unregister_family(&snet_genl_family);
> +}
> +
> +__initcall(snet_netlink_init);

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +++ b/security/snet/include/snet_netlink.h
>> +	SNET_A_SYSCALL,		/* (NLA_U8)  a syscall identifier	*/
>
> We're already using 299 syscalls on x86, so perhaps a larger type
> would be better suited.

I moved all references to u16

thanks

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +++ b/security/snet/include/snet_netlink.h
>> +	SNET_A_VERSION,		/* (NLA_U32) the snet protocol version	*/
>
> You're using this to check for a "compliant protocol version" below.
> This shouldn't be needed as any protocol changes need to be done
> in a compatible fashion.

what if userspace lib is using a old protocol version ? kernel and
userspace will use incompatible protocol, which may result in errors.

The idea of this 'version' mecanism is to prevent such
incompatibilities, even if the userspace is (un)volontary not using the
good library (which may be the one tagged in the same time as the kernel
running)

sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Patrick McHardy]

Samir Bellabes wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>> Samir Bellabes wrote:
>>> +++ b/security/snet/include/snet_netlink.h
>>> +	SNET_A_VERSION,		/* (NLA_U32) the snet protocol version	*/
>> You're using this to check for a "compliant protocol version" below.
>> This shouldn't be needed as any protocol changes need to be done
>> in a compatible fashion.
> 
> what if userspace lib is using a old protocol version ? kernel and
> userspace will use incompatible protocol, which may result in errors.

Any protocol changes need to be done in a compatible fashion once
this is in the upstream kernel, so that case should never happen.

> The idea of this 'version' mecanism is to prevent such
> incompatibilities, even if the userspace is (un)volontary not using the
> good library (which may be the one tagged in the same time as the kernel
> running)

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +++ b/security/snet/include/snet_netlink.h
>> +
>> +struct snet_sock_half {
>> +	struct {
>> +		union {
>> +			__be32 ip;
>> +			struct in6_addr ip6;
>> +		};
>> +	} u3;
>> +	struct {
>> +		__be16 port;
>> +	} u;
>> +};
>> +
>> +struct snet_sock_info {
>> +	struct snet_sock_half src;
>> +	struct snet_sock_half dst;
>> +	int type;
>> +};
>
> How about using a struct sockaddr or encoding the values within
> netlink attributes? That would provide a bit more flexibility in
> case you want to support more protocols in the future.

indeed, I already move to the encoding of values independantly within
netlink attributes. This had to be done before, and it was in the
TODO, so now it's done. 

At first, I tried to use a attribute NLA_BINARY with all the datas
inside snet_sock_info, so it won't break the netlink protocol between
userspace and kernel, at each modification (adding/removing element
inside the structure) 

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +atomic_t snet_nl_seq = ATOMIC_INIT(0);
>> +static uint32_t snet_nl_pid;
>> +static struct genl_family snet_genl_family;
>> +atomic_t snet_num_listeners = ATOMIC_INIT(0);
>
> The num_listeners seem to be redundant as you only support a
> single listener anyways, whose presence is indicated by
> snet_nl_pid != 0.

Actually, I may want to support multiple listeners, or simultaneous
communication with userspace, as we may want to listen for event and
reply for verdict in the same time as adding or deleting events.
I will fix this mecanism.

thanks Patrick,
sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +
>> +	msg_head = genlmsg_put(skb_rsp, snet_nl_pid,
>> +			       atomic_inc_return(&snet_nl_seq),
>> +			       &snet_genl_family, 0, SNET_C_VERDICT);
>> +	if (msg_head == NULL)
>> +		goto send_event_failure;
>> +
>> +	snet_dbg("verdict_id=0x%x syscall=%s protocol=%u "
>> +		 "family=%u uid=%u pid=%u\n",
>> +		 verdict_id, snet_syscall_name(syscall),
>> +		 protocol, family, current_uid(), current->pid);
>> +
>> +	if (verdict_id) {
>> +		ret = nla_put_u32(skb_rsp, SNET_A_VERDICT_ID, verdict_id);
>> +		if (ret != 0)
>> +			goto send_event_failure;
>> +	}
>> +	ret = nla_put_u8(skb_rsp, SNET_A_SYSCALL, syscall);
>> +	if (ret != 0)
>> +		goto send_event_failure;
>> +	ret = nla_put_u8(skb_rsp, SNET_A_PROTOCOL, protocol);
>> +	if (ret != 0)
>> +		goto send_event_failure;
>> +	ret = nla_put_u8(skb_rsp, SNET_A_FAMILY, family);
>> +	if (ret != 0)
>> +		goto send_event_failure;
>> +	ret = nla_put_u32(skb_rsp, SNET_A_UID, current_uid());
>> +	if (ret != 0)
>> +		goto send_event_failure;
>> +	ret = nla_put_u32(skb_rsp, SNET_A_PID, current->pid);
>> +	if (ret != 0)
>> +		goto send_event_failure;
>> +	ret = nla_put(skb_rsp, SNET_A_DATA_EXT, len, data);
>> +	if (ret != 0)
>> +		goto send_event_failure;
>
> I guess its a matter of taste, but the NLA_PUT macros already take
> care of exception handling and keep the code smaller.

indeed, I moved the code to use NLA_PUT and the hidden failure label

>> +
>> +	ret = genlmsg_end(skb_rsp, msg_head);
>> +	if (ret < 0)
>> +		goto send_event_failure;
>> +
>> +	genlmsg_unicast(&init_net, skb_rsp, snet_nl_pid);
>> +	return 0;
>> +
>> +send_event_failure:
>> +	kfree_skb(skb_rsp);
>> +	return 0;
>
> Shouldn't this error be returned to the caller to avoid waiting
> for the timeout to occur (same question for the return value of
> genlmsg_unicast, which can also fail).

indeed, if error occurred, no need to wait, we can apply the polocy
verdict.
genlmsg_unicast() is taking care of freeing the skb if error occured, so
we can return directly the value of genlmsg_unicast()

thanks Patrick

sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +static const struct nla_policy snet_genl_policy[SNET_A_MAX + 1]
>> +__read_mostly = {
>
> You don't need __read_mostly for const. If I recall correctly, it even
> causes an error with certain compiler or linker versions.

fixed, thanks Patrick

I didn't had errors with (fedora core 11):
gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)
GNU ld version 2.19.51.0.2-17.fc11 20090204

nor (ubuntu 9.10):
gcc (Ubuntu 4.4.1-4ubuntu8) 4.4.1 
GNU ld (GNU Binutils for Ubuntu) 2.20 (2.20-0ubuntu2)

sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +/**
>> + * snet_nl_register - Handle a REGISTER message
>> + * @skb: the NETLINK buffer
>> + * @info: the Generic NETLINK info block
>> + *
>> + * Description:
>> + * Notify the kernel that an application is listening for events.
>> + * Returns zero on success, negative values on failure.
>> + */
>> +static int snet_nl_register(struct sk_buff *skb, struct genl_info *info)
>> +{
>> +	int ret = -EINVAL;
>> +	u32 version = 0;
>> +	u8 set_resp_flag = 0;
>> +
>> +	atomic_set(&snet_nl_seq, info->snd_seq);
>> +
>> +	if (!info->attrs[SNET_A_VERSION])
>> +		return -EINVAL;
>> +	version = nla_get_u32(info->attrs[SNET_A_VERSION]);
>> +
>> +	if (version == SNET_VERSION) {	/* version is compliant */
>> +		atomic_inc(&snet_num_listeners);
>> +		set_resp_flag = 1;
>> +	}
>> +
>> +	ret = snet_nl_response_flag(info, &snet_genl_family,
>> +				    SNET_C_REGISTER, SNET_A_REGISTERED,
>> +				    set_resp_flag);
>
> Is this really needed? A return value of 0 should already tell userspace
> that the command was successful. If it really wants a seperate success
> message, it can use NLM_F_ACK. This will also automatically take care
> of using the proper sequence number, so the snet_nl_seq handling isn't
> required anymore. Same for all similar cases below.

Indeed, we can use the return value to inform the userspace about the
status of operations. I moved all the code, and deleted the no longer
used function snet_nl_response_flag()

Thanks Patrick,
sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +static struct genl_ops snet_genl_ops[] = {
>> +	{
>> +		.cmd		= SNET_C_VERSION,
>> +		.flags		= GENL_ADMIN_PERM,
>> +		.policy		= snet_genl_policy,
>> +		.doit		= snet_nl_version,
>> +		.dumpit		= NULL,
>
> The NULL initializations aren't neccessary.
>
[...]
Instead of dropping lines with the NULL initializations, I reduced the
code with a declaration macro

Patricks, thanks
sam
 
commit f8539d466dbe081b35f6813cfedd91947615bee9
Author: Samir Bellabes <sam@synack.fr>
Date:   Fri Jan 15 09:53:09 2010 +0100

    snet: reducing code by using macros to define snet_genl_ops
    
    This patch introduces the macro SNET_GENL_OPS. This reduces the code of the
    declaration struct genl_ops snet_genl_ops[]
    
    Signed-off-by: Samir Bellabes <sam@synack.fr>

diff --git a/security/snet/snet_netlink.c b/security/snet/snet_netlink.c
index e108e7a..ba6a26d 100644
--- a/security/snet/snet_netlink.c
+++ b/security/snet/snet_netlink.c
@@ -400,70 +400,33 @@ out:
 	return ret;
 }
 
+#define SNET_GENL_OPS(_cmd, _flags, _policy, _op, _opname)		\
+	{							\
+		.cmd	= _cmd,					\
+		.flags	= _flags,				\
+		.policy	= _policy,				\
+		._op	= snet_nl_##_opname,			\
+	}
+
 static struct genl_ops snet_genl_ops[] = {
-	{
-		.cmd		= SNET_C_VERSION,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_version,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_REGISTER,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_register,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_UNREGISTER,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_unregister,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_INSERT,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_insert,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_REMOVE,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_remove,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_FLUSH,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_flush,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_LIST,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= NULL,
-		.dumpit		= snet_nl_list,
-	},
-	{
-		.cmd		= SNET_C_VERDICT,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_verdict,
-		.dumpit		= NULL,
-	},
-	{
-		.cmd		= SNET_C_VERDICT_DELAY,
-		.flags		= GENL_ADMIN_PERM,
-		.policy		= snet_genl_policy,
-		.doit		= snet_nl_verdict_delay,
-		.dumpit		= NULL,
-	},
+	SNET_GENL_OPS(SNET_C_VERSION, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, version),
+	SNET_GENL_OPS(SNET_C_REGISTER, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, register),
+	SNET_GENL_OPS(SNET_C_UNREGISTER, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, unregister),
+	SNET_GENL_OPS(SNET_C_INSERT, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, insert),
+	SNET_GENL_OPS(SNET_C_REMOVE, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, remove),
+	SNET_GENL_OPS(SNET_C_FLUSH, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, flush),
+	SNET_GENL_OPS(SNET_C_REGISTER, GENL_ADMIN_PERM, snet_genl_policy,
+		      dumpit, list),
+	SNET_GENL_OPS(SNET_C_REGISTER, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, verdict),
+	SNET_GENL_OPS(SNET_C_VERDICT_DELAY, GENL_ADMIN_PERM, snet_genl_policy,
+		      doit, verdict_delay),
 };
 
 static __init int snet_netlink_init(void)

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

>> +atomic_t snet_num_listeners = ATOMIC_INIT(0);
>
> The num_listeners seem to be redundant as you only support a
> single listener anyways, whose presence is indicated by
> snet_nl_pid != 0.

I simplified the code, by removing the variable snet_num_listeners and
use the value of snet_nl_pid
 * if snet_nl_pid == 0, then there is no userspace listening application 
 * if snet_nl_pid > 0, then its value is the PID of the listening
   application

In the same time, I deleted the check on a listener for this operations
on verdict : snet_nl_version, snet_nl_insert, snet_nl_remove,
snet_nl_flush, snet_nl_verdict_delay. 
In this way, it's possible to execute this operations and get events in
the same time (which means have a listeners)

Patrick, thanks again for reviewing

sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +	skb_rsp = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>> +	if (skb_rsp == NULL)
>> +		return 0;
>
> You could decrease the chance of rcvqueue overflow by using a smaller
> allocation size.

I took care of this by calculating the exact needed size.

thanks,
sam

Re: [RFC 7/9] snet: introduce snet_netlink.c and snet_netlink.h

[Samir Bellabes]

Patrick McHardy <kaber@trash.net> writes:

> Samir Bellabes wrote:
>> +++ b/security/snet/include/snet_netlink.h
>> @@ -0,0 +1,201 @@
>> +#ifndef _SNET_NETLINK_H
>> +#define _SNET_NETLINK_H
>> +
>> +#include <linux/in6.h>
>> +#include "snet_hooks.h"
>> +
>> +extern unsigned int snet_verdict_delay;
>
> As this file defines the userspace interface, it probably shouldn't
> contain declarations of kernel-internal variables (same for
> snet_hooks.h). It would also be better placed in include/linux as
> the other netlink API definitions.

sorry the the delay on this.
indeed, this is fixed now, thanks Patrick

sam

[RFC 8/9] snet: introduce snet_verdict.c and snet_verdict.h

[Samir Bellabes]

This patch adds the snet's subsystem responsive of managing verdicts

snet is using the word 'verdict' for the returning value of LSM hooks.
Different states exist (grant/deny/pending/none).

This patch introduces a hashtable 'verdict_hash' and operations (set/get/search..)
in order to manage verdicts. Syscalls are waiting, inside a classical waitqueue,
for theirs verdicts or for a timeout. Timeout value and the default verdict
policy are configurable at boot.
With the help of the communication's subsystem, verdicts are coming from userspace.

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet_verdict.h |   33 ++++++
 security/snet/snet_verdict.c         |  210 ++++++++++++++++++++++++++++++++++
 2 files changed, 243 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet_verdict.h
 create mode 100644 security/snet/snet_verdict.c

diff --git a/security/snet/include/snet_verdict.h b/security/snet/include/snet_verdict.h
new file mode 100644
index 0000000..fd9a5e5
--- /dev/null
+++ b/security/snet/include/snet_verdict.h
@@ -0,0 +1,33 @@
+#ifndef _SNET_VERDICT_H
+#define _SNET_VERDICT_H
+
+extern unsigned int verdict_hash_size;
+extern unsigned int snet_verdict_delay;
+
+enum snet_verdict {
+	SNET_VERDICT_GRANT = 0,	/* grant the syscall */
+	SNET_VERDICT_DENY,	/* deny the syscall */
+	SNET_VERDICT_PENDING,	/* waiting for a decision */
+	SNET_VERDICT_NONE,	/* no decision can be set */
+	SNET_VERDICT_INVALID,
+};
+
+#define SNET_NR_VERDICT_TYPES SNET_VERDICT_INVALID
+
+/* helper functions */
+const enum snet_verdict snet_verdict_wait(const u32 verdict_id);
+
+/* manipulate the verdicts hash table */
+const enum snet_verdict snet_verdict_get(const u32 verdict_id);
+int snet_verdict_set(const u32 verdict_id, const enum snet_verdict verdict);
+int snet_verdict_insert(void);
+int snet_verdict_remove(const u32 verdict_id);
+int snet_verdict_insert(void);
+void snet_verdict_flush(void);
+
+/* init function */
+int snet_verdict_init(void);
+/* exit function */
+int snet_verdict_exit(void);
+
+#endif /* _SNET_VERDICT_H */
diff --git a/security/snet/snet_verdict.c b/security/snet/snet_verdict.c
new file mode 100644
index 0000000..55dccea
--- /dev/null
+++ b/security/snet/snet_verdict.c
@@ -0,0 +1,210 @@
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/random.h>
+#include <linux/wait.h>
+#include <linux/jhash.h>
+#include <asm/atomic.h>
+
+#include "snet.h"
+#include "snet_verdict.h"
+#include "snet_utils.h"
+
+static struct list_head *verdict_hash;
+static rwlock_t verdict_hash_lock = __RW_LOCK_UNLOCKED();
+
+struct snet_verdict_entry {
+	struct list_head list;
+	u32 verdict_id;
+	enum snet_verdict verdict;
+};
+
+static atomic_t value = ATOMIC_INIT(1);
+
+/* when waiting for a verdict, process is added to this queue */
+static DECLARE_WAIT_QUEUE_HEAD(snet_wq);
+
+/* lookup for a verdict - before using this function, lock verdict_hash_lock */
+static struct snet_verdict_entry *__snet_verdict_lookup(const u32 verdict_id)
+{
+	unsigned int h = 0;
+	struct list_head *l = NULL;
+	struct snet_verdict_entry *s = NULL;
+	u32 vid = 0;
+
+	if (!verdict_hash)
+		return NULL;
+
+	vid = verdict_id;
+	/* computing its hash value */
+	h = jhash(&vid, sizeof(u32), 0) % verdict_hash_size;
+	l = &verdict_hash[h];
+
+	list_for_each_entry(s, l, list) {
+		if (s->verdict_id == vid) {
+			return s;
+		}
+	}
+	return NULL;
+}
+
+const enum snet_verdict snet_verdict_wait(const u32 verdict_id)
+{
+	enum snet_verdict verdict = SNET_VERDICT_NONE;
+	long ret = 0;
+
+	ret = wait_event_timeout(snet_wq,
+				 (verdict = snet_verdict_get(verdict_id))
+				 != SNET_VERDICT_PENDING,
+				 snet_verdict_delay * HZ);
+	if (ret)
+		return snet_verdict_get(verdict_id);
+	else
+		return SNET_VERDICT_NONE;
+}
+
+const enum snet_verdict snet_verdict_get(const u32 verdict_id)
+{
+	enum snet_verdict v = SNET_VERDICT_NONE;
+	struct snet_verdict_entry *data = NULL;
+
+	read_lock_bh(&verdict_hash_lock);
+	data = __snet_verdict_lookup(verdict_id);
+	if (data != NULL)
+		v = data->verdict;
+
+	read_unlock_bh(&verdict_hash_lock);
+	return v;
+}
+
+int snet_verdict_set(const u32 verdict_id, const enum snet_verdict verdict)
+{
+	struct snet_verdict_entry *data = NULL;
+	int ret = -EINVAL;
+
+	if (verdict >= SNET_NR_VERDICT_TYPES)
+		goto out;
+
+	write_lock_bh(&verdict_hash_lock);
+	data = __snet_verdict_lookup(verdict_id);
+	if (data != NULL) {
+		/* if verdict is already set because of
+		   timeout, we won't modify it */
+		if (data->verdict == SNET_VERDICT_PENDING) {
+			data->verdict = verdict;
+			ret = 0;
+		}
+	}
+	write_unlock_bh(&verdict_hash_lock);
+	wake_up(&snet_wq);
+out:
+	return ret;
+}
+
+int snet_verdict_remove(const u32 verdict_id)
+{
+	struct snet_verdict_entry *data = NULL;
+
+	write_lock_bh(&verdict_hash_lock);
+	data = __snet_verdict_lookup(verdict_id);
+	if (data == NULL) {
+		write_unlock_bh(&verdict_hash_lock);
+		return -EINVAL;
+	}
+
+	list_del(&data->list);
+	write_unlock_bh(&verdict_hash_lock);
+	kfree(data);
+	return 0;
+}
+
+int snet_verdict_insert(void)
+{
+	struct snet_verdict_entry *data = NULL;
+	unsigned int h = 0;
+	u32 verdict_id = 0;
+
+	data = kzalloc(sizeof(struct snet_verdict_entry), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	do {
+		verdict_id = atomic_inc_return(&value);
+	} while (verdict_id == 0);
+
+	data->verdict_id = verdict_id;
+	data->verdict = SNET_VERDICT_PENDING;
+	INIT_LIST_HEAD(&(data->list));
+	h = jhash(&(data->verdict_id), sizeof(u32), 0) % verdict_hash_size;
+
+	write_lock_bh(&verdict_hash_lock);
+	if (verdict_hash) {
+		list_add_tail(&data->list, &verdict_hash[h]);
+		write_unlock_bh(&verdict_hash_lock);
+	} else {
+		write_unlock_bh(&verdict_hash_lock);
+		kfree(data);
+		verdict_id = 0;
+	}
+
+	return verdict_id;
+}
+
+void __snet_verdict_flush(void)
+{
+	struct snet_verdict_entry *data = NULL;
+	unsigned int i = 0;
+
+	for (i = 0; i < verdict_hash_size; i++) {
+		while (!list_empty(&verdict_hash[i])) {
+			data = list_entry(verdict_hash[i].next,
+					  struct snet_verdict_entry, list);
+			list_del(&data->list);
+			kfree(data);
+		}
+	}
+	return;
+}
+
+void snet_verdict_flush(void)
+{
+	write_lock_bh(&verdict_hash_lock);
+	if (verdict_hash)
+		__snet_verdict_flush();
+	write_unlock_bh(&verdict_hash_lock);
+	return;
+}
+
+/* init function */
+int snet_verdict_init(void)
+{
+	int err = 0, i = 0;
+
+	verdict_hash = kzalloc(sizeof(struct list_head) * verdict_hash_size,
+			  GFP_KERNEL);
+	if (!verdict_hash) {
+		printk(KERN_WARNING
+		       "snet: can't alloc memory for verdict\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < verdict_hash_size; i++)
+		INIT_LIST_HEAD(&(verdict_hash[i]));
+
+out:
+	return err;
+}
+
+/* exit function */
+int snet_verdict_exit(void)
+{
+	write_lock_bh(&verdict_hash_lock);
+	if (verdict_hash) {
+		__snet_verdict_flush();
+		kfree(verdict_hash);
+		verdict_hash = NULL;
+	}
+	write_unlock_bh(&verdict_hash_lock);
+
+	return 0;
+}
-- 
1.6.3.3

[RFC 9/9] snet: introduce snet_utils.c and snet_utils.h

[Samir Bellabes]

This patch provides helper functions for other subsystems

Signed-off-by: Samir Bellabes <sam@synack.fr>
---
 security/snet/include/snet_utils.h |    9 ++++++++
 security/snet/snet_utils.c         |   40 ++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100644 security/snet/include/snet_utils.h
 create mode 100644 security/snet/snet_utils.c

diff --git a/security/snet/include/snet_utils.h b/security/snet/include/snet_utils.h
new file mode 100644
index 0000000..131222a
--- /dev/null
+++ b/security/snet/include/snet_utils.h
@@ -0,0 +1,9 @@
+#ifndef _SNET_UTILS_H
+#define _SNET_UTILS_H
+
+#include <linux/types.h>
+
+const char *snet_verdict_name(const enum snet_verdict cmd);
+const char *snet_syscall_name(const enum snet_syscall sys);
+
+#endif	/* _SNET_UTILS_H */
diff --git a/security/snet/snet_utils.c b/security/snet/snet_utils.c
new file mode 100644
index 0000000..6bfdcf6
--- /dev/null
+++ b/security/snet/snet_utils.c
@@ -0,0 +1,40 @@
+#include <linux/types.h>
+#include "snet_verdict.h"
+#include "snet_hooks.h"
+#include "snet_utils.h"
+
+const char *snet_verdict_name(const enum snet_verdict cmd)
+{
+	static const char *const verdict_name[] = {
+		[SNET_VERDICT_GRANT]	= "Grant",
+		[SNET_VERDICT_DENY]	= "Deny",
+		[SNET_VERDICT_PENDING]	= "Pending",
+		[SNET_VERDICT_NONE]	= "None",
+	};
+
+	if (cmd >= SNET_NR_VERDICT_TYPES)
+		return "INVALID";
+	else
+		return verdict_name[cmd];
+}
+
+const char *snet_syscall_name(const enum snet_syscall sys)
+{
+	static const char *const syscall_name[] = {
+		[SNET_SOCKET_CREATE]		= "Create",
+		[SNET_SOCKET_BIND]		= "Bind",
+		[SNET_SOCKET_CONNECT]		= "Connect",
+		[SNET_SOCKET_LISTEN]		= "Listen",
+		[SNET_SOCKET_ACCEPT]		= "Accept",
+		[SNET_SOCKET_POST_ACCEPT]	= "Post Accept",
+		[SNET_SOCKET_SENDMSG]		= "Sendmsg",
+		[SNET_SOCKET_RECVMSG]		= "Recvmsg",
+		[SNET_SOCKET_SOCK_RCV_SKB]	= "Sock Rcv Skb",
+		[SNET_SOCKET_CLOSE]		= "Close",
+	};
+
+	if (sys >= SNET_NR_SOCKET_TYPES)
+		return "INVALID";
+	else
+		return syscall_name[sys];
+}
-- 
1.6.3.3

Re: [RFC 0/9] snet: Security for NETwork syscalls

[jamal]

Hi Samir,

This fills in a gap i always thought was missing from LSM's 
boolean verdict policies. So good effort.

1)I would love to see the send/recvmsg interface complete (seems
missing).
2) If you can provide an async scheme which allows re-injection of
policy verdicts in addition to the sync interface, i think that would be
more valuable. I can see many apps which collect multiple states before
making a policy decision on multiple messages (example a multipart
message). Is SNET_VERDICT_PENDING intended for this?

A small glitch i noticed; you have defines in patches 8 and 9 which are
needed by patches 6 and 7. I think the general idea should be to compile
after adding each patch. So you may need to move some defines in earlier
patches.

cheers,
jamal

Re: [RFC 0/9] snet: Security for NETwork syscalls

[Samir Bellabes]

Hello Jamal,

jamal <hadi@cyberus.ca> writes:
> Hi Samir,
>
> This fills in a gap i always thought was missing from LSM's 
> boolean verdict policies. So good effort.
>
> 1)I would love to see the send/recvmsg interface complete (seems
> missing).

about the hook security_socket_sendmsg(), I added netlink attributes
SNET_A_BUFFER and SNET_A_BUFFERLEN, and get the buffer and the length
from the iov.iov_base and iov.iov_len at the security_socket_sendmsg()
so kernel part is now able to send those informations to userspace.

about sendmsg(), that's totaly different.
from what I understand, the call of the security_socket_recvmsg() is
made before the call of sock->ops->recvmsg(). As the buffer is not yet
copied until tcp_recvmsg(), no data are yet available at the
security_socket_recvmsg() hook.

I'm currently testing security_sock_rcv_skb() hook - which is inside
sk_filter() - to get skbuffs when then are arriving, and so trying to
push the buffer to userspace. In case this is not userfull, userspace
is able to use the NFQUEUE of netfilter to get skbuff, and deal
with incoming datas.
The idea in this later case is:
0. catching sshd listening on port TCP 12345, user is sam
1. receiving skbuff through NFQUEUE,
   skbuff shows it's TCP, and dport is 12345 
2. checking if we known the apps for this port
   (yes, it was catched at 0.)
3. DROP OR ACCEPT packet through NFQUEUE API regarding policy decision

the idea 'push security decision to userspace' is nothing if we don't
use all userspace APIs and tools.

> 2) If you can provide an async scheme which allows re-injection of
> policy verdicts in addition to the sync interface, i think that would be
> more valuable. I can see many apps which collect multiple states before
> making a policy decision on multiple messages (example a multipart
> message).

I didn't think about that yet. thanks
so let's start with a sync interface and mecanism, then we'll see what
we can do about this

> Is SNET_VERDICT_PENDING intended for this?

yes, SNET_VERDICT_PENDING the 'non-decision yet' state. so before
pushing the request to userspace, the verdict is set to this value.
I introduced a netlink attribute SNET_A_DELAY and a netlink command
SNET_C_DELAY, which provide the userspace the possibility increase the
timeout value for a specific request. path becomes :

kernel                                               userspace
request is PENDING timeout 5sec
push request to userspace
                           ----------->
                                        no decision is available yet
                                        DELAY the decision by 30 secs
                           <----------
increase the timeout value
for this verdict and wait

> A small glitch i noticed; you have defines in patches 8 and 9 which are
> needed by patches 6 and 7. I think the general idea should be to compile
> after adding each patch. So you may need to move some defines in earlier
> patches.

yes, currently, you have to apply all the patch in order.
for the true submission, I will introduce patch that compile at each
step. But for now I thought it's safe, because every code is in a new
source code file.

I will dig and fix Patrick's and Serge's remarks, then I will be back
with a new version.

thanks for your time Jamal
sam

Re: [RFC 0/9] snet: Security for NETwork syscalls

[Tetsuo Handa]

Samir Bellabes wrote:
> I'm currently testing security_sock_rcv_skb() hook - which is inside
> sk_filter() - to get skbuffs when then are arriving, and so trying to
> push the buffer to userspace. In case this is not userfull, userspace
> is able to use the NFQUEUE of netfilter to get skbuff, and deal
> with incoming datas.

Pushing buffer to userspace and wait for userspace's decision will sleep.
sk_filter() which calls security_sock_rcv_skb() hook is called from
(e.g.) tcp_v6_do_rcv() and comment of tcp_v6_do_rcv() says that
"The socket must have it's spinlock held when we get here." 
Also, comment of rxrpc_queue_rcv_skb() says that
"the caller must hold a lock on call->lock".
I think it is not permitted to do sleeping operation inside
security_sock_rcv_skb().

Re: [RFC 0/9] snet: Security for NETwork syscalls

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@I-love.SAKURA.ne.jp):
> Samir Bellabes wrote:
> > I'm currently testing security_sock_rcv_skb() hook - which is inside
> > sk_filter() - to get skbuffs when then are arriving, and so trying to
> > push the buffer to userspace. In case this is not userfull, userspace
> > is able to use the NFQUEUE of netfilter to get skbuff, and deal
> > with incoming datas.
> 
> Pushing buffer to userspace and wait for userspace's decision will sleep.
> sk_filter() which calls security_sock_rcv_skb() hook is called from
> (e.g.) tcp_v6_do_rcv() and comment of tcp_v6_do_rcv() says that
> "The socket must have it's spinlock held when we get here." 
> Also, comment of rxrpc_queue_rcv_skb() says that
> "the caller must hold a lock on call->lock".
> I think it is not permitted to do sleeping operation inside
> security_sock_rcv_skb().

Ah, there it is, thanks for finding a specific instance.  I'd looked
a bit yesterday but couldn't find it.

Tetsuo, would you mind sending a patch to add a note to security.h's
comment for socket_sock_rcv_skb: that it can't sleep?  It also used
to be the case for security_task_free() which presumably would be
hooked by the ssyscall or whatever was mentioned.

thanks,
-serge

[PATCH] LSM: Update comment on security_sock_rcv_skb

[Tetsuo Handa]

[PATCH] LSM: Update comment on security_sock_rcv_skb

It is not permitted to do sleeping operation inside security_sock_rcv_skb().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
--
diff --git a/include/linux/security.h b/include/linux/security.h
index 466cbad..3696ca3 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -978,6 +978,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Check permissions on incoming network packets.  This hook is distinct
  *	from Netfilter's IP input hooks since it is the first time that the
  *	incoming sk_buff @skb has been associated with a particular socket, @sk.
+ *	Must not sleep inside this hook because some callers hold spinlocks.
  *	@sk contains the sock (not socket) associated with the incoming sk_buff.
  *	@skb contains the incoming network data.
  * @socket_getpeersec_stream:

Re: [PATCH] LSM: Update comment on security_sock_rcv_skb

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@I-love.SAKURA.ne.jp):
> [PATCH] LSM: Update comment on security_sock_rcv_skb
> 
> It is not permitted to do sleeping operation inside security_sock_rcv_skb().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Serge Hallyn <serue@us.ibm.com>

Thank you for sending this.

-serge

> --
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 466cbad..3696ca3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -978,6 +978,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	Check permissions on incoming network packets.  This hook is distinct
>   *	from Netfilter's IP input hooks since it is the first time that the
>   *	incoming sk_buff @skb has been associated with a particular socket, @sk.
> + *	Must not sleep inside this hook because some callers hold spinlocks.
>   *	@sk contains the sock (not socket) associated with the incoming sk_buff.
>   *	@skb contains the incoming network data.
>   * @socket_getpeersec_stream:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] LSM: Update comment on security_sock_rcv_skb

[James Morris]

On Wed, 6 Jan 2010, Tetsuo Handa wrote:

> [PATCH] LSM: Update comment on security_sock_rcv_skb
> 
> It is not permitted to do sleeping operation inside security_sock_rcv_skb().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>


Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next


-- 
James Morris
<jmorris@namei.org>

Re: [RFC 0/9] snet: Security for NETwork syscalls

[jamal]

Hi Samir,
Apologies for the latency. Just caught up with the discussion.

On Tue, 2010-01-05 at 08:26 +0100, Samir Bellabes wrote:
> Hello Jamal,
> 
> jamal <hadi@cyberus.ca> writes:

> about the hook security_socket_sendmsg(), I added netlink attributes
> SNET_A_BUFFER and SNET_A_BUFFERLEN, and get the buffer and the length
> from the iov.iov_base and iov.iov_len at the security_socket_sendmsg()
> so kernel part is now able to send those informations to userspace.

This looks reasonable. Playing around with it will provide better
insight.

> about sendmsg(), that's totaly different.

I think you meant recvmsg

> from what I understand, the call of the security_socket_recvmsg() is
> made before the call of sock->ops->recvmsg(). As the buffer is not yet
> copied until tcp_recvmsg(), no data are yet available at the
> security_socket_recvmsg() hook.
> 

This is not true for all socket domains. Example, not true for unix or
tipc etc. In any case, I think it should be feasible to do the copy
earlier for udp/tcp/icmp and make it optional to turn on this
(early-copy) feature. 

> I'm currently testing security_sock_rcv_skb() hook - which is inside
> sk_filter() - to get skbuffs when then are arriving, and so trying to
> push the buffer to userspace. In case this is not userfull, userspace
> is able to use the NFQUEUE of netfilter to get skbuff, and deal
> with incoming datas.
> The idea in this later case is:
> 0. catching sshd listening on port TCP 12345, user is sam
> 1. receiving skbuff through NFQUEUE,
>    skbuff shows it's TCP, and dport is 12345 
> 2. checking if we known the apps for this port
>    (yes, it was catched at 0.)
> 3. DROP OR ACCEPT packet through NFQUEUE API regarding policy decision
> 
> the idea 'push security decision to userspace' is nothing if we don't
> use all userspace APIs and tools.
> 

I would rather have one unified interface instead of one from nfqueue
and another from your work. Besides, nfqueue works with a very limited
socket domains. It will be a lot easier to use the security hooks
instead.
I dont think it is wrong to replicate the nfqueue type approach for your
case.

> > 2) If you can provide an async scheme which allows re-injection of
> > policy verdicts in addition to the sync interface, i think that would be
> > more valuable. I can see many apps which collect multiple states before
> > making a policy decision on multiple messages (example a multipart
> > message).
> 
> I didn't think about that yet. thanks
> so let's start with a sync interface and mecanism, then we'll see what
> we can do about this

Could you not just replicate nfqueu approach?

> > Is SNET_VERDICT_PENDING intended for this?
> 
> yes, SNET_VERDICT_PENDING the 'non-decision yet' state. so before
> pushing the request to userspace, the verdict is set to this value.
> I introduced a netlink attribute SNET_A_DELAY and a netlink command
> SNET_C_DELAY, which provide the userspace the possibility increase the
> timeout value for a specific request. path becomes :
> 
> kernel                                               userspace
> request is PENDING timeout 5sec
> push request to userspace
>                            ----------->
>                                         no decision is available yet
>                                         DELAY the decision by 30 secs
>                            <----------
> increase the timeout value
> for this verdict and wait

This is still a synchronous approach with some workaround. It may be
sufficient but you have other problems in cases you cant sleep in;

For async, something along the nfqueue approach or ipsec/xfrm ACQUIRE
approach where state is maintained in the kernel, you shoot the data to
user space and terminate the code path; later on, the data is
re-injected, you lookup the state and continue processing would do.
Given that someone already pointed out that you will have problems
with some code paths because you cant sleep, i believe an async approach
will solve at least that particular issue.

cheers,
jamal