Message ID | 201007171017.DFC73498.SFFFOMLVJOHOtQ@I-love.SAKURA.ne.jp |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
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@I-love.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@hp.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);
Le samedi 17 juillet 2010 à 10:17 +0900, Tetsuo Handa a écrit : > 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? > > Regards. > ---------------------------------------- > >From b43154a90bc7494ec1ee301e692d2bbf29c8f2f8 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.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. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> I read this patch and could not find out if an SNMP counter was increased in the case a frame was not delivered but dropped in kernel land. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Dumazet wrote: > I read this patch and could not find out if an SNMP counter was > increased in the case a frame was not delivered but dropped in kernel > land. UDP_MIB_INDATAGRAMS and UDP_MIB_INERRORS will not be increased if dropped by security_socket_post_recvmsg()'s decision. Should we increment UDP_MIB_INDATAGRAMS and/or UDP_MIB_INERRORS? udpInDatagrams "The total number of UDP datagrams delivered to UDP users." udpNoPorts "The total number of received UDP datagrams for which there was no application at the destination port." udpInErrors "The number of received UDP datagrams that could not be delivered for reasons other than the lack of an application at the destination port." -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Sun, 18 Jul 2010 19:49:11 +0900 > Eric Dumazet wrote: >> I read this patch and could not find out if an SNMP counter was >> increased in the case a frame was not delivered but dropped in kernel >> land. > > UDP_MIB_INDATAGRAMS and UDP_MIB_INERRORS will not be increased > if dropped by security_socket_post_recvmsg()'s decision. > Should we increment UDP_MIB_INDATAGRAMS and/or UDP_MIB_INERRORS? This decision should be guided by what we do for in the case of the other existing security hooks. I don't think it makes any sense to make the post recvmsg() hook behave any differently from the existing hooks in this regard. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Sat, 17 Jul 2010 10:17:10 +0900 > NETWORKING [IPv4/IPv6] maintainers and Paul, is below patch fine for you? Unfortunately, after further consideration, I must reject this patch and also the post accept() LSM hook one. Sorry. I looked into history of the discussions on this issue, and I have found that the core issue with these hooks has not been addressed. We must ensure that if: 1) Application makes poll() on UDP socket in blocking mode, and UDP reports that receive data is available and 2) Application, after such a poll() call, makes a blocking recvmsg() call and no other activity has occurred on the socket meanwhile Then we MUST return immediately with that available data. This LSM hook, when it triggers, can violate this rule, even if you do this looping thing. The post accept() hook has the same problems. Here is where we originally discussed this, in detail: http://www.spinics.net/lists/netdev/msg95660.html Therefore, I think this shows that what Tomoyo is trying to do is fatally flawed. We brought this fundamental issue up to you about a year ago, and the issue is still not addressed. So consider very seriously, that what you are trying to do cannot be performed without breaking applications and API behavioral expectations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Tetsuo Handa > Date: Sat, 17 Jul 2010 10:17:10 +0900 > > > NETWORKING [IPv4/IPv6] maintainers and Paul, is below patch fine for you? > > Unfortunately, after further consideration, I must reject this patch > and also the post accept() LSM hook one. > > Sorry. > > I looked into history of the discussions on this issue, and I have found > that the core issue with these hooks has not been addressed. > > We must ensure that if: > > 1) Application makes poll() on UDP socket in blocking mode, and UDP > reports that receive data is available > > and > > 2) Application, after such a poll() call, makes a blocking recvmsg() call > and no other activity has occurred on the socket meanwhile > > Then we MUST return immediately with that available data. > > This LSM hook, when it triggers, can violate this rule, even if you do > this looping thing. > Existing LSM hooks already violate this rule. security_socket_accept() and security_socket_recvmsg() are allowed to return immediately with error code instead of available data even if conditions (1) and (2) are met. > The post accept() hook has the same problems. > > Here is where we originally discussed this, in detail: > > http://www.spinics.net/lists/netdev/msg95660.html > > Therefore, I think this shows that what Tomoyo is trying to do is > fatally flawed. We brought this fundamental issue up to you about a > year ago, and the issue is still not addressed. > > So consider very seriously, that what you are trying to do cannot be > performed without breaking applications and API behavioral > expectations. LSM is something that breaks applications and API behavioral expectations. For example, an application called access("/bin/sh", X_OK) and assumed that execution of /bin/sh will be permitted unless somebody does chmod("/bin/sh", 0) before the application calls execve("/bin/sh"). But however, if LSM's policy changed from "allow execution of /bin/sh" to "deny execution of /bin/sh" between access("/bin/sh", X_OK) and execve("/bin/sh"), the application was broken by the LSM. Although such change unlikely happens in normal circumstance, it can happen and we tolerate such breakage. Another example. An application called select() on non-socket object (e.g. regular file). The select() will say "read operation will not block" and the application will call read() with expectations that read() returns immediately with available data (or EOF) rather than error code unless somebody does chmod("the file", 0). But however, if LSM's policy changed from "allow reading the file" to "deny reading the file" between select() and read(), the application was broken by the LSM. Although such change unlikely happens in normal circumstance, it can happen and we tolerate such breakage. Another example. An application called socket(), bind() and listen(). A connection request arrived and enqueued into the listening socket's backlog. Now, select() starts saying "read operation will not block" and the application calls accept() with expectations that accept() returns immediately with established connection rather than error code. But however, if LSM's policy changed from "allow picking up the connection" to "deny picking up the connection" between select() and accept(), the application was broken by the LSM. Although such change unlikely happens in normal circumstance, it can happen and we tolerate such breakage. The only difference between security_socket_accept()/security_socket_recvmsg() and security_socket_post_accept()/security_socket_post_recvmsg() is that the connection/datagram in the queue is removed or not when these LSM hooks returned error code. Existing LSM hooks already made it impossible to return available data even if conditions (1) and (2) are met. Generally speaking, the connection/datagram being not removed from the queue when these LSM hooks returned error code might be preferable. But for TOMOYO, the connection/datagram being removed from the queue is preferable. Reasons shown below. TOMOYO is concerned with protecting applications with minimal side effects. Being unable to boot the system by granting chmod("/sbin/init", 0) or being unable to login to the system by granting rename("/etc/shadow", "/etc/shadow0") or being unable to allocate CPU time for each application by making specific applications to eat 100% of CPU time etc. are serious side effects for TOMOYO. Say, there are 100 connections/datagrams in the socket's queue and 1 out of 100 is the connection/datagram which should not be delivered to the application. (a) If the caller didn't close() the socket when security_socket_accept()/ security_socket_recvmsg() returned error code, subsequent select() will say "read operation will not block" and the caller will immediately call accept()/recvmsg() again. This lets the application to spend 100% of CPU time for only 1 connection/datagram which can not be picked up. This is nearly DoS for server side and completely DoS for client side. (b) If the caller close()d the socket when security_socket_accept()/ security_socket_recvmsg() returned error code, all queued connections/ datagrams are discarded. The connections/datagrams which should be delivered to the application are discarded (i.e. 99 connections/datagrams are disturbed by only 1 connection/datagram). Therefore, I will have to ask application developers to modify the application to call close(), socket(), bind(), listen(), accept() (regarding server side) and call close(), socket(), connect() (regarding client side) whenever security_socket_accept()/security_socket_recvmsg() returned an error. This is nearly DoS for client side. Silently dropping the 1 connection/datagram with returning non-fatal error code (e.g. -EAGAIN) (or wait for next connection/datagram unless MSG_DONTWAIT or O_NONBLOCK is set) seems to give minimal side effects to both server side and client side. But if you still cannot tolerate dropping the connection/datagram, what about below idea? int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, size_t len, int noblock, int flags, int *addr_len) { struct inet_sock *inet = inet_sk(sk); struct sockaddr_in *sin = (struct sockaddr_in *)msg->msg_name; struct sk_buff *skb; unsigned int ulen; int peeked; int err; int is_udplite = IS_UDPLITE(sk); bool slow; + bool peek_forced; /* * Check any passed addresses */ if (addr_len) *addr_len = sizeof(*sin); if (flags & MSG_ERRQUEUE) return ip_recv_error(sk, msg, len); + /* LSM wants to decide permission based on skb? */ + peek_forced = security_socket_recvmsg_force_peek(sk); try_again: - skb = __skb_recv_datagram(sk, flags | (noblock ? MSG_DONTWAIT : 0), - &peeked, &err); + skb = __skb_recv_datagram(sk, flags | (noblock ? MSG_DONTWAIT : 0) | + (peek_forced ? MSG_PEEK : 0), &peeked, &err); if (!skb) goto out; + if (peek_forced) { + err = security_socket_post_recvmsg(sk, skb); + if (err < 0) { + /* + * Do not remove this message from queue because LSM + * decided not to deliver this message to the caller. + */ + peek_forced = false; + goto out_free; + } + } ulen = skb->len - sizeof(struct udphdr); if (len > ulen) len = ulen; else if (len < ulen) msg->msg_flags |= MSG_TRUNC; (...snipped...) out_free: + if (peek_forced && !(flags & MSG_PEEK)) { + /* + * Remove this message from queue because this message was + * peeked for LSM but the caller did not ask to peek. + */ + slow = lock_sock_fast(sk); + skb_kill_datagram(sk, skb, flags); + unlock_sock_fast(sk, slow); + goto out; + } skb_free_datagram_locked(sk, skb); out: return err; (...snipped...) } where security_socket_recvmsg_force_peek() returns true (if LSM module wants to do permission check based on skb) or false (if LSM module does not want to do permission check based on skb) and security_socket_post_recvmsg() returns error code (if LSM module decided not to deliver) or 0 (if LSM module decided to deliver). security_socket_post_recvmsg() must not call skb_kill_datagram(). In this way, security_socket_post_recvmsg() can keep socket's queue state intact like security_socket_recvmsg() (but side effects (a) and (b) remains as well as security_socket_recvmsg() hook). Do permission checks upon enqueue time and do not perform permission check upon dequeue time cannot be an answer. Side effects with security_socket_accept()/ security_socket_recvmsg() are what SELinux is experiencing as well as TOMOYO will experience. (Though, it seems to me that SELinux is not interested in such side effects.) Regards. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Your analysis is wrong, and what Tomoyo is doing is so fundamentally different than what the existing SELINUX hooks do. The existing LSM hooks do not break BSD socket behavior. Do you know why? Because someone who understood all of this spent a great deal of time carefully designing them. The existing hooks do not drop packets on recvmsg() when a security check does not pass, they signal the error long before the socket receive queue is even looked at. It is just like seeing a -EFAULT, -ENFILE, or similar error. Checks are always made _BEFORE_ major state changes are made to the socket. That is critically important, and it's what you seem to fail to see. The hooks you propose _LOSE_ information. So even if another process has the 'fd' for a socket, and they would be allowed to receive the packet by LSM checks, the post hook does not allow that to happen because the failing 'fd' just frees up the packet and loses it forever. The existing hooks signal before we pull the new connection out of the accept queue during accept(), therefore avoiding the illegal situation your post ->accept() hook would create since there is absolutely no way (and there should not be a way) to push a connection back into the sock accept queue after we've taken it from the protocol layer. And again here, the proposed hooks _LOSE_ information. The accepted connection is lost forever, another process with valid security credentials cannot accept the connection. It is completely gone. And I'm not even going to entertain adding facilities to allow pushing things back into the socket state after they've been removed for inspection. I think we've been through this issue enough times that we have covered the issues in their entirety, and nothing you have written convinces me that my position is wrong and that it is valid to put the Tomoyo post-recvmsg and post-accept hooks into the tree. Sorry, but I'm not applying your patches, they are fundamentally flawed unlike the existing hooks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > Your analysis is wrong, and what Tomoyo is doing is so fundamentally > different than what the existing SELINUX hooks do. > > The existing LSM hooks do not break BSD socket behavior. Do you know > why? Because someone who understood all of this spent a great deal > of time carefully designing them. > > The existing hooks do not drop packets on recvmsg() when a security > check does not pass, they signal the error long before the socket > receive queue is even looked at. It is just like seeing a -EFAULT, > -ENFILE, or similar error. > > Checks are always made _BEFORE_ major state changes are made to the > socket. Excuse me, below check is made inside recvmsg() and may return error if SELinux's policy has changed after the select() said "ready" and before security_socket_recvmsg() is called. No? int avc_has_perm_noaudit(u32 ssid, u32 tsid, u16 tclass, u32 requested, unsigned flags, struct av_decision *in_avd) { struct avc_node *node; struct av_decision avd_entry, *avd; int rc = 0; u32 denied; BUG_ON(!requested); rcu_read_lock(); node = avc_lookup(ssid, tsid, tclass); if (!node) { rcu_read_unlock(); if (in_avd) avd = in_avd; else avd = &avd_entry; security_compute_av(ssid, tsid, tclass, avd); rcu_read_lock(); node = avc_insert(ssid, tsid, tclass, avd); } else { if (in_avd) memcpy(in_avd, &node->ae.avd, sizeof(*in_avd)); avd = &node->ae.avd; } denied = requested & ~(avd->allowed); if (denied) { if (flags & AVC_STRICT) rc = -EACCES; else if (!selinux_enforcing || (avd->flags & AVD_FLAGS_PERMISSIVE)) avc_update_node(AVC_CALLBACK_GRANT, requested, ssid, tsid, tclass, avd->seqno); else rc = -EACCES; } rcu_read_unlock(); return rc; } int avc_has_perm(u32 ssid, u32 tsid, u16 tclass, u32 requested, struct common_audit_data *auditdata) { struct av_decision avd; int rc; rc = avc_has_perm_noaudit(ssid, tsid, tclass, requested, 0, &avd); avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata); return rc; } static int socket_has_perm(struct task_struct *task, struct socket *sock, u32 perms) { struct inode_security_struct *isec; struct common_audit_data ad; u32 sid; int err = 0; isec = SOCK_INODE(sock)->i_security; if (isec->sid == SECINITSID_KERNEL) goto out; sid = task_sid(task); COMMON_AUDIT_DATA_INIT(&ad, NET); ad.u.net.sk = sock->sk; err = avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad); out: return err; } static int selinux_socket_recvmsg(struct socket *sock, struct msghdr *msg, int size, int flags) { return socket_has_perm(current, sock, SOCKET__READ); } static struct security_operations selinux_ops = { (...snipped...) .socket_recvmsg = selinux_socket_recvmsg, (...snipped...) }; Are you saying that selinux_socket_recvmsg() always returns 0? > That is critically important, and it's what you seem to fail to see. > > The hooks you propose _LOSE_ information. So even if another process > has the 'fd' for a socket, and they would be allowed to receive the > packet by LSM checks, the post hook does not allow that to happen > because the failing 'fd' just frees up the packet and loses it > forever. > > The existing hooks signal before we pull the new connection out of the > accept queue during accept(), therefore avoiding the illegal situation > your post ->accept() hook would create since there is absolutely no > way (and there should not be a way) to push a connection back into the > sock accept queue after we've taken it from the protocol layer. > > And again here, the proposed hooks _LOSE_ information. The accepted > connection is lost forever, another process with valid security > credentials cannot accept the connection. It is completely gone. > > And I'm not even going to entertain adding facilities to allow pushing > things back into the socket state after they've been removed for > inspection. > > I think we've been through this issue enough times that we have covered > the issues in their entirety, and nothing you have written convinces > me that my position is wrong and that it is valid to put the Tomoyo > post-recvmsg and post-accept hooks into the tree. > > Sorry, but I'm not applying your patches, they are fundamentally flawed > unlike the existing hooks. Did the idea described in previous mail _LOSE_ information? I made the udp_recvmsg() to force MSG_PEEK so that the message will not be removed from the queue if security_socket_post_recvmsg() returned error code and remove the message from the queue only if security_socket_post_recvmsg() returned 0 and the caller did not pass MSG_PEEK. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Thu, 22 Jul 2010 13:41:38 +0900 > Excuse me, below check is made inside recvmsg() and may return error if > SELinux's policy has changed after the select() said "ready" and before > security_socket_recvmsg() is called. No? It does this before pulling the packet out of the receive queue of the socket. It's like signalling a parameter error to the process, no socket state is changed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Tetsuo Handa > Date: Thu, 22 Jul 2010 13:41:38 +0900 > > > Excuse me, below check is made inside recvmsg() and may return error if > > SELinux's policy has changed after the select() said "ready" and before > > security_socket_recvmsg() is called. No? > > It does this before pulling the packet out of the receive queue of the > socket. It's like signalling a parameter error to the process, no > socket state is changed. So, we agreed that security_socket_recvmsg() is allowed to return error code rather than available data even if both conditions 1) Application makes poll() on UDP socket in blocking mode, and UDP reports that receive data is available and 2) Application, after such a poll() call, makes a blocking recvmsg() call and no other activity has occurred on the socket meanwhile are met. Then, why does below proposal lose information? The message is not removed if security_socket_post_recvmsg() returned error code. int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, size_t len, int noblock, int flags, int *addr_len) { struct inet_sock *inet = inet_sk(sk); struct sockaddr_in *sin = (struct sockaddr_in *)msg->msg_name; struct sk_buff *skb; unsigned int ulen; int peeked; int err; int is_udplite = IS_UDPLITE(sk); bool slow; + bool peek_forced; /* * Check any passed addresses */ if (addr_len) *addr_len = sizeof(*sin); if (flags & MSG_ERRQUEUE) return ip_recv_error(sk, msg, len); + /* LSM wants to decide permission based on skb? */ + peek_forced = security_socket_recvmsg_force_peek(sk); try_again: - skb = __skb_recv_datagram(sk, flags | (noblock ? MSG_DONTWAIT : 0), - &peeked, &err); + skb = __skb_recv_datagram(sk, flags | (noblock ? MSG_DONTWAIT : 0) | + (peek_forced ? MSG_PEEK : 0), &peeked, &err); if (!skb) goto out; + if (peek_forced) { + err = security_socket_post_recvmsg(sk, skb); + if (err < 0) { + /* + * Do not remove this message from queue because LSM + * decided not to deliver this message to the caller. + */ + peek_forced = false; + goto out_free; + } + } ulen = skb->len - sizeof(struct udphdr); if (len > ulen) len = ulen; else if (len < ulen) msg->msg_flags |= MSG_TRUNC; (...snipped...) out_free: + if (peek_forced && !(flags & MSG_PEEK)) { + /* + * Remove this message from queue because this message was + * peeked for LSM but the caller did not ask to peek. + */ + slow = lock_sock_fast(sk); + skb_kill_datagram(sk, skb, flags); + unlock_sock_fast(sk, slow); + goto out; + } skb_free_datagram_locked(sk, skb); out: return err; (...snipped...) } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Thu, 22 Jul 2010 14:02:16 +0900 > Then, why does below proposal lose information? Peek changes state, now it's possible that two processes end up receiving the packet. Please consider deeply how your desired semantics are unobtainable without breaking thigngs fundamentally. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > > Then, why does below proposal lose information? > > Peek changes state, now it's possible that two processes end up > receiving the packet. Indeed. We will need to protect sock->ops->recvmsg() call using a lock like static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t size, int flags) { + int err; struct sock_iocb *si = kiocb_to_siocb(iocb); sock_update_classid(sock->sk); si->sock = sock; si->scm = NULL; si->msg = msg; si->size = size; si->flags = flags; - return sock->ops->recvmsg(iocb, sock, msg, size, flags); + err = security_socket_read_lock(sock); + if (err) + return err; + err = sock->ops->recvmsg(iocb, sock, msg, size, flags); + security_socket_read_unlock(sock); + return err; } in addition to security_socket_recvmsg_force_peek() and security_socket_post_recvmsg(). But locks like above break MSG_DONTWAIT since recv() without MSG_DONTWAIT calls wait_for_packet() inside __skb_recv_datagram(). To make MSG_DONTWAIT work, I have to do like below. struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned flags, int *peeked, int *err) (...snipped...) do { /* Again only user level code calls this function, so nothing * interrupt level will suddenly eat the receive_queue. * * Look at current nfs client by the way... * However, this function was corrent in any case. 8) */ unsigned long cpu_flags; + /* < 0 if lock failed, 0 if no need to lock, > 0 if locked */ + int serialized = security_socket_read_lock(sk); + if (serialized < 0) { + error = serialized; + goto no_packet; + } else if (serialized > 0) { + int err; + spin_lock_irqsave(&sk->sk_receive_queue.lock, cpu_flags); + skb = skb_peek(&sk->sk_receive_queue); + spin_unlock_irqrestore(&sk->sk_receive_queue.lock, + cpu_flags); + if (!skb) + goto no_skb; + err = security_socket_pre_recvmsg(sk, skb); + if (err < 0) { + error = err; + security_socket_read_unlock(sk); + goto no_packet; + } + } + spin_lock_irqsave(&sk->sk_receive_queue.lock, cpu_flags); skb = skb_peek(&sk->sk_receive_queue); if (skb) { *peeked = skb->peeked; if (flags & MSG_PEEK) { skb->peeked = 1; atomic_inc(&skb->users); } else __skb_unlink(skb, &sk->sk_receive_queue); } spin_unlock_irqrestore(&sk->sk_receive_queue.lock, cpu_flags); +no_skb: + if (serialized > 0) + security_socket_read_unlock(sk); if (skb) return skb; /* User doesn't want to wait */ error = -EAGAIN; if (!timeo) goto no_packet; } while (!wait_for_packet(sk, err, &timeo)); (...snipped...) } Inserting LSM hooks like above will be the only way to work properly (i.e. handle MSG_DONTWAIT and avoid showing the same message to multiple readers and keep the queue's state unchanged upon error). But you said ( http://marc.info/?l=linux-netdev&m=124022463014713&w=2 ) > We worked so hard to split out this common code, it is simply > a non-starter for anyone to start putting protocol specific test > into here, or even worse to move this code back to being locally > copied into every protocol implementation. when I proposed inserting LSM hooks into __skb_recv_datagram() ( http://marc.info/?l=linux-netdev&m=124022463014672&w=2 ). So, I have no way to allow performing permission checks based on combination of "process who issued recv() request" and "source address/port of the message which the process is about to pick up" without breaking things (unless you accept inserting LSM hooks into __skb_recv_datagram())... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Thu, 22 Jul 2010 21:46:55 +0900 > David Miller wrote: >> > Then, why does below proposal lose information? >> >> Peek changes state, now it's possible that two processes end up >> receiving the packet. > > Indeed. We will need to protect sock->ops->recvmsg() call using a lock like But this doesn't matter. The fact is going to remain that you will be unable to return data from recvmsg() to a blocking socket when ->poll() returns true even though data is in fact there in the socket receive queue. This is something that the existing LSM hooks do not do. You can't create this silly situation where some packets in the socket receive queue can be recvmsg()'d by some processes, but not by others. At best, it is pure crazyness. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Thu, 22 Jul 2010 10:22:51 -0700 (PDT) > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Thu, 22 Jul 2010 21:46:55 +0900 > >> David Miller wrote: >>> > Then, why does below proposal lose information? >>> >>> Peek changes state, now it's possible that two processes end up >>> receiving the packet. >> >> Indeed. We will need to protect sock->ops->recvmsg() call using a lock like > > But this doesn't matter. Also, btw, we're not adding a lock to a code path which we've worked so hard to make largely lockless. This lock is going to kill performance. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > The fact is going to remain that you will be unable to return data > from recvmsg() to a blocking socket when ->poll() returns true even > though data is in fact there in the socket receive queue. > > This is something that the existing LSM hooks do not do. This is something that the existing security_socket_recvmsg() hook does do. SELinux is unable to return data from recvmsg() to a blocking socket when ->poll() returns true even though data is in fact there in the socket receive queue. We agreed below situation, didn't we? Tetsuo Handa wrote: > > > Excuse me, below check is made inside recvmsg() and may return error if > > > SELinux's policy has changed after the select() said "ready" and before > > > security_socket_recvmsg() is called. No? > > > > It does this before pulling the packet out of the receive queue of the > > socket. It's like signalling a parameter error to the process, no > > socket state is changed. > > So, we agreed that security_socket_recvmsg() is allowed to return error code > rather than available data even if both conditions > > 1) Application makes poll() on UDP socket in blocking mode, and UDP > reports that receive data is available > > and > > 2) Application, after such a poll() call, makes a blocking recvmsg() call > and no other activity has occurred on the socket meanwhile > > are met. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Fri, 23 Jul 2010 09:22:20 +0900 > David Miller wrote: >> The fact is going to remain that you will be unable to return data >> from recvmsg() to a blocking socket when ->poll() returns true even >> though data is in fact there in the socket receive queue. >> >> This is something that the existing LSM hooks do not do. > > This is something that the existing security_socket_recvmsg() hook does do. > SELinux is unable to return data from recvmsg() to a blocking socket when > ->poll() returns true even though data is in fact there in the socket receive > queue. > We agreed below situation, didn't we? Existing LSM hook returns an error early, as if the user passed in incorrect parameters or similar. It's completely stateless and dependent upon purely the labels associated with state visible on recvmsg() entry, and independent of other things such as attributes in the packets contained in the socket's receive queue. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > From: Tetsuo Handa > Date: Fri, 23 Jul 2010 09:22:20 +0900 > > > David Miller wrote: > >> The fact is going to remain that you will be unable to return data > >> from recvmsg() to a blocking socket when ->poll() returns true even > >> though data is in fact there in the socket receive queue. > >> > >> This is something that the existing LSM hooks do not do. > > > > This is something that the existing security_socket_recvmsg() hook does do. > > SELinux is unable to return data from recvmsg() to a blocking socket when > > ->poll() returns true even though data is in fact there in the socket receive > > queue. > > We agreed below situation, didn't we? > > Existing LSM hook returns an error early, as if the user passed in > incorrect parameters or similar. > > It's completely stateless and dependent upon purely the labels > associated with state visible on recvmsg() entry, and independent > of other things such as attributes in the packets contained in > the socket's receive queue. Users calling recvmsg() after select() said "read operation will return with available data without blocking as long as you pass correct parameters" will get error code even if they passed correct parameters. If they passed incorrect parameters, they must accept the error code. But they passed correct parameters, and they expected that recvmsg() returns success with available data. From the point of view of select()->recvmsg() users, they don't care whether internal implementation is stateful and/or dependent of other things or not, they care what the specification says. My understanding was that the specification requires If select() said "read operation will return with available data without blocking", recvmsg() must return available data. No access control mechanism in recvmsg() path is allowed to reject the request. and security_socket_recvmsg() conflicts with this understanding. I'd like to see the specification which states disclaimers like below. (i) Even if select() said "read operation will return with available data without blocking", recvmsg() is allowed to return error code rather than available data when an access control mechanism in recvmsg() path rejected the request. (ii) The access control mechanism in recvmsg() path may use attribute of the socket. (iii) The access control mechanism in recvmsg() path must not use the packets contained in the socket's receive queue. We agreed on (i) and I don't mind (ii). Would you please show me URLs to the specification which states (iii)? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Fri, 23 Jul 2010 16:21:38 +0900 > Users calling recvmsg() after select() said "read operation will return with > available data without blocking as long as you pass correct parameters" will > get error code even if they passed correct parameters. This is nonsense, because not all "parameters" are not visible to poll(). These "parameters" I speak of are virtual, they are my way of saying that the errors returned by the existing LSM hooks are more like -EFAULT or -EACCESS than -EINTR/-EAGAIN or blocking, the latter of which are what are illegal for blocking socket invoking recvmsg() after poll indicates there is data to read. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > The fact is going to remain that you will be unable to return data > from recvmsg() to a blocking socket when ->poll() returns true even > though data is in fact there in the socket receive queue. > Maybe I misunderstood here. Are you worrying that TOMOYO will no longer be able to deliver remaining packets in a receive queue once a packet from address/port which is not permitted to pick up reached the head of the receive queue? (Please answer "yes" or "no" here.) If your answer is "yes", I tolerate the side effect (i.e. applications have to close and recreate the socket in order to resume receiving packets). My preferred behavior is to drop the packet but you will not accept it. I accept not to drop the packet if you can accept security_socket_pre_recvmsg(). If your answer is "no", let me restart from reconfirming requirements which LSM hook for recvmsg() must satisfy, before continuing discussion on security_socket_pre_recvmsg() hook. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);