[PATCH v3] selinux: avoid sk_socket dereference in selinux_sctp_bind_connect()

[PATCH v3] selinux: avoid sk_socket dereference in selinux_sctp_bind_connect()

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

selinux_sctp_bind_connect() dereferences sk->sk_socket to pass a
struct socket * to selinux_socket_bind() and
selinux_socket_connect_helper().  However, when the hook is invoked
from the ASCONF softirq path (sctp_process_asconf), there is no file
reference guaranteeing that sk->sk_socket is non-NULL.  The setsockopt
callers (bindx, connectx, set_primary, sendmsg connect) hold a file
reference and are not affected.

Both selinux_socket_bind() and selinux_socket_connect_helper()
immediately resolve sock->sk, never using the struct socket * for
anything else.  Refactor the inner logic into helpers that take a
struct sock * directly so that selinux_sctp_bind_connect() never needs
to touch sk->sk_socket at all.

Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v3:
  - Keep comment describing IPv4/IPv6 address processing loop
    (Stephen Smalley).

Changes in v2:
  - Refactor selinux_socket_bind() and selinux_socket_connect_helper()
    into sk-based inner helpers instead of adding a NULL check on
    sk->sk_socket (Stephen Smalley).

 security/selinux/hooks.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc926d3..1f202f6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4689,9 +4689,8 @@ static int selinux_socket_socketpair(struct socket *socka,
    Need to determine whether we should perform a name_bind
    permission check between the socket and the port number. */
 
-static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
+static int __selinux_socket_bind(struct sock *sk, struct sockaddr *address, int addrlen)
 {
-	struct sock *sk = sock->sk;
 	struct sk_security_struct *sksec = selinux_sock(sk);
 	u16 family;
 	int err;
@@ -4825,13 +4824,17 @@ err_af:
 	return -EAFNOSUPPORT;
 }
 
+static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
+{
+	return __selinux_socket_bind(sock->sk, address, addrlen);
+}
+
 /* This supports connect(2) and SCTP connect services such as sctp_connectx(3)
  * and sctp_sendmsg(3) as described in Documentation/security/SCTP.rst
  */
-static int selinux_socket_connect_helper(struct socket *sock,
+static int selinux_socket_connect_helper(struct sock *sk,
 					 struct sockaddr *address, int addrlen)
 {
-	struct sock *sk = sock->sk;
 	struct sk_security_struct *sksec = selinux_sock(sk);
 	int err;
 
@@ -4924,7 +4927,7 @@ static int selinux_socket_connect(struct socket *sock,
 	int err;
 	struct sock *sk = sock->sk;
 
-	err = selinux_socket_connect_helper(sock, address, addrlen);
+	err = selinux_socket_connect_helper(sk, address, addrlen);
 	if (err)
 		return err;
 
@@ -5409,13 +5412,11 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
 	int len, err = 0, walk_size = 0;
 	void *addr_buf;
 	struct sockaddr *addr;
-	struct socket *sock;
 
 	if (!selinux_policycap_extsockclass())
 		return 0;
 
 	/* Process one or more addresses that may be IPv4 or IPv6 */
-	sock = sk->sk_socket;
 	addr_buf = address;
 
 	while (walk_size < addrlen) {
@@ -5444,14 +5445,14 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
 		case SCTP_PRIMARY_ADDR:
 		case SCTP_SET_PEER_PRIMARY_ADDR:
 		case SCTP_SOCKOPT_BINDX_ADD:
-			err = selinux_socket_bind(sock, addr, len);
+			err = __selinux_socket_bind(sk, addr, len);
 			break;
 		/* Connect checks */
 		case SCTP_SOCKOPT_CONNECTX:
 		case SCTP_PARAM_SET_PRIMARY:
 		case SCTP_PARAM_ADD_IP:
 		case SCTP_SENDMSG_CONNECT:
-			err = selinux_socket_connect_helper(sock, addr, len);
+			err = selinux_socket_connect_helper(sk, addr, len);
 			if (err)
 				return err;
 
-- 
2.47.3

Re: [PATCH v3] selinux: avoid sk_socket dereference in selinux_sctp_bind_connect()

[Stephen Smalley]

On Thu, Jun 25, 2026 at 7:53 PM Tristan Madani <tristmd@gmail.com> wrote:
>
> From: Tristan Madani <tristan@talencesecurity.com>
>
> selinux_sctp_bind_connect() dereferences sk->sk_socket to pass a
> struct socket * to selinux_socket_bind() and
> selinux_socket_connect_helper().  However, when the hook is invoked
> from the ASCONF softirq path (sctp_process_asconf), there is no file
> reference guaranteeing that sk->sk_socket is non-NULL.  The setsockopt
> callers (bindx, connectx, set_primary, sendmsg connect) hold a file
> reference and are not affected.
>
> Both selinux_socket_bind() and selinux_socket_connect_helper()
> immediately resolve sock->sk, never using the struct socket * for
> anything else.  Refactor the inner logic into helpers that take a
> struct sock * directly so that selinux_sctp_bind_connect() never needs
> to touch sk->sk_socket at all.
>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Tested-by: Stephen Smalley <stephen.smalley.work@gmail.com>

> ---
> Changes in v3:
>   - Keep comment describing IPv4/IPv6 address processing loop
>     (Stephen Smalley).
>
> Changes in v2:
>   - Refactor selinux_socket_bind() and selinux_socket_connect_helper()
>     into sk-based inner helpers instead of adding a NULL check on
>     sk->sk_socket (Stephen Smalley).
>
>  security/selinux/hooks.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fc926d3..1f202f6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4689,9 +4689,8 @@ static int selinux_socket_socketpair(struct socket *socka,
>     Need to determine whether we should perform a name_bind
>     permission check between the socket and the port number. */
>
> -static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
> +static int __selinux_socket_bind(struct sock *sk, struct sockaddr *address, int addrlen)
>  {
> -       struct sock *sk = sock->sk;
>         struct sk_security_struct *sksec = selinux_sock(sk);
>         u16 family;
>         int err;
> @@ -4825,13 +4824,17 @@ err_af:
>         return -EAFNOSUPPORT;
>  }
>
> +static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
> +{
> +       return __selinux_socket_bind(sock->sk, address, addrlen);
> +}
> +
>  /* This supports connect(2) and SCTP connect services such as sctp_connectx(3)
>   * and sctp_sendmsg(3) as described in Documentation/security/SCTP.rst
>   */
> -static int selinux_socket_connect_helper(struct socket *sock,
> +static int selinux_socket_connect_helper(struct sock *sk,
>                                          struct sockaddr *address, int addrlen)
>  {
> -       struct sock *sk = sock->sk;
>         struct sk_security_struct *sksec = selinux_sock(sk);
>         int err;
>
> @@ -4924,7 +4927,7 @@ static int selinux_socket_connect(struct socket *sock,
>         int err;
>         struct sock *sk = sock->sk;
>
> -       err = selinux_socket_connect_helper(sock, address, addrlen);
> +       err = selinux_socket_connect_helper(sk, address, addrlen);
>         if (err)
>                 return err;
>
> @@ -5409,13 +5412,11 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
>         int len, err = 0, walk_size = 0;
>         void *addr_buf;
>         struct sockaddr *addr;
> -       struct socket *sock;
>
>         if (!selinux_policycap_extsockclass())
>                 return 0;
>
>         /* Process one or more addresses that may be IPv4 or IPv6 */
> -       sock = sk->sk_socket;
>         addr_buf = address;
>
>         while (walk_size < addrlen) {
> @@ -5444,14 +5445,14 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
>                 case SCTP_PRIMARY_ADDR:
>                 case SCTP_SET_PEER_PRIMARY_ADDR:
>                 case SCTP_SOCKOPT_BINDX_ADD:
> -                       err = selinux_socket_bind(sock, addr, len);
> +                       err = __selinux_socket_bind(sk, addr, len);
>                         break;
>                 /* Connect checks */
>                 case SCTP_SOCKOPT_CONNECTX:
>                 case SCTP_PARAM_SET_PRIMARY:
>                 case SCTP_PARAM_ADD_IP:
>                 case SCTP_SENDMSG_CONNECT:
> -                       err = selinux_socket_connect_helper(sock, addr, len);
> +                       err = selinux_socket_connect_helper(sk, addr, len);
>                         if (err)
>                                 return err;
>
> --
> 2.47.3
>

Re: [PATCH v3] selinux: avoid sk_socket dereference in selinux_sctp_bind_connect()

[Paul Moore]

On Jun 25, 2026 Tristan Madani <tristmd@gmail.com> wrote:
> 
> selinux_sctp_bind_connect() dereferences sk->sk_socket to pass a
> struct socket * to selinux_socket_bind() and
> selinux_socket_connect_helper().  However, when the hook is invoked
> from the ASCONF softirq path (sctp_process_asconf), there is no file
> reference guaranteeing that sk->sk_socket is non-NULL.  The setsockopt
> callers (bindx, connectx, set_primary, sendmsg connect) hold a file
> reference and are not affected.
> 
> Both selinux_socket_bind() and selinux_socket_connect_helper()
> immediately resolve sock->sk, never using the struct socket * for
> anything else.  Refactor the inner logic into helpers that take a
> struct sock * directly so that selinux_sctp_bind_connect() never needs
> to touch sk->sk_socket at all.
> 
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
> Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Tested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> Changes in v3:
>   - Keep comment describing IPv4/IPv6 address processing loop
>     (Stephen Smalley).
> 
> Changes in v2:
>   - Refactor selinux_socket_bind() and selinux_socket_connect_helper()
>     into sk-based inner helpers instead of adding a NULL check on
>     sk->sk_socket (Stephen Smalley).
> 
>  security/selinux/hooks.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Thanks, this looks good to me, I'm going to merge it into
selinux/stable-7.2 now.

However, there is another issue relating to the SCTP softirq code paths:
the fact that we call into sock_has_perm() in both
__selinux_socket_bind() and selinux_socket_connect_helper().  The
sock_has_perm() function uses current_sid() as the subject in the
avc_has_perm() call, and in the softirq case that is not what we want.

It's been few years since I spent any serious time with SCTP so it isn't
immediately clear to me what the solution is to this problem, but if you
wanted to look into this and come up with some ideas that would be a big
help!

--
paul-moore.com

Re: [PATCH v3] selinux: avoid sk_socket dereference in selinux_sctp_bind_connect()

[Tristan Madani]

On Wed, 01 Jul 2026, Paul Moore wrote:
> However, there is another issue relating to the SCTP softirq code paths:
> the fact that we call into sock_has_perm() in both
> __selinux_socket_bind() and selinux_socket_connect_helper().  The
> sock_has_perm() function uses current_sid() as the subject in the
> avc_has_perm() call, and in the softirq case that is not what we want.

Had a look at this. The ASCONF softirq path is:

  sctp_rcv()  [NET_RX softirq]
    -> sctp_process_asconf()
      -> sctp_process_asconf_param()
        -> security_sctp_bind_connect(sk, SCTP_PARAM_ADD_IP/SET_PRIMARY)
          -> selinux_sctp_bind_connect()
            -> sock_has_perm()
              -> avc_has_perm(current_sid(), sksec->sid, ...)

In softirq, current is whatever process was interrupted, so the subject
SID is effectively random. Meanwhile the port/node bind checks further
down in __selinux_socket_bind() and the port connect check in
selinux_socket_connect_helper() already use sksec->sid as the subject,
which is the established pattern for softirq context
(selinux_socket_sock_rcv_skb, selinux_sctp_assoc_request, etc.).

The approach I would suggest: thread an explicit subject SID into the inner
helpers. selinux_sctp_bind_connect() would pass sksec->sid, and the
process-context wrappers (selinux_socket_bind, selinux_socket_connect)
would pass current_sid(). That keeps sock_has_perm() semantics
unchanged for the normal path and makes the SID choice visible at each
call site.

I can send a patch for this if this approach works for you.

--
Tristan