[PATCH net 0/2] ipv4: unclone skbs before recompiling IP options

[PATCH net 0/2] ipv4: unclone skbs before recompiling IP options

[Eric Dumazet]

__ip_options_compile() updates Record Route and Timestamp options in
place. Two callers recompile options on an skb that can be a clone
sharing its data with other skbs, and thus write into shared packet
data:

 - ipv4_send_dest_unreach(), called from ipv4_link_failure()
 - cipso_v4_error(), called from the LSM socket receive hooks

If clones are processed concurrently, an option pointer can change
after it has been checked, corrupting the next option's length, which
__ip_options_echo() then trusts when building the ICMP reply.

Both patches unclone the skb before recompiling the options, as
ip_rcv_options() already does. The two patches are independent.

The issues were reported by Kyle Zeng and found by an AI code review
agent. They were not reproduced, and the fixes were only build-tested.

Eric Dumazet (2):
  ipv4: unshare headers before recompiling IP options in
    ipv4_send_dest_unreach()
  cipso: unshare headers before recompiling IP options in
    cipso_v4_error()

 net/ipv4/cipso_ipv4.c | 3 +++
 net/ipv4/route.c      | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)


base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
-- 
2.53.0

[PATCH net 1/2] ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

[Eric Dumazet]

__ip_options_compile() updates Record Route and Timestamp options in
place. ipv4_send_dest_unreach() can receive skbs whose data is shared
with clones (for example TC-mirred copies), and pskb_network_may_pull()
does not unshare an already linear IPv4 header.

Concurrent link-failure processing of such clones can therefore change
an option pointer after it has been checked. For example, a Record Route
pointer can advance during fib_compute_spec_dst() (called from
spec_dst_fill()), causing the following write to overwrite the next
option's length. __ip_options_echo() then trusts that length when
building the ICMP reply, which can overflow both the stack reply-option
buffer and the cork-option allocation.

Make the header private before recompiling options, as ip_rcv_options()
already does. If the copy fails, do not send the ICMP error.

Fixes: ed0de45a1008 ("ipv4: recompile ip options in ipv4_link_failure")
Reported-by: Kyle Zeng <kylebot@openai.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
The issue was found by an AI code review agent. It was not reproduced,
and the fix was only build-tested.

 net/ipv4/route.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 3a90c8d28b940bf615c165d9b30ac634943586e1..4d8fcc95c2f460585437b1f6fea4e4fd11d3a2f6 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1269,7 +1269,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
 
 	memset(&parm, 0, sizeof(parm));
 	if (ip_hdr(skb)->ihl > 5) {
-		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
+		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4) ||
+		    skb_unclone(skb, GFP_ATOMIC))
 			return;
 		parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
 
-- 
2.53.0

Re: [PATCH net 1/2] ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch adds skb_unclone(skb, GFP_ATOMIC) to
  ipv4_send_dest_unreach() (net/ipv4/route.c:1272-1274).

--

ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

The patch makes the IPv4 header private with skb_unclone() before
__ip_options_compile() rewrites Record Route and Timestamp options in
place in ipv4_send_dest_unreach(). The goal is to stop concurrent
link-failure processing of clones from changing an option pointer after
it has been checked. If the copy fails, no ICMP error is sent.

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 3a90c8d28b940..4d8fcc95c2f46 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1269,7 +1269,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
>  
>  	memset(&parm, 0, sizeof(parm));
>  	if (ip_hdr(skb)->ihl > 5) {
> -		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
> +		if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4) ||
> +		    skb_unclone(skb, GFP_ATOMIC))

[Severity: High]
Can this skb_unclone() leave stale header pointers in callers of
dst_link_failure()?

ipv4_send_dest_unreach() runs as the ipv4 dst_ops->link_failure
callback. Before this patch, a header that was already linear was never
reallocated on this path, because pskb_network_may_pull() returns early.
Now, when the skb is cloned and has options (ihl > 5),
pskb_expand_head() swaps skb->head and calls skb_release_data() on the
old head, which drops this skb's reference.

ip_tunnel_xmit() in net/ipv4/ip_tunnel.c saves a raw pointer into the
head before calling dst_link_failure(), and does not reload it
afterwards:

