[PATCH net] ipv4: fix use-after-free in fib_nhc_update_mtu()

[PATCH net] ipv4: fix use-after-free in fib_nhc_update_mtu()

[Chengfeng Ye]

fib_nhc_update_mtu() walks the nexthop exception table under RTNL, but
RTNL does not serialize this walk with PMTU exception updates. The walk
uses rcu_dereference_protected() with a constant true condition without
holding either fnhe_lock or an RCU read-side critical section.

The following interleaving can therefore occur:

  CPU 0                              CPU 1
  fib_nhc_update_mtu()               update_or_create_fnhe()
    load fnhe                          spin_lock_bh(&fnhe_lock)
                                       fnhe_remove_oldest()
                                         unlink fnhe
                                         kfree_rcu(fnhe, rcu)
    <quiescent state>
    access fnhe after grace period

KASAN reported:

  BUG: KASAN: slab-use-after-free in fib_nhc_update_mtu+0x3df/0x410
  Read of size 8 at addr ffff888107d49000 by task poc/90
  Call Trace:
   fib_nhc_update_mtu+0x3df/0x410
   fib_sync_mtu+0x7a/0xd0
   fib_netdev_event+0x229/0x3f0
   netif_set_mtu_ext+0x33a/0x570
   dev_set_mtu+0x88/0x120
  Allocated by task 89:
   update_or_create_fnhe+0xa80/0x1110
   __ip_rt_update_pmtu+0x8f2/0xcd0
   ipv4_sk_update_pmtu+0x49e/0x690
   udp_err+0xd92/0x1080
  Freed by task 0:
   __kasan_slab_free+0x43/0x70
   kvfree_rcu_cb+0x12f/0x420
   rcu_core+0x509/0x18e0

Protect the full walk with rcu_read_lock() and use rcu_dereference()
for the RCU-published pointers. This prevents reclaim from completing
until all references held by the walk have been dropped, without
serializing PMTU updates against the entire hash-table traversal.

Fixes: af7d6cce5369 ("net: ipv4: update fnhe_pmtu when first hop's MTU changes")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---
 net/ipv4/fib_semantics.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4f3c0740dde9..6ecc6654427c 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1879,16 +1879,17 @@ void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
 	struct fnhe_hash_bucket *bucket;
 	int i;
 
-	bucket = rcu_dereference_protected(nhc->nhc_exceptions, 1);
+	rcu_read_lock();
+	bucket = rcu_dereference(nhc->nhc_exceptions);
 	if (!bucket)
