Re: [PATCH net] appletalk: Hold socket reference in atalk_rcv()

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
appletalk: Hold socket reference in atalk_rcv()

This commit addresses a use-after-free race by holding a reference to the
socket found by atalk_search_socket() until after the packet is queued in
atalk_rcv().

> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 30a6dc06291c3..61ec5c569dc3c 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c

[ ... ]

> @@ -1474,9 +1476,12 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
>  		goto drop;
>  
>  	/* Queue packet (standard) */
> -	if (sock_queue_rcv_skb(sock, skb) < 0)
> +	if (sock_queue_rcv_skb(sock, skb) < 0) {
> +		sock_put(sock);
>  		goto drop;
> +	}

[Severity: High]
Does this code introduce a memory leak or an infinite timer loop?

By holding a socket reference in atalk_rcv(), the socket memory is kept alive,
but atalk_rcv() does not hold the socket lock. A concurrent atalk_release()
could lock the socket, mark it SOCK_DEAD, and call atalk_destroy_socket(),
which logically purges the receive queue:

net/appletalk/ddp.c:atalk_destroy_socket() {
    ...
    skb_queue_purge(&sk->sk_receive_queue);
    ...
}

After this purge, atalk_rcv() might call sock_queue_rcv_skb(). The core
function __sock_queue_rcv_skb() does not reject packets for SOCK_DEAD sockets.
It will successfully enqueue the skb and increment sk->sk_rmem_alloc.

Since AppleTalk uses the empty sock_def_destruct() instead of a custom
sk_destruct callback, is the receive queue ever purged upon final sk_free(),
or does this permanently leak the skb?

Furthermore, if atalk_destroy_socket() finds other pending allocations and
arms the destruction timer, will the timer endlessly re-arm itself?

net/appletalk/ddp.c:atalk_destroy_timer() {
    ...
    if (sk_has_allocations(sk)) {
        sk->sk_timer.expires = jiffies + SOCK_DESTROY_TIME;
        add_timer(&sk->sk_timer);
    }
    ...
}

The timer checks sk_has_allocations(sk), which will remain true indefinitely
due to the uncleared sk_rmem_alloc from the leaked skb.

>  
> +	sock_put(sock);
>  	return NET_RX_SUCCESS;