Re: [PATCH net-next 08/37] can: bcm: fix filter removal race between release and netns change

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 16.10.26 04:37, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
> 
> 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 commit moves remove_proc_entry() ahead of rtnl_lock() so
>    that procfs readers cannot stall RTNL from close().

Fixed in new version.

> - [Medium] bcm_release() now holds the global rtnl_mutex across the whole
>    rx_ops unregistration loop (rtnl_lock() at net/can/bcm.c:1951,…

See below.

> - [Low] Stale comment in bcm_release().

Fixed in new version.

> 
> --


> 
> [Severity: Medium]
> The global rtnl_mutex is now held across the whole rx_ops
> unregistration loop, and userspace decides how long that loop is. Is
> that intended?
> 
> bcm_rx_setup() allocates one op per distinct (can_id, ifindex) with
> kzalloc(OPSIZ, GFP_KERNEL). That allocation is not memcg accounted, and
> there is no per-socket limit. bcm_connect() accepts can_ifindex == 0
> without any device, and ANYDEV ops register on the all-devices list
> through:
> 
> 	err = can_rx_register(sock_net(sk), NULL, op->can_id,
> 
> So an unprivileged user with no CAN hardware can create a very large
> number of ops.
> 
> On close(), each op calls can_rx_unregister(). That takes
> spin_lock_bh(&net->can.rcvlists_lock) and walks the receiver hlist
> linearly until it finds the match.
> 
> A second socket can put M newer entries in front of the victim's
> entries in the same bucket. It can do this with EFF ids that fold to
> the same bucket in effhash(), or with the same SFF id. RTNL would then
> be held for roughly K * M list steps.
> 
> Before this patch, the same loop ran only under lock_sock(sk), so only
> the closing task paid for it. Now it would block netlink and netdev
> configuration in every network namespace.
> 
> The loop without cond_resched() already existed, and the attacker needs
> a lot of kernel memory. What is new is that RTNL covers the whole loop.
> 

Yes. This is a potential misuse scenario, which is far away from common
usage patterns.

In v3 RTNL only covers the removal of the device specific filters. The
'any' CAN device filters live in the socket's namespace and are removed
after rtnl_unlock().

The need for RTNL when removing device specific filters comes from
af_can protecting the per-device receive lists with the per-netns
rcvlists_lock. This is not specific to bcm: raw_release() and
raw_setsockopt(CAN_RAW_FILTER) already hold RTNL across up to 512
filters per call. A per-device lock in struct can_dev_rcv_lists would
remove this dependency for raw, isotp and bcm. This is something for a
separate af_can rework in can-next.

Best regards,
Oliver

pw-bot: cr