Re: [PATCH net 2/5] ipv4: fix shift-out-of-bounds in ip_rt_send_redirect()

[Fernando Fernandez Mancera]

On 10/23/26 11:13 AM, Jiayuan Chen wrote:
> ip_rt_redirect_number is the max number of ICMP Redirects we send to one
> endpoint; once we hit it we stop. Before that, ip_rt_redirect_load gives
> the redirects an exponential backoff:
>
> 	if (peer->n_redirects == 0 ||
> 	    time_after(jiffies,
> 		       (peer->rate_last +
> 			(ip_rt_redirect_load << peer->n_redirects)))) {
>
> n_redirects only stops growing when it reaches ip_rt_redirect_number, and
> both redirect_number and redirect_load are plain proc_dointvec sysctls
> with no upper bound. Set redirect_number past 32 and n_redirects climbs
> past 31, so "ip_rt_redirect_load << peer->n_redirects" shifts a 32-bit
> int by >= 32 bits, which is UB:
>
>    UBSAN: shift-out-of-bounds in net/ipv4/route.c:949:6
>    shift exponent 32 is too large for 32-bit type 'int'
>    CPU: 1 UID: 0 PID: 1845 Comm: ping Not tainted 7.3.0-rc7+ #364 PREEMPT
>    Call Trace:
>     <IRQ>
>     dump_stack_lvl+0xc1/0xf0
>     dump_stack+0x10/0x20
>     __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
>     ip_rt_send_redirect.cold+0x2c/0x146
>     ip_forward+0x1ddc/0x2430
>     ip_rcv+0x2b1/0x320
>     __netif_receive_skb_one_core+0x16b/0x1c0
>     __netif_receive_skb+0x21/0x150
>     process_backlog+0x37e/0x1200
>     __napi_poll.constprop.0+0xa5/0x5e0
>     net_rx_action+0x4c0/0xf70
>
> Fixing this from the sysctl side would mean bounding both knobs, e.g.
> redirect_load to [0, 65535] and redirect_number to [0, 16], which gets
> in the way of users who legitimately do not want to cap ICMP redirects.
> Instead compute the backoff in u64 with the shift count capped at 31,
> which cannot overflow since ip_rt_redirect_load is an int, and clamp the
> result to MAX_JIFFY_OFFSET.

I think we should cap redirect_load with a minimum valid value of 0 on 
the sysctl. If the user want to disable ICMP redirects or do not cap 
them, they should use 0 or -1 in redirect_number IMHO.

What do you think?

> The clamp also fixes 32-bit kernels, where the same expression goes
> wrong long before the shift count reaches 32. With the default
> ip_rt_redirect_load of 20, from n_redirects == 27 on the result is
> either truncated or too large for time_after(), which then treats the
> deadline as already passed, and from 30 on it is 0, so every forwarded
> packet gets a redirect again until ip_rt_redirect_number is reached.
> This needs a raised redirect_silence to be reachable, otherwise
> n_redirects is reset long before. On 64-bit, ip_rt_redirect_load << 31
> stays below MAX_JIFFY_OFFSET, so nothing changes there.

Isn't the same issue in 64-bit kernels too? In essence, it would be (int 
<< u32) which results in a negative value and that is later 
sign-extended to a corrupted 64-bit value. Am I missing something here?

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> ---
>   net/ipv4/route.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 595a3b7f1783..76807b757046 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -907,6 +907,7 @@ void ip_rt_send_redirect(struct sk_buff *skb)
>   	struct inet_peer *peer;
>   	int log_martians;
>   	struct net *net;
> +	u64 backoff;
>   	int vif;
>   
>   	rcu_read_lock();
> @@ -945,10 +946,10 @@ void ip_rt_send_redirect(struct sk_buff *skb)
>   	/* Check for load limit; set rate_last to the latest sent
>   	 * redirect.
>   	 */
> +	backoff = (u64)ip_rt_redirect_load << min(peer->n_redirects, 31U);
> +	backoff = min_t(u64, backoff, MAX_JIFFY_OFFSET);
>   	if (peer->n_redirects == 0 ||
> -	    time_after(jiffies,
> -		       (peer->rate_last +
> -			(ip_rt_redirect_load << peer->n_redirects)))) {
> +	    time_after(jiffies, peer->rate_last + (unsigned long)backoff)) {
>   		__be32 gw = rt_nexthop(rt, ip_hdr(skb)->daddr);
>   
>   		icmp_send(skb, ICMP_REDIRECT, ICMP_REDIR_HOST, gw);