Netdev List
 help / color / mirror / Atom feed
* [PATCH net v2] cipso_v4: fail the walk on malformed options
@ 2026-10-27 11:16 omniameasunt
  2026-10-29  1:11 ` Paul Moore
  2026-10-31  2:03 ` Jakub Kicinski
  0 siblings, 2 replies; 4+ messages in thread
From: omniameasunt @ 2026-10-27 11:16 UTC (permalink / raw)
  To: netdev@vger.kernel.org; +Cc: Paul Moore

cipso_v4_get_actual_opt_len() walks a compacted IP option buffer with no
validation of the option it is standing on: it reads data[iter + 1]
without checking that the length byte is inside the buffer, it accepts a
zero length byte (spinning forever), and it never clamps the walked
offset to len, so the returned length can exceed the 40-byte MAX_IPOPTLEN
bound that every caller assumes.

After cipso_v4_delopt() memmove-compacts a socket's options, the bytes
past the compacted region are stale and attacker-controlled via
setsockopt(IP_OPTIONS).  An unprivileged user (CAP_NET_RAW in a user
namespace, CIPSO labeling configured) can trigger the walk with
connect(AF_UNSPEC), which reaches cipso_v4_sock_delattr() through the
SELinux NetLabel hook.  The poisoned optlen is later used in
do_ip_getsockopt(), where it causes

  * an out-of-bounds read of the adjacent heap object copied back to
    userspace (kernel pointer disclosure, KASLR defeat), and
  * a stack buffer overflow when optlen exceeds the 60-byte scratch
    buffer (return-address overwrite; exploitable for privilege
    escalation on kernels built without stack protector).

The same helper is also called from cipso_v4_skbuff_delattr() when a
label is removed from output or forwarded packets; the option bytes there
have already been validated by ip_options_compile(), so the new checks
are defensive for that caller.

Fix the defects by making the walk fail with -EINVAL as soon as an
option header is truncated or malformed (length byte missing, zero, or
one) or an option would run past len.  cipso_v4_delopt() then drops the
socket's entire option set (reusing the existing branch that frees the
ip_options_rcu when CIPSO was the only option), and
cipso_v4_skbuff_delattr() leaves a malformed packet unchanged instead
of rewriting its header based on untrustworthy bytes.

Well-formed option streams are unaffected: a socket whose options
survive the compaction intact keeps them; only malformed streams now
lose all options instead of carrying a partially parsed set.

The function also compacted the option object in place while the
ip_options_rcu remained published.  Lockless readers such as
getsockopt(IP_OPTIONS), UDP sendmsg(), raw sendmsg(), and ping sendmsg()
copy the object under rcu_read_lock(), so they can observe a mixture of
old and new offsets or option bytes.  Build the compacted object in a
replacement ip_options_rcu, modify only the replacement, publish it with
rcu_assign_pointer(), and retire the old object with kfree_rcu().  If the
replacement cannot be allocated, leave the old object untouched and
return -ENOMEM.

Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint")
Signed-off-by: Lucas Dupré--Rabinovici <omniameasunt@proton.me>
---
Previous version:
https://lore.kernel.org/netdev/CBLsHx48w8Bn1k_Xg6kJFliLNkxLCqi8SnkifapxYYpVtll4w-W_2JJePtbk9ssIS701IFJvi0Q2n5l-2GFFe8hNHU9Y_CgD_sdzcAbILVA=@proton.me/

--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1824,7 +1824,11 @@
 		} else if (data[iter] == IPOPT_NOP) {
 			iter++;
 		} else {
+			if (iter + 1 >= len || data[iter + 1] < 2)
+				return -EINVAL;
 			iter += data[iter + 1];
+			if (iter > len)
+				return -EINVAL;
 			optlen = iter;
 		}
 	}
@@ -1992,53 +1996,79 @@
  *
  * Description:
  * Deletes the CIPSO IP option from a set of IP options and makes the necessary
