Re: [PATCH net] cipso: stop modifying live ip_options_rcu

[Casey Schaufler]

On 11/6/2026 1:40 PM, Jann Horn wrote:
> cipso_v4_delopt() operates on the ip_options_rcu of a socket or a request
> socket, and tries to remove CIPSO options from the middle by moving all
> subsequent options.
> That's not allowed - ip_options_rcu can be read locklessly and must always
> be consistent. For example, ip_options_undo() reads trusted offsets and
> length values from the options buffer.
> Fix it by doing the modification in a copy.
>
> The core change is in cipso_v4_delopt(), the rest is ugly plumbing to
> propagate memory allocation failures.

I haven't had the opportunity to test this, but the Smack changes
appear innocuous. I will try them today, weather permitting.

>
> Triggering this bug requires that the system is configured to use CIPSO.
> (Setting up CIPSO requires privileges in init_user_ns.)
>
> Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
> I've sent a separate mail with a reproducer to Paul Moore.
> ---
>  include/net/cipso_ipv4.h     | 11 ++++++-----
>  include/net/netlabel.h       | 11 ++++++-----
>  net/ipv4/cipso_ipv4.c        | 16 +++++++++++++---
>  net/netlabel/netlabel_kapi.c | 24 ++++++++++--------------
>  security/selinux/netlabel.c  |  5 +++--
>  security/smack/smack_lsm.c   | 14 +++++++++-----
>  6 files changed, 47 insertions(+), 34 deletions(-)
>
> diff --git a/include/net/cipso_ipv4.h b/include/net/cipso_ipv4.h
> index d6780d7903f4..b41672221797 100644
> --- a/include/net/cipso_ipv4.h
> +++ b/include/net/cipso_ipv4.h
> @@ -185,12 +185,12 @@ int cipso_v4_sock_setattr(struct sock *sk,
>  			  const struct cipso_v4_doi *doi_def,
>  			  const struct netlbl_lsm_secattr *secattr,
>  			  bool sk_locked);
> -void cipso_v4_sock_delattr(struct sock *sk);
> +int cipso_v4_sock_delattr(struct sock *sk);
>  int cipso_v4_sock_getattr(struct sock *sk, struct netlbl_lsm_secattr *secattr);
>  int cipso_v4_req_setattr(struct request_sock *req,
>  			 const struct cipso_v4_doi *doi_def,
>  			 const struct netlbl_lsm_secattr *secattr);
> -void cipso_v4_req_delattr(struct request_sock *req);
> +int cipso_v4_req_delattr(struct request_sock *req);
>  int cipso_v4_skbuff_setattr(struct sk_buff *skb,
>  			    const struct cipso_v4_doi *doi_def,
>  			    const struct netlbl_lsm_secattr *secattr);
> @@ -221,8 +221,9 @@ static inline int cipso_v4_sock_setattr(struct sock *sk,
>  	return -ENOSYS;
>  }
>  
> -static inline void cipso_v4_sock_delattr(struct sock *sk)
> +static inline int cipso_v4_sock_delattr(struct sock *sk)
>  {
> +	return 0;
>  }
>  
>  static inline int cipso_v4_sock_getattr(struct sock *sk,
> @@ -238,9 +239,9 @@ static inline int cipso_v4_req_setattr(struct request_sock *req,
>  	return -ENOSYS;
>  }
>  
> -static inline void cipso_v4_req_delattr(struct request_sock *req)
> +static inline int cipso_v4_req_delattr(struct request_sock *req)
>  {
> -	return;
> +	return 0;
>  }
>  
>  static inline int cipso_v4_skbuff_setattr(struct sk_buff *skb,
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 02914b1df38b..f896b9d52ed1 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -477,7 +477,7 @@ int netlbl_sock_setattr(struct sock *sk,
>  			u16 family,
>  			const struct netlbl_lsm_secattr *secattr,
>  			bool sk_locked);
> -void netlbl_sock_delattr(struct sock *sk);
> +int netlbl_sock_delattr(struct sock *sk);
>  int netlbl_sock_getattr(struct sock *sk,
>  			struct netlbl_lsm_secattr *secattr);
>  int netlbl_conn_setattr(struct sock *sk,
> @@ -485,7 +485,7 @@ int netlbl_conn_setattr(struct sock *sk,
>  			const struct netlbl_lsm_secattr *secattr);
>  int netlbl_req_setattr(struct request_sock *req,
>  		       const struct netlbl_lsm_secattr *secattr);
> -void netlbl_req_delattr(struct request_sock *req);
> +int netlbl_req_delattr(struct request_sock *req);
>  int netlbl_skbuff_setattr(struct sk_buff *skb,
>  			  u16 family,
>  			  const struct netlbl_lsm_secattr *secattr);
> @@ -626,8 +626,9 @@ static inline int netlbl_sock_setattr(struct sock *sk,
>  {
>  	return -ENOSYS;
>  }
> -static inline void netlbl_sock_delattr(struct sock *sk)
> +static inline int netlbl_sock_delattr(struct sock *sk)
>  {
> +	return 0;
>  }
>  static inline int netlbl_sock_getattr(struct sock *sk,
>  				      struct netlbl_lsm_secattr *secattr)
> @@ -645,9 +646,9 @@ static inline int netlbl_req_setattr(struct request_sock *req,
>  {
>  	return -ENOSYS;
>  }
> -static inline void netlbl_req_delattr(struct request_sock *req)
> +static inline int netlbl_req_delattr(struct request_sock *req)
>  {
> -	return;
> +	return 0;
>  }
>  static inline int netlbl_skbuff_setattr(struct sk_buff *skb,
>  				      u16 family,
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index e65a3f29d2ca..ee0c72fbc877 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -2004,11 +2004,16 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
>  	if (!opt || opt->opt.cipso == 0)
>  		return 0;
>  	if (opt->opt.srr || opt->opt.rr || opt->opt.ts || opt->opt.router_alert) {
> +		struct ip_options_rcu *old_opt = opt;
>  		u8 cipso_len;
>  		u8 cipso_off;
>  		unsigned char *cipso_ptr;
>  		int optlen_new;
>  
> +		/* ip_options_rcu must not be modified in place, use a copy */
> +		opt = kmemdup(opt, sizeof(*opt) + opt->opt.optlen, GFP_ATOMIC);
> +		if (!opt)
> +			return -ENOMEM;
>  		cipso_off = opt->opt.cipso - sizeof(struct iphdr);
>  		cipso_ptr = &opt->opt.__data[cipso_off];
>  		cipso_len = cipso_ptr[1];
> @@ -2036,6 +2041,8 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
>  
>  		memset(&opt->opt.__data[optlen_new], IPOPT_END,
>  		       opt->opt.optlen - optlen_new);
> +		rcu_assign_pointer(*opt_ptr, opt);
> +		kfree_rcu(old_opt, rcu);
>  	} else {
>  		/* only the cipso option was present on the socket so we can
>  		 * remove the entire option struct */
> @@ -2055,7 +2062,7 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
>   * Removes the CIPSO option from a socket, if present.
>   *
>   */
> -void cipso_v4_sock_delattr(struct sock *sk)
> +int cipso_v4_sock_delattr(struct sock *sk)
>  {
>  	struct inet_sock *sk_inet;
>  	int hdr_delta;
> @@ -2068,6 +2075,7 @@ void cipso_v4_sock_delattr(struct sock *sk)
>  		sk_conn->icsk_ext_hdr_len -= hdr_delta;
>  		sk_conn->icsk_sync_mss(sk, sk_conn->icsk_pmtu_cookie);
>  	}
> +	return (hdr_delta < 0) ? hdr_delta : 0;
>  }
>  
>  /**
> @@ -2078,9 +2086,11 @@ void cipso_v4_sock_delattr(struct sock *sk)
>   * Removes the CIPSO option from a request socket, if present.
>   *
>   */
> -void cipso_v4_req_delattr(struct request_sock *req)
> +int cipso_v4_req_delattr(struct request_sock *req)
>  {
> -	cipso_v4_delopt(&inet_rsk(req)->ireq_opt);
> +	int hdr_delta = cipso_v4_delopt(&inet_rsk(req)->ireq_opt);
> +
> +	return (hdr_delta < 0) ? hdr_delta : 0;
>  }
>  
>  /**
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 3583fa63dd01..c0eddb6afef3 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -1045,18 +1045,18 @@ int netlbl_sock_setattr(struct sock *sk,
>   * ensuring that @sk is locked.
>   *
>   */
> -void netlbl_sock_delattr(struct sock *sk)
> +int netlbl_sock_delattr(struct sock *sk)
>  {
>  	switch (sk->sk_family) {
>  	case AF_INET:
> -		cipso_v4_sock_delattr(sk);
> -		break;
> +		return cipso_v4_sock_delattr(sk);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	case AF_INET6:
>  		calipso_sock_delattr(sk);
>  		break;
>  #endif /* IPv6 */
>  	}
> +	return 0;
>  }
>  
>  /**
> @@ -1156,8 +1156,7 @@ int netlbl_conn_setattr(struct sock *sk,
>  		case NETLBL_NLTYPE_UNLABELED:
>  			/* just delete the protocols we support for right now
>  			 * but we could remove other protocols if needed */
> -			netlbl_sock_delattr(sk);
> -			ret_val = 0;
> +			ret_val = netlbl_sock_delattr(sk);
>  			break;
>  		default:
>  			ret_val = -ENOENT;
> @@ -1185,8 +1184,7 @@ int netlbl_conn_setattr(struct sock *sk,
>  		case NETLBL_NLTYPE_UNLABELED:
>  			/* just delete the protocols we support for right now
>  			 * but we could remove other protocols if needed */
> -			netlbl_sock_delattr(sk);
> -			ret_val = 0;
> +			ret_val = netlbl_sock_delattr(sk);
>  			break;
>  		default:
>  			ret_val = -ENOENT;
> @@ -1234,8 +1232,7 @@ int netlbl_req_setattr(struct request_sock *req,
>  						       entry->cipso, secattr);
>  			break;
>  		case NETLBL_NLTYPE_UNLABELED:
> -			netlbl_req_delattr(req);
> -			ret_val = 0;
> +			ret_val = netlbl_req_delattr(req);
>  			break;
>  		default:
>  			ret_val = -ENOENT;
> @@ -1255,8 +1252,7 @@ int netlbl_req_setattr(struct request_sock *req,
>  						      entry->calipso, secattr);
>  			break;
>  		case NETLBL_NLTYPE_UNLABELED:
> -			netlbl_req_delattr(req);
> -			ret_val = 0;
> +			ret_val = netlbl_req_delattr(req);
>  			break;
>  		default:
>  			ret_val = -ENOENT;
> @@ -1280,18 +1276,18 @@ int netlbl_req_setattr(struct request_sock *req,
>  * Remove all the NetLabel labeling from @req.
>  *
>  */
> -void netlbl_req_delattr(struct request_sock *req)
> +int netlbl_req_delattr(struct request_sock *req)
>  {
>  	switch (req->rsk_ops->family) {
>  	case AF_INET:
> -		cipso_v4_req_delattr(req);
> -		break;
> +		return cipso_v4_req_delattr(req);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	case AF_INET6:
>  		calipso_req_delattr(req);
>  		break;
>  #endif /* IPv6 */
>  	}
> +	return 0;
>  }
>  
>  /**
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index d51dfe892312..24b11d1891d8 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -557,9 +557,10 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
>  	 * is set to AF_UNSPEC, if that is what is happening we want to reset
>  	 * the socket */
>  	if (addr->sa_family == AF_UNSPEC) {
> -		netlbl_sock_delattr(sk);
> +		rc = netlbl_sock_delattr(sk);
> +		if (rc)
> +			return rc;
>  		sksec->nlbl_state = NLBL_REQSKB;
> -		rc = 0;
>  		return rc;
>  	}
>  	secattr = selinux_netlbl_sock_genattr(sk);
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 28a8e12603ce..3a4a33568297 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2656,22 +2656,26 @@ static int smack_netlbl_add(struct sock *sk)
>   *
>   * Remove the outbound smack value from a socket
>   */
> -static void smack_netlbl_delete(struct sock *sk)
> +static int smack_netlbl_delete(struct sock *sk)
>  {
>  	struct socket_smack *ssp = smack_sock(sk);
> +	int rc;
>  
>  	/*
>  	 * Take the label off the socket if one is set.
>  	 */
>  	if (ssp->smk_state != SMK_NETLBL_LABELED)
> -		return;
> +		return 0;
>  
>  	local_bh_disable();
>  	bh_lock_sock_nested(sk);
> -	netlbl_sock_delattr(sk);
> +	rc = netlbl_sock_delattr(sk);
>  	bh_unlock_sock(sk);
>  	local_bh_enable();
> +	if (rc)
> +		return rc;
>  	ssp->smk_state = SMK_NETLBL_UNLABELED;
> +	return 0;
>  }
>  
>  /**
> @@ -2711,7 +2715,7 @@ static int smk_ipv4_check(struct sock *sk, struct sockaddr_in *sap)
>  		 * Clear the socket netlabel if it's set.
>  		 */
>  		if (!rc)
> -			smack_netlbl_delete(sk);
> +			rc = smack_netlbl_delete(sk);
>  	}
>  	rcu_read_unlock();
>  
> @@ -4548,7 +4552,7 @@ static int smack_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
>  	if (hskp == NULL)
>  		rc = netlbl_req_setattr(req, &ssp->smk_out->smk_netlabel);
>  	else
> -		netlbl_req_delattr(req);
> +		rc = netlbl_req_delattr(req);
>  
>  	return rc;
>  }
>
> ---
> base-commit: a91337fb5e8e0127c8b5be87be9f3027f95b48bc
> change-id: 20261106-cipso-rcuopts-e26ffa45b392
>
> Best regards,
> --  
> Jann Horn <jannh@xxxxxxxxxx>
>