Re: [PATCH v2 5/5] net: qrtr: ns: Fix use-after-free in driver remove()

[Paolo Abeni]

On 4/3/26 6:06 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> 
> In the remove callback, if a packet arrives after destroy_workqueue() is
> called, but before sock_release(), the qrtr_ns_data_ready() callback will
> try to queue the work, causing use-after-free issue.
> 
> Fix this issue by saving the default 'sk_data_ready' callback during
> qrtr_ns_init() and use it to replace the qrtr_ns_data_ready() callback at
> the start of remove(). This ensures that even if a packet arrives after
> destroy_workqueue(), the work struct will not be dereferenced.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> ---
>  net/qrtr/ns.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index dfb5dad9473c..c62d79e03d64 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -25,6 +25,7 @@ static struct {
>  	u32 lookup_count;
>  	struct workqueue_struct *workqueue;
>  	struct work_struct work;
> +	void (*saved_data_ready)(struct sock *sk);
>  	int local_node;
>  } qrtr_ns;
>  
> @@ -754,6 +755,7 @@ int qrtr_ns_init(void)
>  		goto err_sock;
>  	}
>  
> +	qrtr_ns.saved_data_ready = qrtr_ns.sock->sk->sk_data_ready;
>  	qrtr_ns.sock->sk->sk_data_ready = qrtr_ns_data_ready;
>  
>  	sq.sq_port = QRTR_PORT_CTRL;
> @@ -803,6 +805,10 @@ EXPORT_SYMBOL_GPL(qrtr_ns_init);
>  
>  void qrtr_ns_remove(void)
>  {
> +	write_lock_bh(&qrtr_ns.sock->sk->sk_callback_lock);
> +	qrtr_ns.sock->sk->sk_data_ready = qrtr_ns.saved_data_ready;
> +	write_unlock_bh(&qrtr_ns.sock->sk->sk_callback_lock);

Sashiko says:

---
Does this lock adequately protect against concurrent callback execution?
In the network receive path, __sock_queue_rcv_skb() typically evaluates
!sock_flag(sk, SOCK_DEAD) and invokes sk->sk_data_ready() locklessly,
without acquiring sk_callback_lock or being in an RCU read-side
critical section.
If a thread processing a packet fetches the qrtr_ns_data_ready pointer
and is preempted, could it resume and execute the callback after
qrtr_ns_remove() has already finished destroying the workqueue?
---

There are more remarks from sashiko:

https://sashiko.dev/#/patchset/20260403-qrtr-fix-v2-0-f88a14859c63%40oss.qualcomm.com

AFAICS they are pre-existing issues or false positive, but please have a
look.

/P