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 for Android: free password hash cracker in your pocket

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

Message-Id: <20251214143942.ccc2ec1a46ce6a8fcc3ede55@uniroma2.it>
Date: Sun, 14 Dec 2025 14:39:42 +0100
From: Andrea Mayer <andrea.mayer@...roma2.it>
To: nicolas.dichtel@...nd.com
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, Andrea Mayer
 <andrea.mayer@...roma2.it>
Subject: Re: [PATCH net] seg6: fix route leak for encap routes

On Wed, 10 Dec 2025 18:00:39 +0100
Nicolas Dichtel <nicolas.dichtel@...nd.com> wrote:

> Le 10/12/2025 à 11:37, Andrea Mayer a écrit :
> > On Mon,  8 Dec 2025 11:24:34 +0100
> > Nicolas Dichtel <nicolas.dichtel@...nd.com> wrote:
> > 
> >> The goal is to take into account the device used to set up the route.
> >> Before this commit, it was mandatory but ignored. After encapsulation, a
> >> second route lookup is performed using the encapsulated IPv6 address.
> >> This route lookup is now done in the vrf where the route device is set.
> >>
> > 
> > Hi Nicolas,
> Hi Andrea,
> 

Hi Nicolas,

> > 
> > 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.


> > 
> > 
> >> The l3vpn tests show the inconsistency; they are updated to reflect the
> >> fix. Before the commit, the route to 'fc00:21:100::6046' was put in the
> >> vrf-100 table while the encap route was pointing to veth0, which is not
> >> associated with a vrf.
> >>
> >> Before:
> >>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> >>> cafe::1  encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> >>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
> >>
> >> After:
> >>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> >>> cafe::1  encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> >>> $ ip -n rt_2-Rh5GP7 -6 r list | grep fc00:21:100::6046
> >>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
> >>
> >> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
> >> ---
> >>  net/ipv6/seg6_iptunnel.c                                | 6 ++++++
> >>  tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh | 2 +-
> >>  tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh  | 2 +-
> >>  tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh  | 2 +-
> >>  4 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
> >> index 3e1b9991131a..9535aea28357 100644
> >> --- 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.


> >> +		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.


> >> +		rcu_read_unlock();
> >> +	}


> Thanks,
> Nicolas

Thanks,

Ciao,
Andrea