- * adjustments to the IP option structure.  Returns zero on success, negative
- * values on failure.
+ * adjustments to the IP option structure.  Returns zero or a positive header
+ * length delta on success, negative values on failure.
  *
  */
 static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
 {
 	struct ip_options_rcu *opt = rcu_dereference_protected(*opt_ptr, 1);
+	struct ip_options_rcu *new;
 	int hdr_delta = 0;
+	u8 cipso_len, cipso_off;
+	unsigned char *cipso_ptr;
+	int optlen, optlen_new;
 
 	if (!opt || opt->opt.cipso == 0)
 		return 0;
-	if (opt->opt.srr || opt->opt.rr || opt->opt.ts || opt->opt.router_alert) {
-		u8 cipso_len;
-		u8 cipso_off;
-		unsigned char *cipso_ptr;
-		int optlen_new;
-
-		cipso_off = opt->opt.cipso - sizeof(struct iphdr);
-		cipso_ptr = &opt->opt.__data[cipso_off];
-		cipso_len = cipso_ptr[1];
-
-		if (opt->opt.srr > opt->opt.cipso)
-			opt->opt.srr -= cipso_len;
-		if (opt->opt.rr > opt->opt.cipso)
-			opt->opt.rr -= cipso_len;
-		if (opt->opt.ts > opt->opt.cipso)
-			opt->opt.ts -= cipso_len;
-		if (opt->opt.router_alert > opt->opt.cipso)
-			opt->opt.router_alert -= cipso_len;
-		opt->opt.cipso = 0;
 
-		memmove(cipso_ptr, cipso_ptr + cipso_len,
-			opt->opt.optlen - cipso_off - cipso_len);
-
-		optlen_new = cipso_v4_get_actual_opt_len(opt->opt.__data,
-							 opt->opt.optlen);
-		hdr_delta = opt->opt.optlen;
-		opt->opt.optlen = (optlen_new + 3) & ~3;
-		hdr_delta -= opt->opt.optlen;
-	} else {
+	if (!opt->opt.srr && !opt->opt.rr && !opt->opt.ts &&
+	    !opt->opt.router_alert) {
 		/* only the cipso option was present on the socket so we can
 		 * remove the entire option struct */
-		*opt_ptr = NULL;
+		rcu_assign_pointer(*opt_ptr, NULL);
 		hdr_delta = opt->opt.optlen;
 		kfree_rcu(opt, rcu);
+		return hdr_delta;
 	}
 
+	new = kzalloc(sizeof(*new) + opt->opt.optlen, GFP_ATOMIC);
+	if (!new)
+		return -ENOMEM;
+	memcpy(&new->opt, &opt->opt, sizeof(new->opt) + opt->opt.optlen);
+
+	cipso_off = new->opt.cipso - sizeof(struct iphdr);
+	if (cipso_off + 1 >= new->opt.optlen)
+		goto drop_all;
+	cipso_ptr = &new->opt.__data[cipso_off];
+	cipso_len = cipso_ptr[1];
+	if (cipso_len < 2 || cipso_len > new->opt.optlen - cipso_off)
+		goto drop_all;
+
+	if (new->opt.srr > new->opt.cipso)
+		new->opt.srr -= cipso_len;
+	if (new->opt.rr > new->opt.cipso)
+		new->opt.rr -= cipso_len;
+	if (new->opt.ts > new->opt.cipso)
+		new->opt.ts -= cipso_len;
+	if (new->opt.router_alert > new->opt.cipso)
+		new->opt.router_alert -= cipso_len;
+	new->opt.cipso = 0;
+
+	memmove(cipso_ptr, cipso_ptr + cipso_len,
+		new->opt.optlen - cipso_off - cipso_len);
+
+	/* Only the compacted region is live.  The old tail is stale and
+	 * must not take part in the walk or remain visible as IP options.
+	 */
+	optlen = new->opt.optlen - cipso_len;
+	memset(new->opt.__data + optlen, IPOPT_END, cipso_len);
+	optlen_new = cipso_v4_get_actual_opt_len(new->opt.__data, optlen);
+	if (optlen_new < 0)
+		goto drop_all;
+
+	hdr_delta = opt->opt.optlen;
+	new->opt.optlen = (optlen_new + 3) & ~3;
+	hdr_delta -= new->opt.optlen;
+	rcu_assign_pointer(*opt_ptr, new);
+	kfree_rcu(opt, rcu);
+	return hdr_delta;
+
+drop_all:
+	kfree(new);
+	rcu_assign_pointer(*opt_ptr, NULL);
+	hdr_delta = opt->opt.optlen;
+	kfree_rcu(opt, rcu);
 	return hdr_delta;
 }
 
@@ -2248,14 +2278,18 @@
  */
 int cipso_v4_skbuff_delattr(struct sk_buff *skb)
 {
-	int ret_val, cipso_len, hdr_len_actual, new_hdr_len_actual, new_hdr_len,
-	    hdr_len_delta;
+	int ret_val, cipso_len, cipso_off, hdr_len_actual, new_hdr_len_actual;
+	int new_hdr_len, hdr_len_delta, optlen;
 	struct iphdr *iph;
 	struct ip_options *opt = &IPCB(skb)->opt;
 	unsigned char *cipso_ptr;
 
 	if (opt->cipso == 0)
 		return 0;
+	if (opt->cipso < sizeof(struct iphdr) ||
+	    opt->cipso - sizeof(struct iphdr) + 1 >= opt->optlen)
+		return -EINVAL;
+	cipso_off = opt->cipso - sizeof(struct iphdr);
 
 	/* since we are changing the packet we should make a copy */
 	ret_val = skb_cow(skb, skb_headroom(skb));
@@ -2263,12 +2297,19 @@
 		return ret_val;
 
 	iph = ip_hdr(skb);
+	if (opt->optlen > (iph->ihl << 2) - sizeof(struct iphdr))
+		return -EINVAL;
 	cipso_ptr = (unsigned char *)iph + opt->cipso;
 	cipso_len = cipso_ptr[1];
+	if (cipso_len < 2 || cipso_len > opt->optlen - cipso_off)
+		return -EINVAL;
 
-	hdr_len_actual = sizeof(struct iphdr) +
-			 cipso_v4_get_actual_opt_len((unsigned char *)(iph + 1),
-						     opt->optlen);
+	optlen = cipso_v4_get_actual_opt_len((unsigned char *)(iph + 1),
+					     opt->optlen);
+	/* malformed option stream, leave the packet unchanged */
+	if (optlen < 0 || cipso_off + cipso_len > optlen)
+		return -EINVAL;
+	hdr_len_actual = sizeof(struct iphdr) + optlen;
 	new_hdr_len_actual = hdr_len_actual - cipso_len;
 	new_hdr_len = (new_hdr_len_actual + 3) & ~3;
 	hdr_len_delta = (iph->ihl << 2) - new_hdr_len;


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2] cipso_v4: fail the walk on malformed options
@ 2026-10-27 11:21 netdev-bot+sinfo
  0 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sinfo @ 2026-10-27 11:21 UTC (permalink / raw)
  To: omniameasunt; +Cc: netdev@vger.kernel.org, Paul Moore

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2] cipso_v4: fail the walk on malformed options
  2026-10-27 11:16 [PATCH net v2] cipso_v4: fail the walk on malformed options omniameasunt