ip_tunnel_xmit() {
    ...
	inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
    ...
			WRITE_ONCE(tunnel->err_count, err_count - 1);

			dst_link_failure(skb);
    ...
	tos = ip_tunnel_ecn_encap(tos, inner_iph, skb);
	ttl = tnl_params->ttl;
	if (ttl == 0) {
		if (payload_protocol == htons(ETH_P_IP))
			ttl = inner_iph->ttl;
    ...
}

For header_ops GRE devices, ipgre_xmit() in net/ipv4/ip_gre.c also
points tnl_params into the head:

	tnl_params = (const struct iphdr *)skb->data;

The call chain is:

ip_tunnel_xmit()->dst_link_failure()->ipv4_link_failure()->
ipv4_send_dest_unreach()->skb_unclone()->pskb_expand_head()

Cloned skbs do get here. TCP transmits nohdr clones, so
skb_header_cloned() is false and skb_cow_head() in
ipgre_xmit()/gre_tap_xmit() does not unclone them, but skb_cloned() is
still true. An unprivileged user can add RR/TS options with
setsockopt(IP_OPTIONS). err_count goes up on ICMP errors from remote
hosts in ipgre_err().

After the expand, the old head stays alive only while the other clone
holder, such as the TCP rtx queue skb or a tc-mirred original, keeps its
reference. If the tunnel has a qdisc, the clone can be sent without the
socket lock, and an ACK on another CPU could free the original. That
could happen during __icmp_send(), or even inside pskb_expand_head()
between the skb_cloned() check and skb_release_data().

In that case, are the inner_iph->tos, tnl_params->ttl and inner_iph->ttl
reads use-after-free reads, with the stale bytes written into the outer
header's TOS/TTL?

Would it be safer to compile and echo the options from a private copy
rather than uncloning the caller's skb inside a link_failure callback?
Another option would be to reload inner_iph and tnl_params in
ip_tunnel_xmit() after dst_link_failure(). Either way, it seems worth
checking the other dst_link_failure() callers for the same pattern.

>  			return;
>  		parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106234313.2537882-1-edumazet%40kernel.org

Re: [PATCH net 1/2] ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()

[Eric Dumazet]

Le dim. 8 nov. 2026 à 01:12, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] This patch adds skb_unclone(skb, GFP_ATOMIC) to
>   ipv4_send_dest_unreach() (net/ipv4/route.c:1272-1274).
>
> --
>
> ipv4: unshare headers before recompiling IP options in ipv4_send_dest_unreach()
>
> The patch makes the IPv4 header private with skb_unclone() before
> __ip_options_compile() rewrites Record Route and Timestamp options in
> place in ipv4_send_dest_unreach(). The goal is to stop concurrent
> link-failure processing of clones from changing an option pointer after
> it has been checked. If the copy fails, no ICMP error is sent.
>
> > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > index 3a90c8d28b940..4d8fcc95c2f46 100644
> > --- a/net/ipv4/route.c
> > +++ b/net/ipv4/route.c
> > @@ -1269,7 +1269,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
> >
> >       memset(&parm, 0, sizeof(parm));
> >       if (ip_hdr(skb)->ihl > 5) {
> > -             if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
> > +             if (!pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4) ||
> > +                 skb_unclone(skb, GFP_ATOMIC))
>
> [Severity: High]
> Can this skb_unclone() leave stale header pointers in callers of
> dst_link_failure()?
>
> ipv4_send_dest_unreach() runs as the ipv4 dst_ops->link_failure
> callback. Before this patch, a header that was already linear was never
> reallocated on this path, because pskb_network_may_pull() returns early.
> Now, when the skb is cloned and has options (ihl > 5),
> pskb_expand_head() swaps skb->head and calls skb_release_data() on the
> old head, which drops this skb's reference.
>
> ip_tunnel_xmit() in net/ipv4/ip_tunnel.c saves a raw pointer into the
> head before calling dst_link_failure(), and does not reload it
> afterwards:
>
> ip_tunnel_xmit() {
>     ...
>         inner_iph = (const struct iphdr *)skb_inner_network_header(skb);
>     ...
>                         WRITE_ONCE(tunnel->err_count, err_count - 1);
>
>                         dst_link_failure(skb);
>     ...
>         tos = ip_tunnel_ecn_encap(tos, inner_iph, skb);
>         ttl = tnl_params->ttl;
>         if (ttl == 0) {
>                 if (payload_protocol == htons(ETH_P_IP))
>                         ttl = inner_iph->ttl;
>     ...
> }
>
> For header_ops GRE devices, ipgre_xmit() in net/ipv4/ip_gre.c also
> points tnl_params into the head:
>
>         tnl_params = (const struct iphdr *)skb->data;
>
> The call chain is:
>
> ip_tunnel_xmit()->dst_link_failure()->ipv4_link_failure()->
> ipv4_send_dest_unreach()->skb_unclone()->pskb_expand_head()
>
> Cloned skbs do get here. TCP transmits nohdr clones, so
> skb_header_cloned() is false and skb_cow_head() in
> ipgre_xmit()/gre_tap_xmit() does not unclone them, but skb_cloned() is
> still true. An unprivileged user can add RR/TS options with
> setsockopt(IP_OPTIONS). err_count goes up on ICMP errors from remote
> hosts in ipgre_err().
>
> After the expand, the old head stays alive only while the other clone
> holder, such as the TCP rtx queue skb or a tc-mirred original, keeps its
> reference. If the tunnel has a qdisc, the clone can be sent without the
> socket lock, and an ACK on another CPU could free the original. That
> could happen during __icmp_send(), or even inside pskb_expand_head()
> between the skb_cloned() check and skb_release_data().
>
> In that case, are the inner_iph->tos, tnl_params->ttl and inner_iph->ttl
> reads use-after-free reads, with the stale bytes written into the outer
> header's TOS/TTL?
>
> Would it be safer to compile and echo the options from a private copy
> rather than uncloning the caller's skb inside a link_failure callback?
> Another option would be to reload inner_iph and tnl_params in
> ip_tunnel_xmit() after dst_link_failure(). Either way, it seems worth
> checking the other dst_link_failure() callers for the same pattern.

Good catch, thanks. Callers of dst_link_failure() such as
ip_tunnel_xmit() keep pointers into skb->head (inner_iph, and
tnl_params for GRE devices with header_ops) across the call, so
ipv4_send_dest_unreach() must not reallocate the caller's skb head.

v2 will compile and echo the options on a private pskb_copy() instead,
leaving the caller's skb untouched.

pw-bot: cr

[PATCH net 2/2] cipso: unshare headers before recompiling IP options in cipso_v4_error()

[Eric Dumazet]

cipso_v4_error() recompiles the IP options of the offending packet with
__ip_options_compile() before echoing them in an ICMP error.
__ip_options_compile() updates Record Route and Timestamp options in
place.

cipso_v4_error() is called from the LSM socket receive hooks through
netlbl_skbuff_err(). ip_rcv_options() unshared the IPv4 header earlier,
but at socket delivery time the skb can again be a clone sharing its
data with other skbs, such as the per-socket clones made for raw sockets
or multicast/broadcast delivery. Writing to the options then modifies
packet data seen by the other users of the buffer, and a concurrent
recompilation would hit the same check-then-write race as the one fixed
in ipv4_send_dest_unreach().

Make the header private before recompiling options, as ip_rcv_options()
already does. If the copy fails, do not send the ICMP error.

Fixes: 3da1ed7ac398 ("net: avoid use IPCB in cipso_v4_error")
Reported-by: Kyle Zeng <kylebot@openai.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
The issue was found by an AI code review agent. It was not reproduced,
and the fix was only build-tested.

 net/ipv4/cipso_ipv4.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index e65a3f29d2caf31c34cc5aaaf862257d7d374275..f5f6a812ab9ef05117e002779baf136e615c043d 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1725,6 +1725,9 @@ void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
 	 * so we can not use icmp_send and IPCB here.
 	 */
 
+	if (skb_unclone(skb, GFP_ATOMIC))
+		return;
+
 	memset(&parm, 0, sizeof(parm));
 	parm.opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
 	rcu_read_lock();
-- 
2.53.0

Re: [PATCH net 2/2] cipso: unshare headers before recompiling IP options in cipso_v4_error()

[Paul Moore]

On Fri, Nov 6, 2026 at 6:43 PM Eric Dumazet <edumazet@kernel.org> wrote:
>
> cipso_v4_error() recompiles the IP options of the offending packet with
> __ip_options_compile() before echoing them in an ICMP error.
> __ip_options_compile() updates Record Route and Timestamp options in
> place.
>
> cipso_v4_error() is called from the LSM socket receive hooks through
> netlbl_skbuff_err(). ip_rcv_options() unshared the IPv4 header earlier,
> but at socket delivery time the skb can again be a clone sharing its
> data with other skbs, such as the per-socket clones made for raw sockets
> or multicast/broadcast delivery. Writing to the options then modifies
> packet data seen by the other users of the buffer, and a concurrent
> recompilation would hit the same check-then-write race as the one fixed
> in ipv4_send_dest_unreach().
>
> Make the header private before recompiling options, as ip_rcv_options()
> already does. If the copy fails, do not send the ICMP error.
>
> Fixes: 3da1ed7ac398 ("net: avoid use IPCB in cipso_v4_error")
> Reported-by: Kyle Zeng <kylebot@openai.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> ---
> The issue was found by an AI code review agent. It was not reproduced,
> and the fix was only build-tested.
>
>  net/ipv4/cipso_ipv4.c | 3 +++
>  1 file changed, 3 insertions(+)

This seems okay to me, thanks Eric.

Acked-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com