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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

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.