Re: rds_wq UAF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] rds: ib: skipping flush_workqueue() in rds_ib_init() causes UAF on error
- [Critical] rds: rdma: rds_rdma_init() still accesses invalid rds_wq when rds_init() fails
--

commit 1ca141aafc7f21d383b77362aeece7748105d3b9
Author: Oleg Nesterov <oleg@redhat.com>

rds_wq UAF

This commit replaces rds_ib_unregister_client() with
ib_unregister_client(&rds_ib_client) on the rds_ib_init() error path to avoid
flushing a NULL or already-destroyed rds_wq.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 786f39169bc14..cde2d1bfabbd1 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -619,7 +619,7 @@ int rds_ib_init(void)
>  out_sysctl:
>  	rds_ib_sysctl_exit();
>  out_ibreg:
> -	rds_ib_unregister_client();
> +	ib_unregister_client(&rds_ib_client);

[Severity: Critical]
Could skipping flush_workqueue(rds_wq) here cause a use-after-free of
rds_ib_mr_wq or the rds_rdma module?

In rds_ib_init(), ib_register_client(&rds_ib_client) synchronously calls
rds_ib_add_one() for every registered InfiniBand device.

If rds_ib_add_one() fails after allocating rds_ibdev (for example at
ib_alloc_pd() or rds_ib_create_mr_pool()), its put_dev path drops the
reference count to zero and queues rds_ib_dev_free() onto rds_wq:

net/rds/ib.c:rds_ib_dev_put()
	if (refcount_dec_and_test(&rds_ibdev->refcount))
		queue_work(rds_wq, &rds_ibdev->free_work);

Similarly, if rds_ib_add_one() succeeds for one or more devices and a later
step in rds_ib_init() fails (rds_ib_sysctl_init() or rds_ib_recv_init()),
ib_unregister_client(&rds_ib_client) calls rds_ib_remove_one() for each added
device, which also drops the final reference and queues rds_ibdev->free_work
(rds_ib_dev_free()) onto rds_wq.

Commit 24fa163a4bae74 added flush_workqueue(rds_wq) in
rds_ib_unregister_client() specifically to wait for rds_ib_dev_free() to
complete:

static void rds_ib_unregister_client(void)
{
	ib_unregister_client(&rds_ib_client);
	/* wait for rds_ib_dev_free() to complete */
	flush_workqueue(rds_wq);
}

Without that flush, rds_ib_init() proceeds directly to out_mr_exit and calls
rds_ib_mr_exit() -> destroy_workqueue(rds_ib_mr_wq) while rds_ib_dev_free()
is still pending or running on rds_wq and about to tear down
rds_ibdev->mr_8k_pool and rds_ibdev->mr_1m_pool (which flushes and cancels
delayed work on rds_ib_mr_wq).

In addition, when CONFIG_RDS_RDMA=m, returning from rds_ib_init() causes the
module loader to unmap rds_rdma.ko while rds_ib_dev_free() is still queued or
executing on rds_wq.

[Severity: Critical]
Also, if rds_init() has failed and destroyed rds_wq (or has not run yet so
rds_wq is NULL), doesn't rds_rdma_init() still access the invalid rds_wq even
with this change?

1. During rds_ib_init(), if an IB device is present and either
   rds_ib_add_one() fails at put_dev or ib_unregister_client(&rds_ib_client)
   calls rds_ib_remove_one(), rds_ib_dev_put() will still call
   queue_work(rds_wq, &rds_ibdev->free_work) on the destroyed or NULL rds_wq.

2. In rds_rdma_init(), if rds_ib_init() succeeds and rds_rdma_listen_init()
   fails, rds_ib_exit() is called, which still calls
   rds_ib_unregister_client() -> flush_workqueue(rds_wq):

net/rds/rdma_transport.c:rds_rdma_init()
	ret = rds_ib_init();
	if (ret)
		goto out;

	ret = rds_rdma_listen_init();
	if (ret)
		rds_ib_exit();

3. If rds_rdma_init() (or rds_tcp_init()) succeeds after rds_init() has
   failed and torn down RDS core resources in rds_threads_exit(), the
   transport remains registered and active callbacks will queue work onto the
   destroyed rds_wq.

>  out_mr_exit:
>  	rds_ib_mr_exit();
>  out:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/atdoGiW_sHlWgOwt@redhat.com?part=1