@ 2026-10-29  1:11 ` Paul Moore
  2026-10-31  2:03 ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: Paul Moore @ 2026-10-29  1:11 UTC (permalink / raw)
  To: omniameasunt; +Cc: netdev@vger.kernel.org, Qihang

On Tue, Oct 27, 2026 at 7:16 AM <omniameasunt@proton.me> wrote:
>
> cipso_v4_get_actual_opt_len() walks a compacted IP option buffer with no
> validation of the option it is standing on: it reads data[iter + 1]
> without checking that the length byte is inside the buffer, it accepts a
> zero length byte (spinning forever), and it never clamps the walked
> offset to len, so the returned length can exceed the 40-byte MAX_IPOPTLEN
> bound that every caller assumes.
>
> After cipso_v4_delopt() memmove-compacts a socket's options, the bytes
> past the compacted region are stale and attacker-controlled via
> setsockopt(IP_OPTIONS).  An unprivileged user (CAP_NET_RAW in a user
> namespace, CIPSO labeling configured) can trigger the walk with
> connect(AF_UNSPEC), which reaches cipso_v4_sock_delattr() through the
> SELinux NetLabel hook.  The poisoned optlen is later used in
> do_ip_getsockopt(), where it causes
>
>   * an out-of-bounds read of the adjacent heap object copied back to
>     userspace (kernel pointer disclosure, KASLR defeat), and
>   * a stack buffer overflow when optlen exceeds the 60-byte scratch
>     buffer (return-address overwrite; exploitable for privilege
>     escalation on kernels built without stack protector).
>
> The same helper is also called from cipso_v4_skbuff_delattr() when a
> label is removed from output or forwarded packets; the option bytes there
> have already been validated by ip_options_compile(), so the new checks
> are defensive for that caller.
>
> Fix the defects by making the walk fail with -EINVAL as soon as an
> option header is truncated or malformed (length byte missing, zero, or
> one) or an option would run past len.  cipso_v4_delopt() then drops the
> socket's entire option set (reusing the existing branch that frees the
> ip_options_rcu when CIPSO was the only option), and
> cipso_v4_skbuff_delattr() leaves a malformed packet unchanged instead
> of rewriting its header based on untrustworthy bytes.
>
> Well-formed option streams are unaffected: a socket whose options
> survive the compaction intact keeps them; only malformed streams now
> lose all options instead of carrying a partially parsed set.
>
> The function also compacted the option object in place while the
> ip_options_rcu remained published.  Lockless readers such as
> getsockopt(IP_OPTIONS), UDP sendmsg(), raw sendmsg(), and ping sendmsg()
> copy the object under rcu_read_lock(), so they can observe a mixture of
> old and new offsets or option bytes.  Build the compacted object in a
> replacement ip_options_rcu, modify only the replacement, publish it with
> rcu_assign_pointer(), and retire the old object with kfree_rcu().  If the
> replacement cannot be allocated, leave the old object untouched and
> return -ENOMEM.
>
> Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint")
> Signed-off-by: Lucas Dupré--Rabinovici <omniameasunt@proton.me>
> ---
> Previous version:
> https://lore.kernel.org/netdev/CBLsHx48w8Bn1k_Xg6kJFliLNkxLCqi8SnkifapxYYpVtll4w-W_2JJePtbk9ssIS701IFJvi0Q2n5l-2GFFe8hNHU9Y_CgD_sdzcAbILVA=@proton.me/

Hi Lucas,

Thanks again for your help with this.

Qihang (added to the CC line) independently reported, and has been
working on a fix for some of the issues Sashiko identified in your
first patchset.  I expect that Qihang will be posting their patch
publicly very soon, perhaps you can take a look once it is posted?  I
suspect that basing your patch on top of Qihang's patch may simplify
some things.

-- 
paul-moore.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net v2] cipso_v4: fail the walk on malformed options
  2026-10-27 11:16 [PATCH net v2] cipso_v4: fail the walk on malformed options omniameasunt
  2026-10-29  1:11 ` Paul Moore
