Re: [PATCH net] seg6: ensure packet data is writable before modifying SRH and IPv6 DA

[Hangbin Liu]

Hi Andrea,
On Fri, Sep 25, 2026 at 03:38:07PM +0200, Andrea Mayer wrote:
> advance_nextseg() modifies the SRH Segments Left field and the IPv6
> destination address without ensuring the packet data is writable.
> seg6_next_csid_advance_arg() has the same problem when it advances the
> NEXT-C-SID argument in the destination address.
> 
> The skb may be cloned, for example by an AF_PACKET socket receiving on
> the ingress device. advance_nextseg() and seg6_next_csid_advance_arg()
> then write into the packet data shared with the clone. A read from that
> socket can return the modified packet instead of the received one. The
> simplified path below shows this for advance_nextseg():
> 
> __netif_receive_skb_one_core
>   __netif_receive_skb_core
>     deliver_skb               [orig: users=2, cloned=0]
>       packet_rcv
>         skb_clone             clone queued to the AF_PACKET socket
>         consume_skb(orig)     [orig: users=1, cloned=1]
>   ipv6_rcv
>     ip6_rcv_core              skb_share_check: no-op [orig: users=1]
>     [...]
>       input_action_end_core
>         advance_nextseg       writes into the data shared with the clone
> 
> Call skb_ensure_writable() in advance_nextseg() and in
> seg6_next_csid_advance_arg() before they modify the packet data.
> skb_ensure_writable() may reallocate skb->head, which invalidates the
> pointers into the packet data taken before the call.
> advance_nextseg() now returns a valid SRH pointer, or NULL if
> skb_ensure_writable() fails.
> seg6_next_csid_advance_arg() takes the pointer to the destination
> address from the skb after skb_ensure_writable().
> On failure, the callers drop the packet with SKB_DROP_REASON_NOMEM.
> 
> Fixes: 140f04c33bbc ("ipv6: sr: implement several seg6local actions")
> Fixes: 848f3c0d4769 ("seg6: add NEXT-C-SID support for SRv6 End behavior")
> Signed-off-by: Andrea Mayer <andrea.mayer@xxxxxxxxxxx>
> ---
>  net/ipv6/seg6_local.c | 140 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 117 insertions(+), 23 deletions(-)

[...]

>  
>  static int input_action_end_finish(struct sk_buff *skb,
> @@ -408,31 +454,42 @@ static int input_action_end_finish(struct sk_buff *skb,
>  static int input_action_end_core(struct sk_buff *skb,
>  				 struct seg6_local_lwt *slwt)
>  {
> +	enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  	struct ipv6_sr_hdr *srh;
> +	int err = -EINVAL;
>  
>  	srh = get_and_validate_srh(skb);
>  	if (!srh)
>  		goto drop;
>  
> -	advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> +	srh = advance_nextseg(skb, srh, &reason);
> +	if (!srh) {
> +		err = -ENOMEM;
> +		goto drop;
> +	}

The srh is not used in the later function, maybe just

	if (!advance_nextseg(skb, srh, &reason)) {
		err = -ENOMEM;
		goto drop;
	}

>  
>  	return input_action_end_finish(skb, slwt);
>  
>  drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +	kfree_skb_reason(skb, reason);
> +	return err;
>  }
>  
>  static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt)
>  {
>  	const struct seg6_flavors_info *finfo = &slwt->flv_info;
>  	struct in6_addr *daddr = &ipv6_hdr(skb)->daddr;
> +	enum skb_drop_reason reason;
>  
>  	if (seg6_next_csid_is_arg_zero(daddr, finfo))
>  		return input_action_end_core(skb, slwt);
>  
>  	/* update DA */
> -	seg6_next_csid_advance_arg(daddr, finfo);
> +	reason = seg6_next_csid_advance_arg(skb, finfo);
> +	if (reason) {
> +		kfree_skb_reason(skb, reason);
> +		return -ENOMEM;
> +	}
>  
>  	return input_action_end_finish(skb, slwt);
>  }
> @@ -448,19 +505,25 @@ static int input_action_end_x_finish(struct sk_buff *skb,
>  static int input_action_end_x_core(struct sk_buff *skb,
>  				   struct seg6_local_lwt *slwt)
>  {
> +	enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  	struct ipv6_sr_hdr *srh;
> +	int err = -EINVAL;
>  
>  	srh = get_and_validate_srh(skb);
>  	if (!srh)
>  		goto drop;
>  
> -	advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> +	srh = advance_nextseg(skb, srh, &reason);
> +	if (!srh) {
> +		err = -ENOMEM;
> +		goto drop;
> +	}

Same here and all later similar functions.

>  
>  	return input_action_end_x_finish(skb, slwt);
>  
>  drop:
> -	kfree_skb(skb);
> -	return -EINVAL;
> +	kfree_skb_reason(skb, reason);
> +	return err;
>  }
>  

[...]

>  
> @@ -1434,6 +1523,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
>  				struct seg6_local_lwt *slwt)
>  {
>  	struct seg6_bpf_srh_state *srh_state;
> +	enum skb_drop_reason reason;
>  	struct ipv6_sr_hdr *srh;
>  	int ret;
>  
> @@ -1442,7 +1532,11 @@ static int input_action_end_bpf(struct sk_buff *skb,
>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
> -	advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> +	srh = advance_nextseg(skb, srh, &reason);
> +	if (!srh) {
> +		kfree_skb_reason(skb, reason);
> +		return -ENOMEM;
> +	}

The srh here is still needed by later srh_state->srh = srh; so we can keep it.

Thanks
Hangbin