Re: [PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

Hi Marc,

do you consider to upstream this patch together with the BCM patch set?

I know sashiko-bot had again some "Pre-existing issue" remarked on this 
patch. But this simple fix should probably go into upstream, before the 
next "AI Pre-existing issue"-party starts with isotp.c and sashiko-bot ;-)

Best regards,
Oliver

On 07.07.26 11:47, Oliver Hartkopp wrote:
> isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister()
> and clears so->bound without waiting for a grace period. isotp_release()
> uses so->bound to decide whether it needs to call synchronize_rcu()
> before cancelling so->rxtimer, so when NETDEV_UNREGISTER runs first it
> skips that synchronize_rcu() and can cancel the timer while an
> in-flight isotp_rcv() is still executing and about to re-arm it via
> isotp_send_fc(), leading to a use-after-free timer callback on the
> freed socket.
> 
> sakisho-bot remarked a problem with rtnl_lock held in isotp_notify(),
> therefore make isotp_release() always call synchronize_rcu() before
> cancelling the timers, regardless of so->bound. This still closes the
> original race (isotp_notify() clearing so->bound without waiting for
> in-flight isotp_rcv() callers before isotp_release() cancels the RX
> timer) without adding any RCU wait to the netdevice notifier path.
> 
> Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
> Closes: https://lore.kernel.org/linux-can/[email protected]/
> Reported-by: Nico Yip <[email protected]> (ZDI-CAN-31764)
> Signed-off-by: Oliver Hartkopp <[email protected]>
> ---
>   net/can/isotp.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297..d30937345bcd 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1235,15 +1235,22 @@ static int isotp_release(struct socket *sock)
>   
>   				can_rx_unregister(net, dev, so->txid,
>   						  SINGLE_MASK(so->txid),
>   						  isotp_rcv_echo, sk);
>   				dev_put(dev);
> -				synchronize_rcu();
>   			}
>   		}
>   	}
>   
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +
>   	hrtimer_cancel(&so->txfrtimer);
>   	hrtimer_cancel(&so->txtimer);
>   	hrtimer_cancel(&so->rxtimer);
>   
>   	so->ifindex = 0;