Date:	Thu, 9 Aug 2012 16:06:18 -0400
From:	Eric Paris <eparis@...isplace.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Paul Moore <paul@...l-moore.com>,
	David Miller <davem@...emloft.net>,
	Casey Schaufler <casey@...aufler-ca.com>,
	John Stultz <johnstul@...ibm.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	lkml <linux-kernel@...r.kernel.org>,
	James Morris <james.l.morris@...cle.com>,
	selinux@...ho.nsa.gov, john.johansen@...onical.com,
	LSM <linux-security-module@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

NAK.

I personally think commit be9f4a44e7d41cee should be reverted until it
is fixed.  Let me explain what all I believe it broke and how.

Old callchain of the creation of the 'equivalent' socket previous to
the patch in question just for reference:

    inet_ctl_sock_create
      sock_create_kern
        __sock_create
          pf->create (inet_create)
            sk_alloc
              sk_prot_alloc
                security_sk_alloc()


This WAS working properly.  All of it.  The equivalent struct sock was
being created and allocated in inet_create(), which called to
sk_alloc->sk_prot_alloc->security_sk_alloc().  We all agree that
failing to call security_sk_alloc() is the first regression
introduced.

The second regression was the labeling issue.  There was a call to
security_socket_post_create (from __sock_create) which was properly
setting the labels on both the socket and sock.  This new patch broke
that as well.  We don't expose an equivalent
security_sock_post_create() interface in the LSM currently, and until
we do, this can't be fixed.  It's why I say it should be reverted.

I have a patch I'm testing right now which takes care of the first
part the way I like (and yes, I'm doing the allocation on the correct
number node).  It basically looks like so:

+       for_each_possible_cpu(cpu) {
+               sock = &per_cpu(unicast_sock, cpu);
+               rc = security_sk_alloc(&sock->sk, PF_INET, GFP_KERNEL,
cpu_to_node(cpu));
+               if (rc)
+                       return rc;
+       }

I'm going to work right now on exposing the equivalent struct sock LSM
interface so we can call that as well.  But it's going to take me a
bit.  Attached is the patch just to (hopefully untested) shut up the
panic.

-Eric

On Thu, Aug 9, 2012 at 10:50 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> commit be9f4a44e7d41cee (ipv4: tcp: remove per net tcp_sock) added a
> selinux regression, reported and bisected by John Stultz
>
> selinux_ip_postroute_compat() expect to find a valid sk->sk_security
> pointer, but this field is NULL for unicast_sock
>
> Fix this by adding a new 'kernel' parameter to security_sk_alloc(),
> set to true if socket might already have a valid sk->sk_security
> pointer. ip_send_unicast_reply() uses a percpu fake socket, so the first
> call to security_sk_alloc() will populate sk->sk_security pointer,
> subsequent ones will reuse existing context.
>
> Reported-by: John Stultz <johnstul@...ibm.com>
> Bisected-by: John Stultz <johnstul@...ibm.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Paul Moore <paul@...l-moore.com>
> Cc: Eric Paris <eparis@...isplace.org>
> Cc: "Serge E. Hallyn" <serge@...lyn.com>
> ---
>  include/linux/security.h   |    6 +++---
>  net/core/sock.c            |    2 +-
>  net/ipv4/ip_output.c       |    4 +++-
>  security/security.c        |    4 ++--
>  security/selinux/hooks.c   |    5 ++++-
>  security/smack/smack_lsm.c |   10 ++++++++--
>  6 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 4e5a73c..4d8e454 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1601,7 +1601,7 @@ struct security_operations {
>         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);
> -       int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
> +       int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool kernel);
>         void (*sk_free_security) (struct sock *sk);
>         void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
>         void (*sk_getsecid) (struct sock *sk, u32 *secid);
> @@ -2539,7 +2539,7 @@ 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);
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel);
>  void security_sk_free(struct sock *sk);
>  void security_sk_clone(const struct sock *sk, struct sock *newsk);
>  void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
> @@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
>         return -ENOPROTOOPT;
>  }
>
> -static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
>  {
>         return 0;
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..e00cadf 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
>         if (sk != NULL) {
>                 kmemcheck_annotate_bitfield(sk, flags);
>
> -               if (security_sk_alloc(sk, family, priority))
> +               if (security_sk_alloc(sk, family, priority, false))
>                         goto out_free;
>
>                 if (!try_module_get(prot->owner))
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..b233d6e 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
>         sk->sk_priority = skb->priority;
>         sk->sk_protocol = ip_hdr(skb)->protocol;
>         sk->sk_bound_dev_if = arg->bound_dev_if;
> +       if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
> +               goto out;
>         sock_net_set(sk, net);
>         __skb_queue_head_init(&sk->sk_write_queue);
>         sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
>                 skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
>                 ip_push_pending_frames(sk, &fl4);
>         }
> -
> +out:
>         put_cpu_var(unicast_sock);
>
>         ip_rt_put(rt);
> diff --git a/security/security.c b/security/security.c
> index 860aeb3..23cf297 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
>  }
>  EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
>  {
> -       return security_ops->sk_alloc_security(sk, family, priority);
> +       return security_ops->sk_alloc_security(sk, family, priority, kernel);
>  }
>
>  void security_sk_free(struct sock *sk)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6c77f63..ccd4374 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4289,10 +4289,13 @@ out:
>         return 0;
>  }
>
> -static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
> +static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool kernel)
>  {
>         struct sk_security_struct *sksec;
>
> +       if (kernel && sk->sk_security)
> +               return 0;
> +
>         sksec = kzalloc(sizeof(*sksec), priority);
>         if (!sksec)
>                 return -ENOMEM;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8221514..0b066d0 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1749,20 +1749,26 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
>   * @sk: the socket
>   * @family: unused
>   * @gfp_flags: memory allocation flags
> + * @kernel: true if we should check sk_security being already set
>   *
>   * Assign Smack pointers to current
>   *
>   * Returns 0 on success, -ENOMEM is there's no memory
>   */
> -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> +static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool kernel)
>  {
> -       char *csp = smk_of_current();
> +       char *csp;
>         struct socket_smack *ssp;
>
> +       if (kernel && sk->sk_security)
> +               return 0;
> +
>         ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
>         if (ssp == NULL)
>                 return -ENOMEM;
>
> +       csp = kernel ? smack_net_ambient : smk_of_current();
> +
>         ssp->smk_in = csp;
>         ssp->smk_out = csp;
>         ssp->smk_packet = NULL;
>
>

Download attachment "tmp.patch" of type "application/octet-stream" (12667 bytes)

Powered by blists - more mailing lists