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

[Manivannan Sadhasivam]

On Tue, Apr 07, 2026 at 05:33:55PM +0200, Paolo Abeni wrote:
> 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?
> ---
> 

This is a legitimate concern. I believe adding synchronize_net() before
destroy_workqueue() will ensure that all the RX packets are flushed before
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.
> 

There are a couple of worth fixing issues mentioned there in the error path that
I'll incorporate in the next revision. But for the issues not related to this
series, I will defer them to follow up series.

- Mani

-- 
மணிவண்ணன் சதாசிவம்