@ 2026-10-31  2:03 ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2026-10-31  2:03 UTC (permalink / raw)
  To: omniameasunt; +Cc: Jakub Kicinski, netdev, paul

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
cipso_v4: fail the walk on malformed options

This patch makes cipso_v4_get_actual_opt_len() return -EINVAL when an
option is truncated or malformed. It also makes cipso_v4_delopt() and
cipso_v4_skbuff_delattr() bail out on malformed streams. In addition,
cipso_v4_delopt() now builds the compacted options in a new
ip_options_rcu, publishes it with rcu_assign_pointer() and retires the
old object with kfree_rcu().

>   * a stack buffer overflow when optlen exceeds the 60-byte scratch
>     buffer (return-address overwrite; exploitable for privilege
>     escalation on kernels built without stack protector).

[Severity: Low]

Is the buffer size right here? In do_ip_getsockopt() the buffer is:

	unsigned char optbuf[sizeof(struct ip_options)+40];

struct ip_options is 16 bytes: two __be32 fields plus eight one-byte
fields, and the flexible __data[] adds nothing. That makes optbuf 56
bytes.

The copy into it is:

	memcpy(optbuf, &inet_opt->opt,
	       sizeof(struct ip_options) +
	       inet_opt->opt.optlen);

This goes through the fortified memcpy(), and the destination size is
known at compile time. Wouldn't CONFIG_FORTIFY_SOURCE also trap the
oversized write? The stated exploitability condition seems incomplete
without that.

> cipso_v4_delopt() then drops the
> socket's entire option set (reusing the existing branch that frees the
> ip_options_rcu when CIPSO was the only option)

The patch doesn't reuse that branch. It adds a new drop_all label that
copies the same logic. Could the wording be adjusted?

> Well-formed option streams are unaffected: a socket whose options
> survive the compaction intact keeps them; only malformed streams now
> lose all options instead of carrying a partially parsed set.

[Severity: Medium]

Is this accurate? cipso_v4_delopt() now walks only the compacted region
and fills the stale tail with IPOPT_END:

	optlen = new->opt.optlen - cipso_len;
	memset(new->opt.__data + optlen, IPOPT_END, cipso_len);
	optlen_new = cipso_v4_get_actual_opt_len(new->opt.__data, optlen);

