Re: [PATCH net] net/smc: prevent device removal during connection setup

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Use-After-Free in smc_ibdev_init_put() racing with smc_ib_remove_dev()

Pre-existing issues:
- [High] Incomplete Fix / Use-After-Free for SMC-D (ISM) devices during connection setup
--

--- Patch [1]: [PATCH net] net/smc: prevent device removal during connection setup ---
commit 7f17c01034c751116f18a345f6abf2f1dee4b013
Author: Chengfeng Ye <[email protected]>
Subject: net/smc: prevent device removal during connection setup

This patch introduces a reference count mechanism to track in-flight
connection setups and prevent SMC-R devices from being removed prematurely.

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index b5db69073e20f..160271464305c 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
[ ... ]
> @@ -1596,14 +1596,14 @@ static int __smc_connect(struct smc_sock *smc)
>  	SMC_STAT_CLNT_SUCC_INC(sock_net(smc->clcsock->sk), aclc);
>  	smc_connect_ism_vlan_cleanup(ini);
>  	kfree(buf);
> -	kfree(ini);
> +	smc_init_info_free(ini);

[Severity: High]
This is a pre-existing issue, but while this patch addresses the use-after-free
for RoCE devices, does the same vulnerability still exist for SMC-D (ISM)
devices?

Looking at the connection setup path, an ISM device pointer is selected and
stored without taking a reference:

net/smc/af_smc.c:smc_find_ism_v2_device_clnt() {
    ...
    ini->ism_dev[i] = smcd;
    ...
}

If the ISM device is unregistered concurrently, smcd_unregister_dev() frees it
without waiting for in-flight connection setups:

net/smc/smc_ism.c:smcd_unregister_dev() {
    ...
    kfree(smcd);
}

When the connection setup resumes, the freed smcd pointer is dereferenced:

net/smc/smc_core.c:smc_conn_create() {
    ...
    lgr_list = ini->is_smcd ? &ini->ism_dev[ini->ism_selected]->lgr_list :
                              &smc_lgr_list.list;
    ...
}

Should a similar initialization reference count be added for SMC-D devices?

> diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> index 9bb495707445e..01c9b5c62df76 100644
> --- a/net/smc/smc_ib.c
> +++ b/net/smc/smc_ib.c
[ ... ]
> @@ -1000,6 +1002,9 @@ static void smc_ib_remove_dev(struct ib_device *ibdev, void *client_data)
>  	mutex_unlock(&smc_ib_devices.mutex);
>  	pr_warn_ratelimited("smc: removing ib device %s\n",
>  			    smcibdev->ibdev->name);
> +	if (atomic_read(&smcibdev->init_cnt))
> +		wait_event(smcibdev->init_wait,
> +			   !atomic_read(&smcibdev->init_cnt));
>  	smc_smcr_terminate_all(smcibdev);
>  	smc_ib_cleanup_per_ibdev(smcibdev);
>  	ib_unregister_event_handler(&smcibdev->event_handler);
> diff --git a/net/smc/smc_ib.h b/net/smc/smc_ib.h
> index ef8ac2b7546df..8e9fd5a200a2b 100644
> --- a/net/smc/smc_ib.h
> +++ b/net/smc/smc_ib.h
[ ... ]
> @@ -59,6 +61,17 @@ struct smc_ib_device {				/* ib-device infos for smc */
>  	int			ndev_ifidx[SMC_MAX_PORTS]; /* ndev if indexes */
>  };
>  
> +static inline void smc_ibdev_init_get(struct smc_ib_device *smcibdev)
> +{
> +	atomic_inc(&smcibdev->init_cnt);
> +}
> +
> +static inline void smc_ibdev_init_put(struct smc_ib_device *smcibdev)
> +{
> +	if (atomic_dec_and_test(&smcibdev->init_cnt))
> +		wake_up(&smcibdev->init_wait);
> +}
> +

[Severity: High]
Can this race with device removal and cause a use-after-free of smcibdev?

If Thread A executes smc_ibdev_init_put() and drops the count to 0 via
atomic_dec_and_test(), it might be preempted before calling wake_up():

Thread A:
    if (atomic_dec_and_test(&smcibdev->init_cnt))
        <preempted>

Thread B then executes smc_ib_remove_dev(), sees the count is 0, skips
wait_event(), and frees the device:

Thread B (smc_ib_remove_dev):
    if (atomic_read(&smcibdev->init_cnt))
        wait_event(...) // skipped
    ...
    kfree(smcibdev);

When Thread A resumes, it will call wake_up() on the now freed smcibdev
pointer.

Could this be avoided by using standard completions or by moving the cleanup
logic into a release function when the count reaches zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1