Re: [PATCH net] seg6: fix transport header for inner IPv4 with IP options

[Paolo Abeni <pabeni@redhat.com>]

On 10/21/26 03:32, Hangbin Liu wrote:
> On Tue, Oct 20, 2026 at 04:33:36AM +0200, Andrea Mayer wrote:
>> The End.DT4, End.DT46 (its IPv4 path only) and End.DX4 behaviors set the
>> transport header of the decapsulated IPv4 packet at a fixed 20 bytes, the
>> size of an IPv4 header without options. They do not validate the header
>> length either. With options, the transport header points at the first
>> option byte. The effects include:
>> - ping -R sends an echo request with a 60-byte IPv4 header to a local
>>    address through an End.DT4 or an End.DX4 SID. Since icmp_rcv()
>>    computes the ICMP checksum from the first option byte, the request
>>    fails the check and is counted in IcmpInCsumErrors.
>> - A UDP datagram can reach a port that an iptables rule drops. The rule
>>    checks the UDP header after the full IPv4 header, while the socket
>>    lookup reads it at the fixed 20-byte offset.
>> - With an inbound AH state, ah_input() reads the AH header at the fixed
>>    20-byte offset, but copies the IPv4 header over it with the length in
>>    the IHL field. A packet shorter than that length can make ah_input()
>>    write past the end of the data, into skb_shared_info when the skb has
>>    no tailroom.
>>
>> Validate the inner IPv4 header length with the new helper
>> seg6_validate_iphdr_len() and set the transport header at that length. A
>> packet that is not IPv4, whose header length field is below 5, or whose
>> header is truncated, is dropped with a drop reason. The fix is limited to
>> the header length, which the transport header relies on. Other checks done
>> on IPv4 input, such as the header checksum, the total length and the IP
>> options, are not addressed here.
>>
>> Found by code inspection while reviewing commit 44930446dde4 ("ipv6: seg6:
>> clear IPv4 control block on IPIP decapsulation"), as a pre-existing issue
>> that Sashiko also reported on v2 of that patch. Without this patch, a
>> bpftrace probe on ah_input() shows the reporter's reproducer packet
>> written into skb_shared_info. With it, that packet is dropped before
>> ah_input(), ping -R no longer fails the ICMP checksum, the UDP datagram
>> reaches its real port, and the SRv6 selftests still pass. These effects
>> reproduce on an unmodified kernel, without error injection.
>>
>> Fixes: 891ef8dd2a8d ("ipv6: sr: implement additional seg6local actions")
>> Fixes: 664d6f86868b ("seg6: add support for the SRv6 End.DT4 behavior")
>> Reported-by: Hazley Samsudin <hazleycryilis@gmail.com>
>> Closes: https://lore.kernel.org/netdev/20260809221915.2ffc1ecd0f445fcb70270e31@uniroma2.it/
>> Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
>> ---
>>   net/ipv6/seg6_local.c | 56 ++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
>> index 53f7b2452575..c249f7751d41 100644
>> --- a/net/ipv6/seg6_local.c
>> +++ b/net/ipv6/seg6_local.c
>> @@ -1056,6 +1056,40 @@ static int input_action_end_dx6(struct sk_buff *skb,
>>   	return -EINVAL;
>>   }
>>   
>> +/* Validate the length of the IPv4 header at skb->data (which must point to
>> + * the network header) and report that length, options included.
>> + * Only the version, the header length and the presence of the whole header
>> + * are checked.
>> + * The length is written only when SKB_NOT_DROPPED_YET is returned.
>> + *
>> + * skb pointers may change: after this call, the caller must evaluate again
>> + * any pointer into the packet data.
>> + */
>> +static enum skb_drop_reason
>> +seg6_validate_iphdr_len(struct sk_buff *skb, int *hdrlen)
>> +{
>> +	enum skb_drop_reason reason;
>> +	const struct iphdr *iph;
>> +	int len;
>> +
>> +	reason = pskb_may_pull_reason(skb, sizeof(struct iphdr));
>> +	if (reason)
>> +		return reason;
>> +
>> +	iph = ip_hdr(skb);
>> +	if (iph->version != 4 || iph->ihl < 5)
>> +		return SKB_DROP_REASON_IP_INHDR;
>> +
>> +	len = ip_hdrlen(skb);
>> +
>> +	reason = pskb_may_pull_reason(skb, len);
>> +	if (reason)
>> +		return reason;
>> +
>> +	*hdrlen = len;
>> +	return SKB_NOT_DROPPED_YET;
>> +}
>> +
>>   static int input_action_end_dx4_finish(struct net *net, struct sock *sk,
>>   				       struct sk_buff *skb)
>>   {
>> @@ -1085,14 +1119,18 @@ static int input_action_end_dx4_finish(struct net *net, struct sock *sk,
>>   static int input_action_end_dx4(struct sk_buff *skb,
>>   				struct seg6_local_lwt *slwt)
>>   {
>> +	enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
>> +	int hdrlen;
>> +
>>   	if (!decap_and_validate(skb, IPPROTO_IPIP))
>>   		goto drop;
>>   
>> -	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
>> +	reason = seg6_validate_iphdr_len(skb, &hdrlen);
>> +	if (reason)
>>   		goto drop;
>>   
>>   	skb->protocol = htons(ETH_P_IP);
>> -	skb_set_transport_header(skb, sizeof(struct iphdr));
>> +	skb_set_transport_header(skb, hdrlen);
>>   	nf_reset_ct(skb);
>>   
>>   	if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled))
>> @@ -1102,7 +1140,7 @@ static int input_action_end_dx4(struct sk_buff *skb,
>>   
>>   	return input_action_end_dx4_finish(dev_net(skb->dev), NULL, skb);
>>   drop:
>> -	kfree_skb(skb);
>> +	kfree_skb_reason(skb, reason);
>>   	return -EINVAL;
>>   }
>>   
>> @@ -1234,6 +1272,7 @@ static struct net_device *end_dt_get_vrf_rcu(struct sk_buff *skb,
>>   static struct sk_buff *end_dt_vrf_core(struct sk_buff *skb,
>>   				       struct seg6_local_lwt *slwt, u16 family)
>>   {
>> +	enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
> 
> Nit: We can use SKB_DR(reason) without the long definition.
> 
>>   	struct seg6_end_dt_info *info = &slwt->dt_info;
>>   	struct net_device *vrf;
>>   	__be16 protocol;
>> @@ -1246,7 +1285,9 @@ static struct sk_buff *end_dt_vrf_core(struct sk_buff *skb,
>>   	switch (family) {
>>   	case AF_INET:
>>   		protocol = htons(ETH_P_IP);
>> -		hdrlen = sizeof(struct iphdr);
>> +		reason = seg6_validate_iphdr_len(skb, &hdrlen);
>> +		if (unlikely(reason))
>> +			goto drop;
>>   		break;
>>   	case AF_INET6:
>>   		protocol = htons(ETH_P_IPV6);
>> @@ -1260,6 +1301,7 @@ static struct sk_buff *end_dt_vrf_core(struct sk_buff *skb,
>>   
>>   	if (unlikely(info->family != AF_UNSPEC && info->family != family)) {
>>   		pr_warn_once("seg6local: SRv6 End.DT* family mismatch");
>> +		reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>   		goto drop;
>>   	}
> 
> And maybe move this checking upper before switch(), then we don't need
> to reset reason.
I think both suggestions make sense, please send a v2.

Thanks,

Paolo