Re: [PATCH net] cipso: adjust cached option offsets when removing CIPSO

[Ondrej Mosnáček]

On Wed, 30 Sept 2026 at 16:04, Daehyeon Ko <4ncienth@xxxxxxxxx> wrote:
>
> cipso_v4_skbuff_delattr() shifts every IPv4 option after CIPSO left by
> cipso_len, but leaves the cached srr, rr, ts and router_alert offsets at
> their old locations.
>
> With a valid eight-byte CIPSO option followed by a seven-byte RR option,
> RR moves from offset 28 to 20 while opt->rr remains 28.  The stale offset
> then points into packet payload, letting the sender control both the
> length and bytes copied by __ip_options_echo().  Setting the length to
> 255 produced this condensed KASAN result:
>
>   BUG: KASAN: stack-out-of-bounds in __ip_options_echo
>   Write of size 255
>   Call Trace:
>    __ip_options_echo
>    run_cipso_stale_offset [net_candidate_probe]
>
> The destination holds 40 bytes of option data.  In an existing
> uninstrumented x86-64 v7.2 build, __icmp_send() places that buffer
> immediately before its stack canary, and the 255-byte copy also crosses
> the saved return address.  A canary mismatch unconditionally panics;
> no canary disclosure or control-flow exploit was demonstrated.
>
> Mirror cipso_v4_delopt() and subtract cipso_len from each cached offset
> that follows CIPSO.  cipso_len is the logical distance moved by the first
> memmove(); hdr_len_delta includes padding, while the later header move and
> network-header reset relocate the bytes and their base together.
>
> The adjusted input leaves rr at 20 with length 7 and completes without a
> sanitizer report.  Natural remote reachability requires a configured
> SELinux/NetLabel gateway and a later option consumer; the retained test
> invokes the real mutation and echo consumer directly because the available
> rootfs has no SELinux policy or netlabelctl.
>
> Fixes: 89aa3619d141 ("cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Daehyeon Ko <4ncienth@xxxxxxxxx>
> ---
>  net/ipv4/cipso_ipv4.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index a05aa075de1a5..135b57c2c4f9e 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -2272,6 +2272,14 @@ int cipso_v4_skbuff_delattr(struct sk_buff *skb)
>         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;
> +       if (opt->srr > opt->cipso)
> +               opt->srr -= cipso_len;
> +       if (opt->rr > opt->cipso)
> +               opt->rr -= cipso_len;
> +       if (opt->ts > opt->cipso)
> +               opt->ts -= cipso_len;
> +       if (opt->router_alert > opt->cipso)
> +               opt->router_alert -= cipso_len;

Minor nit: IMHO, it would be better to move the added bits to the
block by the end of the function where the other opt->... metadata is
updated.

One way or the other:
Reviewed-by: Ondrej Mosnáček <omosnacek@xxxxxxxxx>

Thank you for the fix!

>
>         /* 1. shift any options after CIPSO to the left */
>         memmove(cipso_ptr, cipso_ptr + cipso_len,
>
> base-commit: 99b43ede9e355ba35244cc9470bf1819774ce39d
> --
> 2.55.0
>