Re: [PATCH net] net/packet: avoid fanout hook re-registration after 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.

Thanks for the report with fix.

> 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.

I guess key here is that po->ifindex == -1 is not the normal device
unbound state. That would be po->ifindex 0, and those sockets do need
to be restored.

So this LGTM.

The bug here is having a stale fanout->prot_hook->dev.
If this is the only socket in a fanout group then
__unregister_prot_hook calls __fanout_unlink calls
__dev_remove_pack(&f->prot_hook).

Then later __register_prot_hook calls __fanout_link and
dev_add_pack(&f->prot_hook), rather than checking po->prot_hook.dev,
which packet_notifier modified.

An alternative would be for __fanout_link to check

@@ -1518,7 +1518,8 @@ static void __fanout_link(struct sock *sk, struct packet_sock *po)
        rcu_assign_pointer(f->arr[f->num_members], sk);
        smp_wmb();
        f->num_members++;
-       if (f->num_members == 1)
+       if (f->num_members == 1 &&
+           f->prot_hook.dev == po->prot_hook.dev)
                dev_add_pack(&f->prot_hook);
        spin_unlock(&f->lock);
 }

This condition is true when the group is created and checked on every
socket joining the group.

Or even simpler

+       if (f->num_members == 1 && READ_ONCE(po->ifindex) != -1)

But as said the patch as shared should work too.

Patches to net need a Fixes tag:

Fixes: dc99f600698d ("packet: Add fanout support.")
 
> Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.czarnota@trailofbits.com>

I'm not entirely sure what the policy on nicknames is. But at best it
is uncommon. Consider dropping.

> Assisted-by: Codex:gpt-5
> ---
> Bug found and triaged by David Lee from Trail of Bits.

Instead, a Reported-by tag?

> 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
> index 8e6f3a734ba0..000000000000 100644
> --- 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
x