Re: [PATCH 5.10] net/smc: fix potential panic dues to unprotected smc_llc_srv_add_link()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `llc_conf_mutex` lock around `smc_llc_cli_add_link()` on the client side.
- [High] Missing cancelation of `link_down_wrk` before clearing or freeing the link memory causes workqueue corruption and Use-After-Free.
--

commit 95b1079562e96ef6213155f37083256de0a0c01b
Author: D. Wythe <alibuda@linux.alibaba.com>

net/smc: fix potential panic dues to unprotected smc_llc_srv_add_link()

This commit fixes a potential panic when an SMC server creates a second
link. It adds llc_conf_mutex locking around smc_llc_srv_add_link() to
protect the link group from concurrent modifications.

Link: https://nvd.nist.gov/vuln/detail/CVE-2023-54237

> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 5dd4faaf7d6e5..fe6fb719a9a5f 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1268,8 +1268,10 @@ static int smcr_serv_conf_first_link(struct smc_sock *smc)
>  	smc_llc_link_active(link);
>  	smcr_lgr_set_type(link->lgr, SMC_LGR_SINGLE);
>  
> +	mutex_lock(&link->lgr->llc_conf_mutex);
>  	/* initial contact - try to establish second link */
>  	smc_llc_srv_add_link(link);
> +	mutex_unlock(&link->lgr->llc_conf_mutex);

[Severity: High]
This isn't a bug introduced by this patch, but does the client side need
this same locking?

Looking at smcr_clnt_conf_first_link() in the same file:

net/smc/af_smc.c:smcr_clnt_conf_first_link() {
    ...
        smc_llc_flow_qentry_clr(&link->lgr->llc_flow_lcl);
        smc_llc_cli_add_link(link, qentry);
        return 0;
}

It appears smc_llc_cli_add_link() modifies the link array without holding
llc_conf_mutex. Could this lead to the same panic if an asynchronous event
like smc_link_down_work() runs concurrently and operates on a partially
initialized link array?

>  	return 0;
>  }

[Severity: High]
This is another pre-existing issue, but I am wondering about the
cancelation of link_down_wrk when clearing a link.

In net/smc/smc_core.c:smcr_link_clear():

net/smc/smc_core.c:smcr_link_clear() {
    ...
        smcibdev = lnk->smcibdev;
        memset(lnk, 0, sizeof(struct smc_link));
        lnk->state = SMC_LNK_UNUSED;
    ...
}

If a device error triggers smcr_link_down_cond_sched() to queue
lnk->link_down_wrk, and the link group is terminated before the work
executes, could the memset() corrupt the workqueue by zeroing out its
linked list pointers?

If the link group is subsequently freed via smc_lgr_free(), could the
pending work then execute on freed memory? Should there be a
cancel_work_sync() for link_down_wrk before clearing the link memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901090545.10419-1-arefev@swemel.ru?part=1