From: Casey Schaufler <casey@schaufler-ca.com>
To: Jann Horn <jannh@google.com>, Paul Moore <paul@paul-moore.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@kernel.org>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: "James Morris" <jmorris@namei.org>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Simon Horman" <horms@kernel.org>,
"Stephen Smalley" <stephen.smalley.work@gmail.com>,
"Ondrej Mosnáček" <omosnacek@gmail.com>,
netdev@vger.kernel.org, linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, selinux@vger.kernel.org,
stable@vger.kernel.org,
"Casey Schaufler" <casey@schaufler-ca.com>
Subject: Re: [PATCH net] cipso: stop modifying live ip_options_rcu
Date: Tue, 10 Nov 2026 08:10:05 -0800 [thread overview]
Message-ID: <3eac0567-4f7e-4d55-a051-44aabf1a4690@schaufler-ca.com> (raw)
In-Reply-To: <20261106-cipso-rcuopts-v1-1-31ae931d7a0f@google.com>
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@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> 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@google.com>
>
prev parent reply other threads:[~2026-11-10 16:10 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-06 21:40 Jann Horn
2026-11-06 23:06 ` Eric Dumazet
2026-11-09 20:14 ` Paul Moore
2026-11-10 16:41 ` Casey Schaufler
2026-11-09 20:13 ` Paul Moore
2026-11-10 16:13 ` Casey Schaufler
2026-11-10 16:10 ` Casey Schaufler [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3eac0567-4f7e-4d55-a051-44aabf1a4690@schaufler-ca.com \
--to=casey@schaufler-ca.com \
--cc=davem@davemloft.net \
--cc=edumazet@kernel.org \
--cc=horms@kernel.org \
--cc=jannh@google.com \
--cc=jmorris@namei.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=omosnacek@gmail.com \
--cc=pabeni@redhat.com \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=stable@vger.kernel.org \
--cc=stephen.smalley.work@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®