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

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

[Mahanta Jambigi]

The SMC_LLC_CONFIRM_LINK / SMC_LLC_ADD_LINK_CONT branch in
smc_llc_event_handler() stores an incoming qentry into the local LLC flow
without first checking whether a qentry is already pending. If a malicious or
buggy peer sends a second CONFIRM_LINK or ADD_LINK_CONT request while a flow is
active and flow->qentry is already set, smc_llc_flow_qentry_set() overwrites the
pointer without freeing the previous allocation, leaking one kmalloc-96 object
per spurious message.

The sibling SMC_LLC_DELETE_LINK branch already has the correct !flow->qentry
guard. Apply the same guard to the CONFIRM_LINK/ADD_LINK_CONT branch so that a
duplicate message when qentry is already occupied falls through to break and is
freed by the kfree(qentry) at the out: label, rather than silently leaking the
existing allocation.

The response direction (smc_llc_rx_response()) is unaffected: it already guards
with flow->qentry at the equivalent site and drops duplicate responses
correctly.

Fixes: 0fb0b02bd6fd ("net/smc: adapt SMC client code to use the LLC flow")
Signed-off-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Reviewed-by: Hidayath Khan <hidayath@linux.ibm.com>
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
---
 net/smc/smc_llc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
index 954b2ff1815c..aa6d83af55ed 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) {
 			/* a flow is waiting for this message */
 			smc_llc_flow_qentry_set(&lgr->llc_flow_lcl, qentry);
 			wake_up(&lgr->llc_msg_waiter);
-- 
2.53.0

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

[sashiko-bot]

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 <mjambigi@linux.ibm.com>

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/20260729130153.970800-1-mjambigi@linux.ibm.com?part=1