Re: [PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free and Out-of-Bounds Read in `smc_llc_srv_add_link()` when accessing the `add_llc` payload.
- [High] `smc_llc_flow_stop()` overwrites `flow->qentry` with `memset`, unconditionally leaking any stashed LLC message.
- [High] Unsynchronized access to `lgr->llc_flow_lcl` across workqueue and tasklet contexts causes data races, leading to memory leaks and NULL pointer dereferences.
--

--- Patch [1]: [PATCH net] net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition ---
commit 5dc4ee5e350906dce47b3e2165e175388d750d51
Author: Mahanta Jambigi <mjambigi@linux.ibm.com>
Subject: net/smc: free stashed qentry before overwrite in REQ_ADD_LINK to ADD_LINK transition

When an ADD_LINK request arrives while the flow is in REQ_ADD_LINK state,
the event handler overwrites flow->qentry without freeing any stashed entry.
This leak is fixed by unconditionally calling smc_llc_flow_qentry_del()
before overwriting the qentry pointer.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index aa6d83af55eda..c7f0222aff8d0 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
[ ... ]
> @@ -1911,6 +1911,8 @@ static void smc_llc_event_handler(struct smc_llc_qentry *qentry)
>  			if (lgr->llc_flow_lcl.type ==
>  					SMC_LLC_FLOW_REQ_ADD_LINK) {
>  				/* server started add_link processing */
> +				/* free any qentry stashed in REQ_ADD_LINK state */
> +				smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this unsynchronized
access to lgr->llc_flow_lcl create a data race between workqueue and tasklet
contexts?

In smc_llc_event_handler() (workqueue context), we modify the flow type and
qentry without holding lgr->llc_flow_lock:

	smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
	lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
	smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);

If a softirq preempts this immediately after we zero qentry and set the type,
smc_llc_rx_response() could locklessly read the type and assign a new response
to the flow:

smc_llc_rx_response() {
    ...
    /* assign responses to the local flow, we requested them */
    smc_llc_flow_qentry_set(&link->lgr->llc_flow_lcl, qentry);
    ...
}

When the workqueue resumes, it would unconditionally overwrite qentry with its
own message, leaking the softirq's qentry. Also, if the type is changed
concurrently, smc_llc_add_link_work() might falsely trigger
smc_llc_flow_stop() mid-flow, zeroing the state and leading to a subsequent
NULL pointer dereference. Could this be prevented by holding the appropriate
locks?

>  				lgr->llc_flow_lcl.type = SMC_LLC_FLOW_ADD_LINK;
>  				smc_llc_flow_qentry_set(&lgr->llc_flow_lcl,
>  							qentry);

[Severity: High]
This is a pre-existing issue, but while we are looking at memory leaks
related to flow->qentry, does smc_llc_flow_stop() also unconditionally leak
any stashed message?

If an incoming message is stashed into lgr->llc_flow_lcl.qentry while the link
group is in the SMC_LLC_FLOW_REQ_ADD_LINK state, and then the remote peer
sends an SMC_LLC_REQ_ADD_LINK response, smc_llc_event_handler() handles it by
calling smc_llc_flow_stop():

smc_llc_event_handler() {
    ...
    case SMC_LLC_REQ_ADD_LINK:
        if (lgr->role == SMC_CLNT &&
            lgr->llc_flow_lcl.type == SMC_LLC_FLOW_REQ_ADD_LINK &&
            (llc->raw.hdr.flags & SMC_LLC_FLAG_RESP)) {
            smc_llc_flow_stop(link->lgr, &lgr->llc_flow_lcl);
    ...
}

smc_llc_flow_stop() then uses memset to clear the flow state:

smc_llc_flow_stop() {
    ...
    memset(flow, 0, sizeof(*flow));
    flow->type = SMC_LLC_FLOW_NONE;
    ...
}

This zero-initializes the struct, silently wiping the flow->qentry pointer
without freeing its dynamically allocated memory. Could this allow a remote
peer to cause memory exhaustion by sending a CONFIRM_LINK message followed
immediately by a REQ_ADD_LINK response?

[Severity: Critical]
This is another pre-existing issue, but does smc_llc_srv_add_link() have a
use-after-free and out-of-bounds read when accessing the add_llc payload?

In smc_llc_srv_add_link(), qentry is retrieved and explicitly freed via
smc_llc_flow_qentry_del():

smc_llc_srv_add_link() {
    ...
    smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
    ...
}

Later in the same function, if the connection uses SMC_V2 and the rxbuf is
not shared, add_llc is used as the llc_msg argument:

smc_llc_srv_add_link() {
    ...
    u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
        (u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
    smc_llc_save_add_link_rkeys(link, link_new, llc_msg);
    ...
}

Since add_llc is a pointer inside the previously freed qentry, doesn't this
result in reading from a freed memory allocation? Also, because qentry is
allocated with a fixed size of sizeof(*qentry) and does not hold the extended
V2 rkeys, wouldn't this also cause a heap out-of-bounds read past the slab
chunk boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818073107.466506-1-mjambigi@linux.ibm.com?part=1