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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new backoff code in ip_rt_send_redirect() casts the signed
  sysctl `ip_rt_redirect_load` straight to u64.

--

ipv4: fix shift-out-of-bounds in ip_rt_send_redirect()

This commit fixes a UBSAN shift-out-of-bounds in ip_rt_send_redirect().
The shift happens when ip_rt_redirect_number is set above 32. The backoff
is now computed in u64 with the shift count capped at 31, and the result
is clamped to MAX_JIFFY_OFFSET before it is passed to time_after().

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 595a3b7f17830..76807b7570467 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c

[ ... ]

> @@ -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);

[Severity: Medium]
What happens here when ip_rt_redirect_load is negative?

The redirect_load sysctl is a plain proc_dointvec with no extra1/extra2
bounds, so root can write a negative value:

net/ipv4/route.c:ipv4_route_table[] {
    ...
	.procname	= "redirect_load",
	.data		= &ip_rt_redirect_load,
	.maxlen		= sizeof(int),
	.mode		= 0644,
	.proc_handler	= proc_dointvec,
    ...
}

When the load is a negative x, (u64)ip_rt_redirect_load sign-extends to
2^64 + x. For any shift n <= 31, the shifted value mod 2^64 is
2^64 + x * 2^n, which is still at or above 2^63.

The min_t() would then always return MAX_JIFFY_OFFSET, and this check
would effectively never be true:

    time_after(jiffies, peer->rate_last + MAX_JIFFY_OFFSET)

So after the first redirect (n_redirects == 0 skips the check), does a
peer get no more redirects until the ip_rt_redirect_silence reset clears
n_redirects?

Before this patch, a negative load produced a negative offset from
rate_last. time_after() was then always true, so a redirect went out on
every packet until ip_rt_redirect_number was reached. With this patch a
negative redirect_load seems to change from no backoff to maximum backoff.

The commit message says the u64 computation "cannot overflow since
ip_rt_redirect_load is an int". That only seems to hold for non-negative
values.

A later patch in this series restricts message_cost and message_burst to
non-negative values. It doesn't touch redirect_load, and route.c is
unchanged through the end of the series.

Would it make sense to treat a negative load as 0 before the cast? The
other option is a lower bound on the sysctl, for example
proc_dointvec_minmax with extra1 = SYSCTL_ZERO.

>  	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);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023091419.167088-1-jiayuan.chen%40linux.dev