Re: [PATCH net] net/smc: fix qentry overwrite for CONFIRM_LINK and ADD_LINK_CONT in smc_llc_event_handler()

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

Pre-existing issues:
- [High] Spurious messages stashed in `qentry` during the `SMC_LLC_FLOW_REQ_ADD_LINK` state are deterministically leaked when transitioning to `ADD_LINK` or stopping the flow.
- [High] Lockless TOCTOU race in `qentry` validation allows concurrent softirq and workqueue to overwrite the pointer and leak memory.
- [High] Late duplicate messages bypass the `!qentry` guard and are subsequently leaked by `smc_llc_flow_stop()`.
--

--- Patch [1]: [PATCH net] net/smc: fix qentry overwrite for CONFIRM_LINK and ADD_LINK_CONT in smc_llc_event_handler() ---
commit 708ba737c6e1123f6d2c6447d7060356d5db40c0
Author: Mahanta Jambigi <[email protected]>

net/smc: fix qentry overwrite for CONFIRM_LINK and ADD_LINK_CONT in smc_llc_event_handler()

This commit fixes a memory leak in the SMC LLC event handler by adding a
guard to check if flow->qentry is already set before storing a new message.
It targets the CONFIRM_LINK and ADD_LINK_CONT messages to prevent duplicate
messages from overwriting an existing qentry allocation.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 954b2ff1815c2..aa6d83af55eda 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1927,7 +1927,8 @@ static void smc_llc_event_handler(struct smc_llc_qentry *qentry)
>  		return;
>  	case SMC_LLC_CONFIRM_LINK:
>  	case SMC_LLC_ADD_LINK_CONT:
> -		if (lgr->llc_flow_lcl.type != SMC_LLC_FLOW_NONE) {
> +		if (lgr->llc_flow_lcl.type != SMC_LLC_FLOW_NONE &&
> +		    !lgr->llc_flow_lcl.qentry) {

[Severity: High]
This isn't a bug introduced by this patch, but can spurious messages stashed
during the SMC_LLC_FLOW_REQ_ADD_LINK state still leak?

If a peer sends an unexpected CONFIRM_LINK message while the client is in the
REQ_ADD_LINK state, the event handler stashes it here because the type is not
NONE and qentry is NULL. When the legitimate ADD_LINK request arrives,
smc_llc_event_handler() processes it and overwrites this stashed qentry via
smc_llc_flow_qentry_set() without freeing the previous one.

Alternatively, if an ADD_LINK response arrives, it calls smc_llc_flow_stop()
directly, which does a memset on the flow:

smc_llc_flow_stop() {
    ...
    memset(flow, 0, sizeof(*flow));
    ...
}

Could this sequence leak the kmalloc-96 allocation?

[Severity: High]
This is a pre-existing issue, but does this lockless check introduce a TOCTOU
race if a duplicate packet arrives concurrently?

If a peer sends two duplicate CONFIRM_LINK messages concurrently, one with
the RESP flag clear (processed via smc_llc_event_handler in a workqueue)
and one with the RESP flag set (processed via smc_llc_rx_response in
softirq context), both contexts could locklessly evaluate !flow->qentry
as true.

They would then both call smc_llc_flow_qentry_set(), overwriting one of the
pointers and leaking the allocation.

[Severity: High]
This is a pre-existing issue, but does this guard fail to protect against
late duplicate messages?

When a legitimate CONFIRM_LINK is processed, the flow handler (for example,
smc_llc_cli_conf_link) calls smc_llc_flow_qentry_del(), which resets
flow->qentry to NULL and frees it. However, the flow->type remains active
while remaining hardware setup is performed.

If a duplicate CONFIRM_LINK arrives during this window, this check sees
type != NONE and !qentry, and erroneously stashes the duplicate. When the
flow eventually completes, smc_llc_flow_stop() unconditionally clears the
flow via memset(), leaking the newly stashed late-duplicate qentry.

>  			/* a flow is waiting for this message */
>  			smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);
>  			wake_up(&lgr->llc_msg_waiter);

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