Re: [PATCH net v2] packet: avoid re-registering fanout hook after device unregister

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

David Lee wrote:
> packet_set_ring() temporarily detaches a socket from packet delivery while
> reconfiguring its ring. It records the previous running state, clears
> po->num, unregisters the protocol hook when needed, drops po->bind_lock,
> and later restores po->num and re-registers the hook from the saved
> was_running value.
> 
> That unlocked window can race with NETDEV_UNREGISTER. The notifier can
> observe the socket as not running, skip __unregister_prot_hook(), and
> invalidate the per-socket binding by setting po->ifindex to -1 and clearing
> po->prot_hook.dev. A one-member fanout group can still retain its shared
> fanout hook device pointer. When packet_set_ring() resumes, re-registering
> solely from the stale was_running state can re-add the fanout hook after
> the device has been unregistered.
> 
> Treat po->ifindex == -1 as an invalidated binding after reacquiring
> po->bind_lock. Restore po->num as before, but do not re-register the hook
> if device unregister already detached the socket.
> 
> Fixes: dc99f600698d ("packet: Add fanout support.")
> Reported-by: David Lee <david.lee@trailofbits.com>
> Signed-off-by: David Lee <david.lee@trailofbits.com>

My suggestion of a Reported-by was becaused the Signed-off-by was from
a different author. Both tags from the same author is generally
discouraged.

Perhaps also add a Link to the discussion in the first patch, which
adds some more rationale. The crux to me is that ifindex -1 is not
just the default for detached sockets, it is a special invalidated
condition.

Link: https://lore.kernel.org/netdev/20260701113947.23180-1-david.lee@trailofbits.com/

With those asides

Reviewed-by: Willem de Bruijn <willemb@google.com>


> Assisted-by: Codex:gpt-5
> ---
> Changes in v2:
> - Add Fixes and Reported-by tags.
> - Fix the incorrect Signed-off-by tag from v1.
> 
> Trail of Bits has a PoC that achieves local privilege escalation using this
> bug on a custom kernel config with CONFIG_LIST_HARDENED disabled, which can
> be shared further if needed.
> 
>  net/packet/af_packet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4561,7 +4561,11 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>  
>  	spin_lock(&po->bind_lock);
>  	WRITE_ONCE(po->num, num);
> -	if (was_running)
> +	/*
> +	 * NETDEV_UNREGISTER may have invalidated the binding while bind_lock
> +	 * was dropped above.  Do not re-add a fanout hook to a dead device.
> +	 */
> +	if (was_running && READ_ONCE(po->ifindex) != -1)
>  		register_prot_hook(sk);
>  
>  	spin_unlock(&po->bind_lock);
> -- 
> 2.43.0