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

[Jiayuan Chen]

On 10/23/26 10:11 PM, Fernando Fernandez Mancera wrote:
> 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?

Thanks for pointing this out, agreed. A negative redirect_load never
meant anything: with the old int shift it just made the deadline land in
the past, i.e. no backoff at all, which is exactly what 0 gives. And once
converted to u64 it would turn into the maximum backoff instead, so it
should be rejected rather than silently mapped to something.