-		return;
+		goto out;
 
 	for (i = 0; i < FNHE_HASH_SIZE; i++) {
 		struct fib_nh_exception *fnhe;
 
-		for (fnhe = rcu_dereference_protected(bucket[i].chain, 1);
+		for (fnhe = rcu_dereference(bucket[i].chain);
 		     fnhe;
-		     fnhe = rcu_dereference_protected(fnhe->fnhe_next, 1)) {
+		     fnhe = rcu_dereference(fnhe->fnhe_next)) {
 			if (fnhe->fnhe_mtu_locked) {
 				if (new <= fnhe->fnhe_pmtu) {
 					fnhe->fnhe_pmtu = new;
@@ -1900,6 +1901,8 @@ void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
 			}
 		}
 	}
+out:
+	rcu_read_unlock();
 }
 
 void fib_sync_mtu(struct net_device *dev, u32 orig_mtu)
-- 
2.43.0

Re: [PATCH net] ipv4: fix use-after-free in fib_nhc_update_mtu()

[David Ahern]

On 7/31/26 10:29 AM, Chengfeng Ye wrote:
> fib_nhc_update_mtu() walks the nexthop exception table under RTNL, but
> RTNL does not serialize this walk with PMTU exception updates. The walk
> uses rcu_dereference_protected() with a constant true condition without
> holding either fnhe_lock or an RCU read-side critical section.
> 
> The following interleaving can therefore occur:
> 
>   CPU 0                              CPU 1
>   fib_nhc_update_mtu()               update_or_create_fnhe()
>     load fnhe                          spin_lock_bh(&fnhe_lock)
>                                        fnhe_remove_oldest()
>                                          unlink fnhe
>                                          kfree_rcu(fnhe, rcu)
>     <quiescent state>
>     access fnhe after grace period
> 
> KASAN reported:
> 
>   BUG: KASAN: slab-use-after-free in fib_nhc_update_mtu+0x3df/0x410
>   Read of size 8 at addr ffff888107d49000 by task poc/90
>   Call Trace:
>    fib_nhc_update_mtu+0x3df/0x410
>    fib_sync_mtu+0x7a/0xd0
>    fib_netdev_event+0x229/0x3f0
>    netif_set_mtu_ext+0x33a/0x570
>    dev_set_mtu+0x88/0x120
>   Allocated by task 89:
>    update_or_create_fnhe+0xa80/0x1110
>    __ip_rt_update_pmtu+0x8f2/0xcd0
>    ipv4_sk_update_pmtu+0x49e/0x690
>    udp_err+0xd92/0x1080
>   Freed by task 0:
>    __kasan_slab_free+0x43/0x70
>    kvfree_rcu_cb+0x12f/0x420
>    rcu_core+0x509/0x18e0
> 
> Protect the full walk with rcu_read_lock() and use rcu_dereference()
> for the RCU-published pointers. This prevents reclaim from completing
> until all references held by the walk have been dropped, without
> serializing PMTU updates against the entire hash-table traversal.
> 
> Fixes: af7d6cce5369 ("net: ipv4: update fnhe_pmtu when first hop's MTU changes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>  net/ipv4/fib_semantics.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net] ipv4: fix use-after-free in fib_nhc_update_mtu()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Clashiko says we need a real lock?"
---
ipv4: fix use-after-free in fib_nhc_update_mtu()

This wraps the fnhe hash table walk in fib_nhc_update_mtu() with
rcu_read_lock() and switches the pointer loads from
rcu_dereference_protected(..., 1) to rcu_dereference(), so that an fnhe
freed by kfree_rcu() on another CPU cannot be reclaimed while the walk
still holds a pointer to it.

> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 4f3c0740dde91..6ecc6654427ce 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -1879,16 +1879,17 @@ void fib_nhc_update_mtu(struct fib_nh_common *nhc, u32 new, u32 orig)
>  	struct fnhe_hash_bucket *bucket;
>  	int i;
>  
> -	bucket = rcu_dereference_protected(nhc->nhc_exceptions, 1);
> +	rcu_read_lock();
> +	bucket = rcu_dereference(nhc->nhc_exceptions);
>  	if (!bucket)
> -		return;
> +		goto out;
>  
>  	for (i = 0; i < FNHE_HASH_SIZE; i++) {
>  		struct fib_nh_exception *fnhe;
>  
> -		for (fnhe = rcu_dereference_protected(bucket[i].chain, 1);
> +		for (fnhe = rcu_dereference(bucket[i].chain);
>  		     fnhe;
> -		     fnhe = rcu_dereference_protected(fnhe->fnhe_next, 1)) {
> +		     fnhe = rcu_dereference(fnhe->fnhe_next)) {
>  			if (fnhe->fnhe_mtu_locked) {
>  				if (new <= fnhe->fnhe_pmtu) {
>  					fnhe->fnhe_pmtu = new;

[Severity: High]
This isn't a race introduced by this patch, but since the patch is
specifically fixing the locking of this walk, is the write side left
unprotected on purpose?

fib_nhc_update_mtu() does not only read the fnhe, it stores to
fnhe->fnhe_pmtu and fnhe->fnhe_mtu_locked, and now does so with only
rcu_read_lock() held. Every other writer of these two fields in
net/ipv4/route.c holds fnhe_lock and treats them as one pair, for example
update_or_create_fnhe():

net/ipv4/route.c:update_or_create_fnhe() {
	spin_lock_bh(&fnhe_lock);
	...
		if (pmtu) {
			fnhe->fnhe_pmtu = pmtu;
			fnhe->fnhe_mtu_locked = lock;
		}
		fnhe->fnhe_expires = max(1UL, expires);
		/* Update all cached dsts too */
		rt = rcu_dereference(fnhe->fnhe_rth_input);
		if (rt)
			fill_route_from_fnhe(rt, fnhe);
	...
}

rt_bind_exception() clears the same pair under fnhe_lock on a genid change,
and fnhe_remove_oldest()/ip_del_fnhe() use
rcu_dereference_protected(..., lockdep_is_held(&fnhe_lock)) for the chain,
which suggests fnhe_lock is the intended write-side lock here.

Can the following interleaving mix the pair?

CPU0: dev_set_mtu() -> netif_set_mtu_ext() -> fib_netdev_event() ->
      fib_sync_mtu() -> fib_nhc_update_mtu()

	} else if (new < fnhe->fnhe_pmtu ||
		   orig == fnhe->fnhe_pmtu) {
		/* fnhe_mtu_locked read as false, condition evaluated */

CPU1: icmp frag-needed in softirq -> __ip_rt_update_pmtu() ->
      update_or_create_fnhe()

	spin_lock_bh(&fnhe_lock);
	fnhe->fnhe_pmtu = pmtu;		/* clamped to ip_rt_min_pmtu */
	fnhe->fnhe_mtu_locked = true;

CPU0 then completes its store:

		fnhe->fnhe_pmtu = new;

leaving fnhe_pmtu set to the device MTU with fnhe_mtu_locked still true,
which neither writer intended. Since ip_mtu_locked() then makes
__ip_rt_update_pmtu() return early, would further PMTU reductions for that
destination be suppressed until the exception expires (ip_rt_mtu_expires,
600s by default, with expiry no longer refreshed because the updates are
skipped)?

The mirror ordering looks like it drops the new device MTU clamp instead,
leaving fnhe_pmtu above the link MTU, which is the condition af7d6cce5369
was added to avoid.

There is also no WRITE_ONCE() on these stores while lockless readers such
as ip_mtu_from_fib_result() consume fnhe_pmtu:

net/ipv4/route.c:ip_mtu_from_fib_result() {
	...
		fnhe = find_exception(nhc, daddr);
		if (fnhe && !time_after_eq(jiffies, fnhe->fnhe_expires))
			mtu = fnhe->fnhe_pmtu;
	...
}

Would taking spin_lock_bh(&fnhe_lock) around the walk (or calling a helper
exported from route.c that does) cover both the reclaim race described in
the changelog and the write-write race, and also keep the
rcu_dereference() annotations honest for a walk that mutates the entries?

The changelog says the change works "without serializing PMTU updates
against the entire hash-table traversal" - is that tradeoff still correct
given the walk writes to the entries it visits?