netdev - Re: [PATCH net] seg6: fix route leak for encap routes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed575eb5-013c-413f-b411-be14f7acc249@6wind.com>
Date: Thu, 18 Dec 2025 15:24:10 +0100
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Andrea Mayer <andrea.mayer@...roma2.it>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>, David Lebrun
 <david.lebrun@...ouvain.be>, Paolo Lungaroni <paolo.lungaroni@...roma2.it>,
 David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
 stefano.salsano@...roma2.it
Subject: Re: [PATCH net] seg6: fix route leak for encap routes

Le 14/12/2025 à 14:39, Andrea Mayer a écrit :
[snip]
>>> I've got your point. However, I'm still concerned about the implications of
>>> using the *dev* field in the root lookup. This field has been ignored for this
>>> purpose so far, so some existing configurations/scripts may need to be adapted
>>> to work again. The adjustments made to the self-tests below show what might
>>> happen.
>> Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
>> something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>>
> 
> IMHO using a new attribute seems to be a safer approach.
> 
> Is this new attribute intended to be used (a) to enable/disable the use of *dev*
> during the route lookup, or (b) to carry the interface identifier (oif)
> explicitly for use in the lookup?
> In the latter case (b), the route *dev* would no longer be consulted at all for 
> this purpose.
I would tend to prefer option (a), something like:
> cafe::1  encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] use_dev dev eth0

With option (b), we could end up with this kind of route:
> cafe::1  encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] vrf blue dev eth0
where eth0 is not in vrf blue. This is confusing.

[snip]

>>>> --- a/net/ipv6/seg6_iptunnel.c
>>>> +++ b/net/ipv6/seg6_iptunnel.c
>>>> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
>>>>  	 * now and use it later as a comparison.
>>>>  	 */
>>>>  	lwtst = orig_dst->lwtstate;
>>>> +	if (orig_dst->dev) {
>>>
>>> When can 'orig_dst->dev' be NULL in this context?
>> I was cautious to avoid any unpleasant surprises. A dst can have dst->dev set to
>> NULL.
>>
> 
> I see your point regarding caution.
> 
> However, if 'orig_dst->dev' were NULL at this point, the kernel would crash
> anyway because subsequent functions (e.g., __seg6_do_srh_encap()) rely on
> 'orig_dst->dev' (not NULL) to retrieve the net.
Right, I will remove the test.

> 
> 
>>>> +		rcu_read_lock();
>>>> +		skb->dev = l3mdev_master_dev_rcu(orig_dst->dev) ?:
>>>> +			dev_net(skb->dev)->loopback_dev;
> 
> One issue here is that the outgoing device (*dev*) is being treated as the
> packet's *incoming* interface.
> 
> ip6_route_input() uses 'skb->dev->ifindex' to populate 'flowi6_iif'.
> Consequently, if there is an 'ip rule' matching on 'iif' (ingress interface),
> it will evaluate against the *dev* (the VRF or the loopback) instead of the
> actual interface the packet was received on.
> This can lead to incorrect policy routing lookups.
Hmm, right, it should be changed only in case of x-vrf.

Thanks,
Nicolas