Before the patch the walk covered the old optlen. If no IPOPT_END was
left in the compacted region, the walk ran into the stale tail and
counted those bytes a second time.

Well-formed options can hit this. ip_options_echo() writes rr/ts/srr
first and CIPSO last, and pads only up to a 4-byte boundary. Suppose
CIPSO ends exactly on that boundary:

  - memmove() copies 0 bytes
  - the CIPSO bytes get walked again
  - optlen_new equals the old optlen and hdr_delta is 0
  - opt.cipso is cleared, but the CIPSO bytes stay in the options and
    go out on the wire

Socket options such as CIPSO(8)+RR(8) with no padding behave the same
way: RR ends up duplicated and optlen does not shrink.

With this patch CIPSO really is removed in those cases. That looks like
a second bug being fixed. Could the commit message mention it?

The message also leaves out the request socket path. There,
cipso_v4_delopt() runs on options echoed from a remote SYN when the peer
maps to an unlabeled domain:

netlbl_req_setattr()
  netlbl_req_delattr()
    cipso_v4_req_delattr()
      cipso_v4_delopt()

Would it be worth describing that caller too?

> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 1aacbbeffbc64..e618cbbd56c6a 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c

[ ... ]

> @@ -1992,53 +1996,79 @@ int cipso_v4_req_setattr(struct request_sock *req,
>   *
>   * Description:
>   * Deletes the CIPSO IP option from a set of IP options and makes the necessary
> - * adjustments to the IP option structure.  Returns zero on success, negative
> - * values on failure.
> + * adjustments to the IP option structure.  Returns zero or a positive header
> + * length delta on success, negative values on failure.
>   *
>   */
>  static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr)
>  {
>  	struct ip_options_rcu *opt = rcu_dereference_protected(*opt_ptr, 1);
> +	struct ip_options_rcu *new;

[ ... ]

> +	new = kzalloc(sizeof(*new) + opt->opt.optlen, GFP_ATOMIC);
> +	if (!new)
> +		return -ENOMEM;

[Severity: Medium]

Before this patch cipso_v4_delopt() could not fail. Can the callers
handle the new -ENOMEM return? On this path *opt_ptr is not touched, so
the ip_options_rcu that still carries CIPSO stays published.

None of the callers check for a negative value:

net/ipv4/cipso_ipv4.c:cipso_v4_sock_delattr() {
    ...
	hdr_delta = cipso_v4_delopt(&sk_inet->inet_opt);
	if (inet_test_bit(IS_ICSK, sk) && hdr_delta > 0) {
    ...
}

net/ipv4/cipso_ipv4.c:cipso_v4_req_delattr() {
	cipso_v4_delopt(&inet_rsk(req)->ireq_opt);
}

The layers above don't see the failure either:

  - netlbl_sock_delattr() and netlbl_req_delattr() return void.
  - netlbl_conn_setattr() and netlbl_req_setattr() set ret_val = 0 in
    the NETLBL_NLTYPE_UNLABELED case.
  - selinux_netlbl_socket_connect_helper() and smack_netlbl_delete()
    (which sets SMK_NETLBL_UNLABELED) then update their state as if
    the label had been removed.

After a GFP_ATOMIC failure, wouldn't packets to an unlabeled destination
still carry the CIPSO option? The same question applies to the SYN-ACK
and the child socket on the request path.

The updated kernel-doc says negative values mean failure, but no caller
acts on that. The drop_all fallback below needs no allocation. Would
using it here keep the CIPSO option always removed, as before?

> +	memcpy(&new->opt, &opt->opt, sizeof(new->opt) + opt->opt.optlen);

[ ... ]

> +	hdr_delta = opt->opt.optlen;
> +	new->opt.optlen = (optlen_new + 3) & ~3;
> +	hdr_delta -= new->opt.optlen;
> +	rcu_assign_pointer(*opt_ptr, new);
> +	kfree_rcu(opt, rcu);

[Severity: High]

In the multi-option branch, the old ip_options_rcu used to be modified
in place and was never freed. It is now replaced and passed to
kfree_rcu(). Is every writer of inet->inet_opt serialized against this?

The Smack caller doesn't seem to be. smk_ipv4_check() is called from
smack_socket_connect() and smack_socket_sendmsg() without lock_sock(),
and it gets here through:

smack_netlbl_delete()
  netlbl_sock_delattr()
    cipso_v4_sock_delattr()
      cipso_v4_delopt()

Only the socket spinlock is held:

security/smack/smack_lsm.c:smack_netlbl_delete() {
    ...
	local_bh_disable();
	bh_lock_sock_nested(sk);
	netlbl_sock_delattr(sk);
	bh_unlock_sock(sk);
	local_bh_enable();
	ssp->smk_state = SMK_NETLBL_UNLABELED;
}

bh_lock_sock_nested() takes only sk_lock.slock.
do_ip_setsockopt(IP_OPTIONS) holds socket ownership through
sockopt_lock_sock(), which has already released slock. So the two paths
don't exclude each other:

CPU1: smack_netlbl_delete()->...->cipso_v4_delopt()
    opt = rcu_dereference_protected(*opt_ptr, 1);

CPU2: do_ip_setsockopt(IP_OPTIONS)
    old = rcu_dereference_protected(inet->inet_opt, ...);

CPU1:
    rcu_assign_pointer(*opt_ptr, new);
    kfree_rcu(opt, rcu);

CPU2:
    rcu_assign_pointer(inet->inet_opt, opt);
    if (old)
        kfree_rcu(old, rcu);

Here opt and old point to the same object. Could this queue call_rcu()
twice on the same rcu_head and free that object twice? It would also
leak one of the two replacement objects.

Because cipso_v4_delopt() uses rcu_dereference_protected(*opt_ptr, 1),
lockdep can't flag the missing exclusion either.

The Smack locking predates this patch, and so does the same free pattern
in the only-CIPSO branch. This patch extends the free to every option
set that includes CIPSO. One example is CIPSO+RR set through
setsockopt(IP_OPTIONS) with CAP_NET_RAW in a user namespace.

> +	return hdr_delta;
> +
> +drop_all:
> +	kfree(new);
> +	rcu_assign_pointer(*opt_ptr, NULL);
> +	hdr_delta = opt->opt.optlen;
> +	kfree_rcu(opt, rcu);
>  	return hdr_delta;
>  }

[Severity: Low]

The kernel-doc for cipso_v4_delopt() still says it "Deletes the CIPSO IP
option from a set of IP options". The kernel-doc for
cipso_v4_sock_delattr() says it "Removes the CIPSO option from a socket".

The drop_all path frees the whole option set, srr/rr/ts/router_alert
included, and returns a positive hdr_delta. The updated Returns sentence
reserves negative values for failure. Yet a malformed stream is reported
as success, and only an allocation failure returns a negative value.

Should the kernel-doc describe this behaviour? drop_all looks
unreachable for options that went through ip_options_compile() or
ip_options_echo(), so this only affects the documentation.

[ ... ]

> @@ -2263,12 +2297,19 @@ int cipso_v4_skbuff_delattr(struct sk_buff *skb)
>  		return ret_val;
>  
>  	iph = ip_hdr(skb);
> +	if (opt->optlen > (iph->ihl << 2) - sizeof(struct iphdr))
> +		return -EINVAL;

[Severity: Low]

sizeof(struct iphdr) is a size_t, so this comparison is unsigned. When
iph->ihl < 5, (iph->ihl << 2) - sizeof(struct iphdr) wraps to a huge
value and the check always passes. Is that intended?

The code further down relies on this guard to keep hdr_len_delta
non-negative:

	hdr_len_delta = (iph->ihl << 2) - new_hdr_len;
	...
	memmove((unsigned char *)iph + hdr_len_delta, iph, new_hdr_len_actual);
	...
	skb_pull(skb, hdr_len_delta);

With a negative delta, memmove() would write below the network header
and skb_pull() would be passed a huge length.

ip_rcv_core() drops packets with ihl < 5, and the local output paths
always build ihl >= 5. Reaching this would need something like
privileged netfilter payload mangling. Would a signed comparison, or an
explicit iph->ihl < 5 check, make the new guard cover the case it was
added for?

[ ... ]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-10-31  2:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-27 11:16 [PATCH net v2] cipso_v4: fail the walk on malformed options omniameasunt
2026-10-29  1:11 ` Paul Moore
2026-10-31  2:03 ` Jakub Kicinski
  -- strict thread matches above, loose matches on Subject: below --
2026-10-27 11:21 netdev-bot+sinfo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox