Date:	Sat, 17 Jul 2010 16:34:50 -0400
From:	Paul Moore <paul.moore@...com>
To:	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:	davem@...emloft.net, kuznet@....inr.ac.ru, pekkas@...core.fi,
	jmorris@...ei.org, yoshfuji@...ux-ipv6.org, kaber@...sh.net,
	netdev@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH] LSM: Add post recvmsg() hook.

On Friday, July 16, 2010 09:17:10 pm Tetsuo Handa wrote:
> David Miller wrote:
> > From: Tetsuo Handa
> > Date: Sat, 17 Jul 2010 01:14:38 +0900
> > 
> > > Below is a patch for post recvmsg() operation. I modified the patch to
> > > call skb_recv_datagram() again (for udp_recvmsg(), raw_recvmsg(),
> > > udpv6_recvmsg()) if LSM dicided to drop the message. (Regarding
> > > rawv6_recvmsg(), I didn't do so in accordance with the comment at
> > > "csum_copy_err:".)
> > > What do you think about this verion?
> > 
> > This looks fine, but regardless of that comment I think the IPV6 raw
> > recvmsg() should loop just as the IPV4 one does in your patch.
> 
> Thank you, David.
> I updated to call skb_recv_datagram() for rawv6_recvmsg() case too.
> 
> NETWORKING [IPv4/IPv6] maintainers and Paul, is below patch fine for you?

Comments below ...

> >From b43154a90bc7494ec1ee301e692d2bbf29c8f2f8 Mon Sep 17 00:00:00 2001
> 
> From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Date: Sat, 17 Jul 2010 09:52:38 +0900
> Subject: [PATCH] LSM: Add post recvmsg() hook.
> 
> Current pre recvmsg hook (i.e. security_socket_recvmsg()) has two problems.
> 
> One is that it will cause eating 100% of CPU time if the caller does not
> close() the socket when recvmsg() failed due to security_socket_recvmsg(),
> for subsequent select() notifies the caller of readiness for recvmsg()
> since the datagram which would have been already picked up if
> security_socket_recvmsg() did not return error is remaining in the queue.
> 
> The other is that it is racy if LSM module wants to do filtering based on
> "which process can pick up datagrams from which source" because the process
> which picks up the datagram is not known until skb_recv_datagram() and lock
> is not held between security_socket_recvmsg() and skb_recv_datagram().
> 
> This patch introduces post recvmsg hook (i.e.
> security_socket_post_recvmsg()) in order to solve above problems at the
> cost of ability to pick up the datagram which would have been picked up if
> preceding security_socket_post_recvmsg() did not return error.

We've had discussions before about the merits of queuing inbound packets to 
the socket buffer only to later reject them when the application reads from 
the socket.  I'd be much happier to see you drop the packets before queuing 
them to the socket, e.g. security_sock_rcv_skb(), but I understand that isn't 
possible with TOMOYO's approach to security.

At least we're not talking about TCP sockets :)

I'll go ahead and add my ACK to this patch, but I wonder if it makes more 
sense in the UDP path to add the LSM hook after the decision to calculate the 
checksum prior to the copy?  If we're going to reject the packet due to a bad 
checksum we might as well do that before we waste our time with the LSM 
processing - right?  Although, if we end up doing checksum verification with 
the copy in the majority of the cases it may not be worth it.

Acked-by: Paul Moore <paul.moore@...com>

> ---
>  include/linux/security.h |   14 ++++++++++++++
>  net/ipv4/raw.c           |   12 +++++++++---
>  net/ipv4/udp.c           |    9 ++++++++-
>  net/ipv6/raw.c           |   12 +++++++++---
>  net/ipv6/udp.c           |    9 ++++++++-
>  security/capability.c    |    6 ++++++
>  security/security.c      |    6 ++++++
>  7 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 723a93d..409c44d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -879,6 +879,12 @@ static inline void security_free_mnt_opts(struct
> security_mnt_opts *opts) *	@size contains the size of message structure.
>   *	@flags contains the operational flags.
>   *	Return 0 if permission is granted.
> + * @socket_post_recvmsg:
> + *	Check permission after receiving a message from a socket.
> + *	The message is discarded if permission is not granted.
> + *	@sk contains the sock structure.
> + *	@skb contains the sk_buff structure.
> + *	Return 0 if permission is granted.
>   * @socket_getsockname:
>   *	Check permission before the local address (name) of the socket object
>   *	@sock is retrieved.
> @@ -1575,6 +1581,7 @@ struct security_operations {
>  			       struct msghdr *msg, int size);
>  	int (*socket_recvmsg) (struct socket *sock,
>  			       struct msghdr *msg, int size, int flags);
> +	int (*socket_post_recvmsg) (struct sock *sk, struct sk_buff *skb);
>  	int (*socket_getsockname) (struct socket *sock);
>  	int (*socket_getpeername) (struct socket *sock);
>  	int (*socket_getsockopt) (struct socket *sock, int level, int optname);
> @@ -2526,6 +2533,7 @@ int security_socket_accept(struct socket *sock,
> struct socket *newsock); int security_socket_sendmsg(struct socket *sock,
> struct msghdr *msg, int size); int security_socket_recvmsg(struct socket
> *sock, struct msghdr *msg, int size, int flags);
> +int security_socket_post_recvmsg(struct sock *sk, struct sk_buff *skb);
>  int security_socket_getsockname(struct socket *sock);
>  int security_socket_getpeername(struct socket *sock);
>  int security_socket_getsockopt(struct socket *sock, int level, int
> optname); @@ -2617,6 +2625,12 @@ static inline int
> security_socket_recvmsg(struct socket *sock, return 0;
>  }
> 
> +static inline int security_socket_post_recvmsg(struct sock *sk,
> +					       struct sk_buff *skb)
> +{
> +	return 0;
> +}
> +
>  static inline int security_socket_getsockname(struct socket *sock)
>  {
>  	return 0;
> diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
> index 2c7a163..69652d4 100644
> --- a/net/ipv4/raw.c
> +++ b/net/ipv4/raw.c
> @@ -676,9 +676,15 @@ static int raw_recvmsg(struct kiocb *iocb, struct sock
> *sk, struct msghdr *msg, goto out;
>  	}
> 
> -	skb = skb_recv_datagram(sk, flags, noblock, &err);
> -	if (!skb)
> -		goto out;
> +	for (;;) {
> +		skb = skb_recv_datagram(sk, flags, noblock, &err);
> +		if (!skb)
> +			goto out;
> +		err = security_socket_post_recvmsg(sk, skb);
> +		if (likely(!err))
> +			break;
> +		skb_kill_datagram(sk, skb, flags);
> +	}
> 
>  	copied = skb->len;
>  	if (len < copied) {
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 5858574..9145685 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1125,6 +1125,7 @@ int udp_recvmsg(struct kiocb *iocb, struct sock *sk,
> struct msghdr *msg, int err;
>  	int is_udplite = IS_UDPLITE(sk);
>  	bool slow;
> +	bool update_stat;
> 
>  	/*
>  	 *	Check any passed addresses
> @@ -1140,6 +1141,12 @@ try_again:
>  				  &peeked, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recvmsg(sk, skb);
> +	if (err) {
> +		update_stat = false;
> +		goto csum_copy_err;
> +	}
> +	update_stat = true;
> 
>  	ulen = skb->len - sizeof(struct udphdr);
>  	if (len > ulen)
> @@ -1200,7 +1207,7 @@ out:
> 
>  csum_copy_err:
>  	slow = lock_sock_fast(sk);
> -	if (!skb_kill_datagram(sk, skb, flags))
> +	if (!skb_kill_datagram(sk, skb, flags) && update_stat)
>  		UDP_INC_STATS_USER(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
>  	unlock_sock_fast(sk, slow);
> 
> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> index 4a4dcbe..6915b01 100644
> --- a/net/ipv6/raw.c
> +++ b/net/ipv6/raw.c
> @@ -464,9 +464,15 @@ static int rawv6_recvmsg(struct kiocb *iocb, struct
> sock *sk, if (np->rxpmtu && np->rxopt.bits.rxpmtu)
>  		return ipv6_recv_rxpmtu(sk, msg, len);
> 
> -	skb = skb_recv_datagram(sk, flags, noblock, &err);
> -	if (!skb)
> -		goto out;
> +	for (;;) {
> +		skb = skb_recv_datagram(sk, flags, noblock, &err);
> +		if (!skb)
> +			goto out;
> +		err = security_socket_post_recvmsg(sk, skb);
> +		if (likely(!err))
> +			break;
> +		skb_kill_datagram(sk, skb, flags);
> +	}
> 
>  	copied = skb->len;
>  	if (copied > len) {
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 87be586..6cae276 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -329,6 +329,7 @@ int udpv6_recvmsg(struct kiocb *iocb, struct sock *sk,
>  	int is_udplite = IS_UDPLITE(sk);
>  	int is_udp4;
>  	bool slow;
> +	bool update_stat;
> 
>  	if (addr_len)
>  		*addr_len=sizeof(struct sockaddr_in6);
> @@ -344,6 +345,12 @@ try_again:
>  				  &peeked, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recvmsg(sk, skb);
> +	if (err) {
> +		update_stat = false;
> +		goto csum_copy_err;
> +	}
> +	update_stat = true;
> 
>  	ulen = skb->len - sizeof(struct udphdr);
>  	if (len > ulen)
> @@ -426,7 +433,7 @@ out:
> 
>  csum_copy_err:
>  	slow = lock_sock_fast(sk);
> -	if (!skb_kill_datagram(sk, skb, flags)) {
> +	if (!skb_kill_datagram(sk, skb, flags) && update_stat) {
>  		if (is_udp4)
>  			UDP_INC_STATS_USER(sock_net(sk),
>  					UDP_MIB_INERRORS, is_udplite);
> diff --git a/security/capability.c b/security/capability.c
> index 4aeb699..709aea3 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -597,6 +597,11 @@ static int cap_socket_recvmsg(struct socket *sock,
> struct msghdr *msg, return 0;
>  }
> 
> +static int cap_socket_post_recvmsg(struct sock *sk, struct sk_buff *skb)
> +{
> +	return 0;
> +}
> +
>  static int cap_socket_getsockname(struct socket *sock)
>  {
>  	return 0;
> @@ -1001,6 +1006,7 @@ void __init security_fixup_ops(struct
> security_operations *ops) set_to_cap_if_null(ops, socket_accept);
>  	set_to_cap_if_null(ops, socket_sendmsg);
>  	set_to_cap_if_null(ops, socket_recvmsg);
> +	set_to_cap_if_null(ops, socket_post_recvmsg);
>  	set_to_cap_if_null(ops, socket_getsockname);
>  	set_to_cap_if_null(ops, socket_getpeername);
>  	set_to_cap_if_null(ops, socket_setsockopt);
> diff --git a/security/security.c b/security/security.c
> index e8c87b8..4291bd7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1037,6 +1037,12 @@ int security_socket_recvmsg(struct socket *sock,
> struct msghdr *msg, return security_ops->socket_recvmsg(sock, msg, size,
> flags);
>  }
> 
> +int security_socket_post_recvmsg(struct sock *sk, struct sk_buff *skb)
> +{
> +	return security_ops->socket_post_recvmsg(sk, skb);
> +}
> +EXPORT_SYMBOL(security_socket_post_recvmsg);
> +
>  int security_socket_getsockname(struct socket *sock)
>  {
>  	return security_ops->socket_getsockname(sock);

-- 
paul moore
linux @ hp
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists