Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Bryam Vargas]

Hidayath,

> I have a standalone net-next patch that aborts the connection when
> bytes_to_rcv + diff_prod exceeds rmb_desc->len.  The check sits before
> the atomic_add(), so the accumulator is never written with an out-of-range
> value

That covers the follow-up I said I would send, and the placement is better than
what I described: I had said a check after the atomic_add, which only notices
the counter is already out of range. Yours doesn't let it get there. Consider my
follow-up withdrawn -- I am not sending a competing patch.

If it is useful for the Fixes decision: I ran the wrap++/count==0 vector on the
real SMC-D path under KASAN while working on the cursor series. With only the
per-cursor bound applied, bytes_to_rcv reaches 6*len and smc_rx_recvmsg() trips
slab-out-of-bounds on a read of 5*len; each CDC advances exactly len, so
diff == len and an advance-bound does not fire -- it's the accumulation that
overruns, which is what your check catches. Logs on request if you want them in
the commit message.

Two heads-up on collisions, since both are in flight this week rather than
merged:

smc_cdc_msg_recv_action() is also touched by "net/smc: order the CDC receive
path against buffer publication" (v4, 20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me),
which hoists sndbuf_desc to the top of the function and gates the tx-trigger on
it. Your hunk sits just above that gate, so whichever lands second will want a
look rather than a blind rebase. I'd rather flag it now than after a conflict.

And you mentioned running the abort_work cancel for both transports in v2 --
that edits smc_conn_free()'s SMC-D branch, which "net/smc: unregister the
connection before draining the rx tasklet"
(20260808-b4-disp-22f119e6-v2-1-61647601a6f3@proton.me) also rewrites: it drops
the !list_empty guard around smc_ism_unset_conn(), moves the drain ahead of the
detach, and clears conn->sndbuf_desc before freeing it. Same branch, same week.

On the shared bitfield -- agreed it needs a layout change rather than something
folded into a fix, and it's yours; I'd noted it and left it alone for the
same reason.

Thanks,
Bryam

[PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Hidayath Khan]

smc_cdc_msg_recv_action() accumulates the peer's producer cursor advance
into conn->bytes_to_rcv:

        atomic_add(diff_prod, &conn->bytes_to_rcv);
        /* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

Nothing enforces that comment.  A conforming peer cannot advance its
producer cursor past our consumer cursor, so a peer that does fills the
accumulator without bound.

Bounding the cursor does not help: a wrap increment with count 0 is a legal
advance of exactly one bufferful, so every per-cursor check accepts it
while smc_curs_diff() returns size for each such message.

The copy-length clamps bound the copy but never repair the counter, so
SIOCINQ reports a readable length that does not exist and
smc_rx_data_available() keeps poll() readable forever with nothing for
recvmsg() to return.

Drop the connection instead, reusing the abort smc_cdc_msg_validate()
performs for an out-of-range sequence number.  The test precedes the
atomic_add() so the counter is never left inflated; abandoning the rest of
the message matches smc_cdc_msg_validate(), and abort_work tears the
connection down.

The test is written as a subtraction.  diff_prod is not bounded by the RMB:
smc_curs_diff() documents "difference cannot exceed size" as an assumption
about its inputs, but the peer controls the cursor it is computed from, so
it can return up to INT_MAX.  Adding that to bytes_to_rcv would overflow
and the comparison would then read as false, defeating the check in exactly
the case it exists for.  Subtracting cannot overflow, since bytes_to_rcv is
in [0, rmb_desc->len].

Both transports are affected: the accumulator is shared.

Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_cdc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..d8f747bbc6f6 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
 	sk_send_sigurg(&smc->sk);
 }
 
+/* Drop a connection whose peer violated the CDC protocol.  @link is the link
+ * the offending message arrived on, or NULL when it is not known to the caller
+ * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
+ * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
+ * no per-link QP to abort on.
+ */
+static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
+{
+	struct smc_connection *conn = &smc->conn;
+
+	conn->out_of_sync = 1;  /* prevent any further receives */
+	spin_lock_bh(&conn->send_lock);
+	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
+	if (link)
+		conn->lnk = link;
+	spin_unlock_bh(&conn->send_lock);
+	sock_hold(&smc->sk); /* sock_put in abort_work */
+	if (!queue_work(smc_close_wq, &conn->abort_work))
+		sock_put(&smc->sk);
+}
+
 static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 				 struct smc_link *link)
 {
@@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 
 	/* check that seqnum was seen before */
 	diff = conn->local_rx_ctrl.seqno - recv_seq;
-	if (diff < 0) { /* diff larger than 0x7fff */
-		/* drop connection */
-		conn->out_of_sync = 1;	/* prevent any further receives */
-		spin_lock_bh(&conn->send_lock);
-		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
-		conn->lnk = link;
-		spin_unlock_bh(&conn->send_lock);
-		sock_hold(&smc->sk); /* sock_put in abort_work */
-		if (!queue_work(smc_close_wq, &conn->abort_work))
-			sock_put(&smc->sk);
-	}
+	if (diff < 0) /* diff larger than 0x7fff */
+		smc_cdc_abort_conn(smc, link);  /* drop connection */
 }
 
 static void smc_cdc_msg_recv_action(struct smc_sock *smc,
@@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
 				  &conn->local_rx_ctrl.prod);
 	if (diff_prod) {
+		if (diff_prod > conn->rmb_desc->len -
+				atomic_read(&conn->bytes_to_rcv)) {
+			smc_cdc_abort_conn(smc, NULL);
+			return;
+		}
 		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
 			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);
 		/* bytes_to_rcv is decreased in smc_recvmsg */

base-commit: cd138abf911f7a6b19656e446b09cff4453d1f3d
-- 
2.52.0

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[sashiko-bot]

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

New issues:
- [High] SMC-D ignores the `out_of_sync` flag, allowing further malicious or corrupted packets to be processed after an abort.

Pre-existing issues:
- [Critical] The patch adds a bounds check for `diff_prod` but fails to apply the same bounds check for the peer's consumer cursor (`diff_tx` and `diff_cons`), leading to a critical heap buffer overflow in `smc_tx_sendmsg()`.
- [Critical] Integer overflow in `smc_curs_diff()` bypasses the newly introduced bounds check for `diff_prod`, leading to a remote kernel panic via out-of-bounds read.
- [High] Unprotected concurrent modifications to the `out_of_sync`, `killed`, and `freed` bitfields lead to torn writes and lost state updates.
--

--- Patch [1]: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB ---
commit f23554c53b371c00958c1ef783733772b67bb8fe
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: abort the connection when the peer overruns the RMB

This patch addresses an issue where a non-compliant peer could inflate the 
bytes_to_rcv accumulator without bound. It adds a bounds check on the 
producer cursor advance and drops the connection if an overrun is detected.

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..d8f747bbc6f69 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
>  	sk_send_sigurg(&smc->sk);
>  }
>  
> +/* Drop a connection whose peer violated the CDC protocol.  @link is the link
> + * the offending message arrived on, or NULL when it is not known to the caller
> + * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
> + * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
> + * no per-link QP to abort on.
> + */
> +static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
> +{
> +	struct smc_connection *conn = &smc->conn;
> +
> +	conn->out_of_sync = 1;  /* prevent any further receives */

[Severity: High]
This is a pre-existing issue, but does setting this bitfield without locks
cause torn writes?

The bitfields killed, freed, and out_of_sync share the same byte in
struct smc_connection. This code sets conn->out_of_sync = 1 from softirq
context without holding locks, while smc_conn_kill() sets conn->killed = 1
from process context (holding lock_sock).

Could this read-modify-write interleaving result in a lost update for killed
or out_of_sync, leaving the connection incompletely terminated?

[Severity: High]
Can SMC-D connections continue processing packets after this flag is set?

The patch relies on setting conn->out_of_sync = 1 here to stop further
receives immediately. However, SMC-D's receive tasklet (smcd_cdc_rx_tsklet)
only checks conn->killed, not conn->out_of_sync:

net/smc/smc_cdc.c:smcd_cdc_rx_tsklet() {
    ...
    if (!conn || conn->killed)
        return;
    ...
}

Because abort_work runs asynchronously to actually kill the connection, is
there a race window where SMC-D will continue passing packets to
smc_cdc_msg_recv_action(), bypassing this protection entirely?

> +	spin_lock_bh(&conn->send_lock);
> +	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
> +	if (link)
> +		conn->lnk = link;
> +	spin_unlock_bh(&conn->send_lock);
> +	sock_hold(&smc->sk); /* sock_put in abort_work */
> +	if (!queue_work(smc_close_wq, &conn->abort_work))
> +		sock_put(&smc->sk);
> +}

[ ... ]

> @@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
>  				  &conn->local_rx_ctrl.prod);
>  	if (diff_prod) {
> +		if (diff_prod > conn->rmb_desc->len -
> +				atomic_read(&conn->bytes_to_rcv)) {

[Severity: Critical]
This is a pre-existing issue, but can an integer overflow in smc_curs_diff()
bypass this new bounds check?

smc_curs_diff() computes the difference using 32-bit unsigned arithmetic:
(size - old->count) + new->count.

If a malicious peer provides an extremely large new->count via CDC messages
(e.g., 0xFFFF0001 when size is 65536 and old->count is 0), the arithmetic
wraps to produce a small positive integer (e.g., 1), which trivially passes
this check.

This out-of-bounds cursor is later used as an array index in
smc_cdc_handle_urg_data_arrival():

net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
    ...
    conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
    ...
}

Could this lead to an out-of-bounds read and remote kernel panic?

[Severity: Critical]
This is a pre-existing issue, but does the peer's consumer cursor logic
earlier in this function also need bounds checking?

A malicious peer can provide an inflated cons cursor via CDC messages.
Earlier in smc_cdc_msg_recv_action(), diff_cons is calculated from this
unvalidated cursor and added directly to conn->sndbuf_space without bounds
checking in SMC-D nocopy mode:

net/smc/smc_cdc.c:smc_cdc_msg_recv_action() {
    ...
    diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
                              &conn->local_rx_ctrl.cons);
    ...
    atomic_add(diff_tx, &conn->sndbuf_space);
    ...
}

This inflated sndbuf_space is later used in smc_tx_sendmsg() to bound
copylen, which can drastically exceed the actual allocation size of
sndbuf_desc->len. The chunking logic then generates a massive size for the
second chunk, resulting in an out-of-bounds write:

net/smc/smc_tx.c:smc_tx_sendmsg() {
    ...
    chunk_len = copylen - chunk_len; /* remainder */
    ...
    rc = memcpy_from_msg(sndbuf_base + chunk_off, msg, chunk_len);
    ...
}

Could this lead to a heap buffer overflow?

> +			smc_cdc_abort_conn(smc, NULL);
> +			return;
> +		}
>  		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
>  			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);

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

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804141109.542202-1-hidayath@linux.ibm.com?part=1
---
--- Patch [1]: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB ---
commit fc7927463c41bac1e3a598538d104b46770ee787
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: abort the connection when the peer overruns the RMB

The patch factors the connection-abort sequence out of
smc_cdc_msg_validate() into a new helper smc_cdc_abort_conn(), and calls it
from smc_cdc_msg_recv_action() when the peer's producer cursor advance would
push conn->bytes_to_rcv past conn->rmb_desc->len.  The test is placed before
the atomic_add() and the rest of the message is abandoned.

A few questions about the new helper and its new caller below.

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5, old-prompts]
The commit message describes a peer-triggerable, user-visible defect
(SIOCINQ reporting a readable length that does not exist, poll() staying
readable with nothing for recvmsg() to return), but the patch carries
neither a Fixes: tag nor Cc: stable.

The accumulation site being repaired:

	atomic_add(diff_prod, &conn->bytes_to_rcv);
	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

is part of the original CDC receive implementation, so every stable release
carrying net/smc looks affected.  Could a Fixes: tag be added so backporters
know which releases to pick this up for?

Simon says: In a similar vein, if the above is so then perhaps
            the patch should be targeted at net rather than net-next?

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..d8f747bbc6f69 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
>  	sk_send_sigurg(&smc->sk);
>  }
>  
> +/* Drop a connection whose peer violated the CDC protocol.  @link is the link
> + * the offending message arrived on, or NULL when it is not known to the caller
> + * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
> + * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
> + * no per-link QP to abort on.
> + */
> +static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
> +{
> +	struct smc_connection *conn = &smc->conn;
> +
> +	conn->out_of_sync = 1;  /* prevent any further receives */

[Severity: Medium]
[Finding: main-2-6]
[Sources: opus-5, old-prompts]
The kerneldoc above states the link == NULL case covers SMC-D, and this
comment says the flag prevents any further receives.  Does
conn->out_of_sync actually stop receives on SMC-D?

The only reader of the flag is the SMC-R IB completion path:

net/smc/smc_cdc.c:smc_cdc_rx_handler() {
	...
	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
	if (!conn || conn->out_of_sync) {
		read_unlock_bh(&lgr->conns_lock);
		return;
	}
	...
}

The SMC-D entry point gates on a different flag:

net/smc/smc_cdc.c:smcd_cdc_rx_tsklet() {
	...
	if (!conn || conn->killed)
		return;

	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
	...
}

and conn->killed is only set later, inside smc_conn_kill(), once abort_work
actually runs.

For the window in between, can every further ISM DMB interrupt re-enter
smc_cdc_msg_recv_action() with peer-controlled cursors, keep updating
conn->local_rx_ctrl via smc_cdc_msg_to_host() and conn->peer_rmbe_space, and
trip the new test again?  Since queue_work() only fails while the item is
still pending, would a second abort_work run smc_conn_kill() on an
already-killed connection once the first one has started?

Would also testing conn->out_of_sync in smcd_cdc_rx_tsklet(), or at the top
of smc_cdc_msg_recv_action(), address this?

[Severity: Medium]
[Finding: main-5-17]
[Sources: opus-5, old-prompts]
This store now happens on the transport-shared receive path in softirq
context.  The flags share one storage unit:

net/smc/smc.h:struct smc_connection {
	...
	u8			killed : 1;	/* abnormal termination */
	u8			freed : 1;	/* normal termination */
	u8			out_of_sync : 1; /* out of sync with peer */
};

so conn->out_of_sync = 1 is a read-modify-write of that byte.
smc_conn_kill(), running from the abort_work queued right below, writes
conn->killed = 1 into the same byte from process context under lock_sock().

The receive path does not defer to the backlog when the socket is owned:

net/smc/smc_cdc.c:smc_cdc_msg_recv() {
	sock_hold(&smc->sk);
	bh_lock_sock(&smc->sk);
	smc_cdc_msg_recv_action(smc, cdc);
	bh_unlock_sock(&smc->sk);
	sock_put(&smc->sk); /* no free sk in softirq-context */
}

Can the two writers interleave and lose the killed update:

tasklet:    load the flags byte (killed == 0)
abort_work: store killed = 1
tasklet:    store the byte back with out_of_sync = 1 and killed == 0

Since smcd_cdc_rx_tsklet() gates only on conn->killed, would a lost killed
bit let a newly scheduled tasklet keep dereferencing conn->rmb_desc->cpu_addr
after smc_conn_kill() has already run smc_ism_unset_conn() and
smcd_buf_detach()?  And on SMC-R, would a lost out_of_sync bit defeat the
receive barrier this patch relies on?

> +	spin_lock_bh(&conn->send_lock);
> +	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
> +	if (link)
> +		conn->lnk = link;
> +	spin_unlock_bh(&conn->send_lock);
> +	sock_hold(&smc->sk); /* sock_put in abort_work */
> +	if (!queue_work(smc_close_wq, &conn->abort_work))
> +		sock_put(&smc->sk);
> +}

[Severity: High]
[Finding: main-3-9]
[Sources: opus-5, old-prompts]
This queues conn->abort_work with no is_smcd distinction, and the new caller
in smc_cdc_msg_recv_action() is reachable from smcd_cdc_rx_tsklet().  The
only other caller, smc_cdc_msg_validate(), runs from smc_cdc_rx_handler(),
an ib_wc handler, so is this the first path that can queue abort_work for an
SMC-D connection?

smc_conn_free() only cancels the work in its non-SMC-D branch:

net/smc/smc_core.c:smc_conn_free() {
	...
	if (lgr->is_smcd) {
		if (!list_empty(&lgr->list))
			smc_ism_unset_conn(conn);
		if (smc_ism_support_dmb_nocopy(lgr->smcd))
			smcd_buf_detach(conn);
		tasklet_kill(&conn->rx_tsklet);
	} else {
		smc_cdc_wait_pend_tx_wr(conn);
		if (current_work() != &conn->abort_work)
			cancel_work_sync(&conn->abort_work);
	}
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

So if the peer trips the new test and the application then closes the
socket, can a still-pending abort_work outlive smc_conn_free() and later
run smc_conn_kill()?

net/smc/smc_core.c:smc_conn_kill() {
	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
	...
	if (conn->lgr->is_smcd) {
		smc_ism_unset_conn(conn);
	...
}

Two things look problematic there.  conn->lgr is dereferenced
unconditionally although smc_conn_free() already dropped the connection's
reference with smc_lgr_put() and never NULLs conn->lgr.  And
smc_ism_unset_conn() uses a descriptor that smc_buf_unuse() already returned
to the reuse pool:

net/smc/smc_ism.c:smc_ism_unset_conn() {
	if (!conn->rmb_desc)
		return;

	spin_lock_irqsave(&conn->lgr->smcd->lock, flags);
	conn->lgr->smcd->conn[conn->rmb_desc->sba_idx] = NULL;
	spin_unlock_irqrestore(&conn->lgr->smcd->lock, flags);
}

Since smc_buf_get_slot() hands back any descriptor with used == 0 and the
new owner is registered via smc_ism_set_conn(), can this clear the
smcd->conn[] slot of a different, live connection and stop its receives?
The sock_hold() here keeps the smc_sock alive but does not pin the link
group or the buffer descriptor.

Simon says: I think the issue flagged below is out of scope for this patch.

[Severity: High]
[Finding: old-prompts-4-16]
[Sources: old-prompts]
This isn't a bug introduced by this patch, since smc_cdc_msg_validate() used
the same hold-and-queue idiom before, but the patch makes it reachable from
any peer-driven producer-cursor advance rather than only from
failover-validation messages, so it seems worth raising here.

The reference taken by sock_hold() is documented as released by abort_work,
and the only sock_put() is there:

net/smc/smc_core.c:smc_conn_abort_work() {
	...
	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
}

But smc_conn_free() discards the return value when it cancels a still
pending item:

	if (current_work() != &conn->abort_work)
		cancel_work_sync(&conn->abort_work);

cancel_work_sync() returns true exactly when the work was removed without
ever executing.  In that case smc_conn_abort_work() never runs, so does the
struct smc_sock leak because sk_refcnt never reaches zero and sk_free() is
never called?

The sibling work item uses the compensating put:

net/smc/smc_close.c:smc_close_cancel_work() {
	...
	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);
	...
}

Should the abort_work cancel path do the same?

> @@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,

[ ... ]

> @@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
>  				  &conn->local_rx_ctrl.prod);
>  	if (diff_prod) {
> +		if (diff_prod > conn->rmb_desc->len -
> +				atomic_read(&conn->bytes_to_rcv)) {
> +			smc_cdc_abort_conn(smc, NULL);
> +			return;
> +		}

Simon says: Also, seems out of scope for this patch

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5, old-prompts]
This is a pre-existing issue, but two sibling accumulators in the same
function are fed by the same peer-controlled cursor and are still unbounded,
and both are updated before this new test runs:

	diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
				  &conn->local_rx_ctrl.cons);
	if (diff_cons) {
		...
		atomic_add(diff_cons, &conn->peer_rmbe_space);
		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */

and, on the SMC-D dmb-nocopy path:

			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */

Both deltas come from conn->local_rx_ctrl.cons, which is as peer-controlled
as the prod cursor this patch defends against, and smcd_cdc_msg_to_host()
copies it verbatim while smc_cdc_cursor_to_host() only rejects a backwards
move.

The commit message says:

    The test precedes the atomic_add() so the counter is never left inflated

Is that only true for bytes_to_rcv?  Even the message that trips the new
check has already inflated peer_rmbe_space (and sndbuf_space on
loopback-ISM) by the time the abort is queued.  Should the wording be
narrowed to bytes_to_rcv, or should the same treatment be applied to the
cons-derived accumulators?

>  		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
>  			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);

Simon says: Also, seems out of scope for this patch

[Severity: Critical]
[Finding: main-6-20]
[Sources: opus-5, old-prompts]
This isn't a bug introduced by this patch, but the new test bounds only the
delta and the commit message explicitly dismisses bounding the cursor
("Bounding the cursor does not help"), so it seems worth stating that the
delta check does not protect the call directly below it.

smc_cdc_handle_urg_data_arrival() uses the absolute peer-supplied count as a
byte offset into the RMB:

net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
	...
	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
	if (conn->urg_curs.count)
		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
	...
}

Can a peer keep prod.count out of range while still producing a small delta?
smc_curs_diff() computes the wrap case in unsigned int arithmetic:

net/smc/smc_cdc.h:smc_curs_diff() {
	if (old->wrap != new->wrap)
		return max_t(int, 0,
			     ((size - old->count) + new->count));

	return max_t(int, 0, (new->count - old->count));
}

With old = {wrap 0, count 0} and new = {wrap 1, count 0xfffffff0} this wraps
modulo 2^32 and yields len - 16, which passes

	diff_prod > conn->rmb_desc->len - atomic_read(&conn->bytes_to_rcv)

for small bytes_to_rcv, while prod.count stays near 4G.  A variant without
the wrap trick: a first message with count 0x80000000 gives
max_t(int, 0, negative) == 0 and is stored, then count 0x80000001 gives
diff_prod == 1.

Nothing clamps the absolute count: smc_cdc_cursor_to_host() only rejects a
backwards move, and smcd_cdc_msg_to_host() copies peer->prod.wrap and
peer->prod.count with no validation.  The byte read at that offset is then
handed to user space:

net/smc/smc_rx.c:smc_rx_recv_urg() {
	...
	rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
	...
}

so is this an out-of-bounds read at a peer-chosen offset that either leaks
kernel memory through recvmsg(MSG_OOB) or faults on unmapped memory?  Would
validating prod.count < conn->rmb_desc->len alongside the new delta test be
appropriate?

>  		/* bytes_to_rcv is decreased in smc_recvmsg */

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Hidayathulla Khan I]

Thanks for the review.

On 05/08/26 9:33 pm, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804141109.542202-1-hidayath@linux.ibm.com?part=1
> ---
> --- Patch [1]: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB ---
> commit fc7927463c41bac1e3a598538d104b46770ee787
> Author: Hidayath Khan <hidayath@linux.ibm.com>
>
> net/smc: abort the connection when the peer overruns the RMB
>
> The patch factors the connection-abort sequence out of
> smc_cdc_msg_validate() into a new helper smc_cdc_abort_conn(), and calls it
> from smc_cdc_msg_recv_action() when the peer's producer cursor advance would
> push conn->bytes_to_rcv past conn->rmb_desc->len.  The test is placed before
> the atomic_add() and the rest of the message is abandoned.
>
> A few questions about the new helper and its new caller below.
>
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5, old-prompts]
> The commit message describes a peer-triggerable, user-visible defect
> (SIOCINQ reporting a readable length that does not exist, poll() staying
> readable with nothing for recvmsg() to return), but the patch carries
> neither a Fixes: tag nor Cc: stable.
>
> The accumulation site being repaired:
>
> 	atomic_add(diff_prod, &conn->bytes_to_rcv);
> 	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
>
> is part of the original CDC receive implementation, so every stable release
> carrying net/smc looks affected.  Could a Fixes: tag be added so backporters
> know which releases to pick this up for?
>
> Simon says: In a similar vein, if the above is so then perhaps
>              the patch should be targeted at net rather than net-next?
Sure.  I will resend against net with

   Fixes: 5f08318f617b ("smc: connection data control (CDC)")
   Cc: stable@vger.kernel.org

I had chosen net-next because the patch changes behaviour, but the defect
is peer-triggerable and present in every release carrying net/smc, so net
is the better fit.
>
>> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
>> index 32d6d03df3214..d8f747bbc6f69 100644
>> --- a/net/smc/smc_cdc.c
>> +++ b/net/smc/smc_cdc.c
>> @@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
>>   	sk_send_sigurg(&smc->sk);
>>   }
>>   
>> +/* Drop a connection whose peer violated the CDC protocol.  @link is the link
>> + * the offending message arrived on, or NULL when it is not known to the caller
>> + * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
>> + * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
>> + * no per-link QP to abort on.
>> + */
>> +static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
>> +{
>> +	struct smc_connection *conn = &smc->conn;
>> +
>> +	conn->out_of_sync = 1;  /* prevent any further receives */
> [Severity: Medium]
> [Finding: main-2-6]
> [Sources: opus-5, old-prompts]
> The kerneldoc above states the link == NULL case covers SMC-D, and this
> comment says the flag prevents any further receives.  Does
> conn->out_of_sync actually stop receives on SMC-D?
No, it does not.  Mine is the first code that sets the flag on an SMC-D
connection, and smcd_cdc_rx_tsklet() gates only on killed.  I will test
out_of_sync there as well in v2.
>
> The only reader of the flag is the SMC-R IB completion path:
>
> net/smc/smc_cdc.c:smc_cdc_rx_handler() {
> 	...
> 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> 	if (!conn || conn->out_of_sync) {
> 		read_unlock_bh(&lgr->conns_lock);
> 		return;
> 	}
> 	...
> }
>
> The SMC-D entry point gates on a different flag:
>
> net/smc/smc_cdc.c:smcd_cdc_rx_tsklet() {
> 	...
> 	if (!conn || conn->killed)
> 		return;
>
> 	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
> 	...
> }
>
> and conn->killed is only set later, inside smc_conn_kill(), once abort_work
> actually runs.
>
> For the window in between, can every further ISM DMB interrupt re-enter
> smc_cdc_msg_recv_action() with peer-controlled cursors, keep updating
> conn->local_rx_ctrl via smc_cdc_msg_to_host() and conn->peer_rmbe_space, and
> trip the new test again?  Since queue_work() only fails while the item is
> still pending, would a second abort_work run smc_conn_kill() on an
> already-killed connection once the first one has started?
>
> Would also testing conn->out_of_sync in smcd_cdc_rx_tsklet(), or at the top
> of smc_cdc_msg_recv_action(), address this?
>
> [Severity: Medium]
> [Finding: main-5-17]
> [Sources: opus-5, old-prompts]
> This store now happens on the transport-shared receive path in softirq
> context.  The flags share one storage unit:
>
> net/smc/smc.h:struct smc_connection {
> 	...
> 	u8			killed : 1;	/* abnormal termination */
> 	u8			freed : 1;	/* normal termination */
> 	u8			out_of_sync : 1; /* out of sync with peer */
> };
>
> so conn->out_of_sync = 1 is a read-modify-write of that byte.
> smc_conn_kill(), running from the abort_work queued right below, writes
> conn->killed = 1 into the same byte from process context under lock_sock().
>
> The receive path does not defer to the backlog when the socket is owned:
>
> net/smc/smc_cdc.c:smc_cdc_msg_recv() {
> 	sock_hold(&smc->sk);
> 	bh_lock_sock(&smc->sk);
> 	smc_cdc_msg_recv_action(smc, cdc);
> 	bh_unlock_sock(&smc->sk);
> 	sock_put(&smc->sk); /* no free sk in softirq-context */
> }
>
> Can the two writers interleave and lose the killed update:
Yes, the three flags share a byte.  Nobody appears to be working on it,
and it needs a layout change rather than a fix folded in here.  I will
send it separately.
>
> tasklet:    load the flags byte (killed == 0)
> abort_work: store killed = 1
> tasklet:    store the byte back with out_of_sync = 1 and killed == 0
>
> Since smcd_cdc_rx_tsklet() gates only on conn->killed, would a lost killed
> bit let a newly scheduled tasklet keep dereferencing conn->rmb_desc->cpu_addr
> after smc_conn_kill() has already run smc_ism_unset_conn() and
> smcd_buf_detach()?  And on SMC-R, would a lost out_of_sync bit defeat the
> receive barrier this patch relies on?
>
>> +	spin_lock_bh(&conn->send_lock);
>> +	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
>> +	if (link)
>> +		conn->lnk = link;
>> +	spin_unlock_bh(&conn->send_lock);
>> +	sock_hold(&smc->sk); /* sock_put in abort_work */
>> +	if (!queue_work(smc_close_wq, &conn->abort_work))
>> +		sock_put(&smc->sk);
>> +}
> [Severity: High]
> [Finding: main-3-9]
> [Sources: opus-5, old-prompts]
> This queues conn->abort_work with no is_smcd distinction, and the new caller
> in smc_cdc_msg_recv_action() is reachable from smcd_cdc_rx_tsklet().  The
> only other caller, smc_cdc_msg_validate(), runs from smc_cdc_rx_handler(),
> an ib_wc handler, so is this the first path that can queue abort_work for an
> SMC-D connection?
>
> smc_conn_free() only cancels the work in its non-SMC-D branch:
>
> net/smc/smc_core.c:smc_conn_free() {
> 	...
> 	if (lgr->is_smcd) {
> 		if (!list_empty(&lgr->list))
> 			smc_ism_unset_conn(conn);
> 		if (smc_ism_support_dmb_nocopy(lgr->smcd))
> 			smcd_buf_detach(conn);
> 		tasklet_kill(&conn->rx_tsklet);
> 	} else {
> 		smc_cdc_wait_pend_tx_wr(conn);
> 		if (current_work() != &conn->abort_work)
> 			cancel_work_sync(&conn->abort_work);
> 	}
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> 		smc_lgr_unregister_conn(conn);
> 	}
> 	...
> 	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
> }
>
> So if the peer trips the new test and the application then closes the
> socket, can a still-pending abort_work outlive smc_conn_free() and later
> run smc_conn_kill()?
Yes.  Same root cause: smc_conn_free() cancels abort_work only in its
non-SMC-D branch, which was enough while smc_cdc_msg_validate() was the
only queuer.  I have confirmed both consequences you list, the stale
conn->lgr, and the smcd->conn[] slot of a descriptor already handed to
another connection.  I will run the cancel for both transports in v2.
>
> net/smc/smc_core.c:smc_conn_kill() {
> 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
> 	...
> 	if (conn->lgr->is_smcd) {
> 		smc_ism_unset_conn(conn);
> 	...
> }
>
> Two things look problematic there.  conn->lgr is dereferenced
> unconditionally although smc_conn_free() already dropped the connection's
> reference with smc_lgr_put() and never NULLs conn->lgr.  And
> smc_ism_unset_conn() uses a descriptor that smc_buf_unuse() already returned
> to the reuse pool:
>
> net/smc/smc_ism.c:smc_ism_unset_conn() {
> 	if (!conn->rmb_desc)
> 		return;
>
> 	spin_lock_irqsave(&conn->lgr->smcd->lock, flags);
> 	conn->lgr->smcd->conn[conn->rmb_desc->sba_idx] = NULL;
> 	spin_unlock_irqrestore(&conn->lgr->smcd->lock, flags);
> }
>
> Since smc_buf_get_slot() hands back any descriptor with used == 0 and the
> new owner is registered via smc_ism_set_conn(), can this clear the
> smcd->conn[] slot of a different, live connection and stop its receives?
> The sock_hold() here keeps the smc_sock alive but does not pin the link
> group or the buffer descriptor.
>
> Simon says: I think the issue flagged below is out of scope for this patch.
>
> [Severity: High]
> [Finding: old-prompts-4-16]
> [Sources: old-prompts]
> This isn't a bug introduced by this patch, since smc_cdc_msg_validate() used
> the same hold-and-queue idiom before, but the patch makes it reachable from
> any peer-driven producer-cursor advance rather than only from
> failover-validation messages, so it seems worth raising here.
>
> The reference taken by sock_hold() is documented as released by abort_work,
> and the only sock_put() is there:
>
> net/smc/smc_core.c:smc_conn_abort_work() {
> 	...
> 	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
> }
>
> But smc_conn_free() discards the return value when it cancels a still
> pending item:
>
> 	if (current_work() != &conn->abort_work)
> 		cancel_work_sync(&conn->abort_work);
>
> cancel_work_sync() returns true exactly when the work was removed without
> ever executing.  In that case smc_conn_abort_work() never runs, so does the
> struct smc_sock leak because sk_refcnt never reaches zero and sk_free() is
> never called?
>
> The sibling work item uses the compensating put:
>
> net/smc/smc_close.c:smc_close_cancel_work() {
> 	...
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
> 	...
> }
>
> Should the abort_work cancel path do the same?
Agreed. It is a real leak though, and I have already
sent a patch for it: "net/smc: drop the abort_work reference when the
work is cancelled"  which adds the compensating sock_put() using the same
smc_close_cancel_work() idiom that the finding points to.
>
>> @@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
> [ ... ]
>
>> @@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>>   	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
>>   				  &conn->local_rx_ctrl.prod);
>>   	if (diff_prod) {
>> +		if (diff_prod > conn->rmb_desc->len -
>> +				atomic_read(&conn->bytes_to_rcv)) {
>> +			smc_cdc_abort_conn(smc, NULL);
>> +			return;
>> +		}
> Simon says: Also, seems out of scope for this patch
>
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5, old-prompts]
> This is a pre-existing issue, but two sibling accumulators in the same
> function are fed by the same peer-controlled cursor and are still unbounded,
> and both are updated before this new test runs:
>
> 	diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
> 				  &conn->local_rx_ctrl.cons);
> 	if (diff_cons) {
> 		...
> 		atomic_add(diff_cons, &conn->peer_rmbe_space);
> 		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */
>
> and, on the SMC-D dmb-nocopy path:
>
> 			atomic_add(diff_tx, &conn->sndbuf_space);
> 			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
>
> Both deltas come from conn->local_rx_ctrl.cons, which is as peer-controlled
> as the prod cursor this patch defends against, and smcd_cdc_msg_to_host()
> copies it verbatim while smc_cdc_cursor_to_host() only rejects a backwards
> move.
>
> The commit message says:
>
>      The test precedes the atomic_add() so the counter is never left inflated
>
> Is that only true for bytes_to_rcv?
Yes, only bytes_to_rcv.  peer_rmbe_space and sndbuf_space are already
moved by the time the abort is queued.  I will narrow the wording in v2.
Bounding the cons-derived accumulators is a separate
change, and "net/smc: bound the send length to the send buffer in
smc_tx_sendmsg()" already clamps the sndbuf_space one where it is consumed.
>    Even the message that trips the new
> check has already inflated peer_rmbe_space (and sndbuf_space on
> loopback-ISM) by the time the abort is queued.  Should the wording be
> narrowed to bytes_to_rcv, or should the same treatment be applied to the
> cons-derived accumulators?
>
>>   		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
>>   			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);
> Simon says: Also, seems out of scope for this patch
>
> [Severity: Critical]
> [Finding: main-6-20]
> [Sources: opus-5, old-prompts]
> This isn't a bug introduced by this patch, but the new test bounds only the
> delta and the commit message explicitly dismisses bounding the cursor
> ("Bounding the cursor does not help"), so it seems worth stating that the
> delta check does not protect the call directly below it.
>
> smc_cdc_handle_urg_data_arrival() uses the absolute peer-supplied count as a
> byte offset into the RMB:
>
> net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
> 	...
> 	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
> 	if (conn->urg_curs.count)
> 		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
> 	...
> }
>
> Can a peer keep prod.count out of range while still producing a small delta?
> smc_curs_diff() computes the wrap case in unsigned int arithmetic:
>
> net/smc/smc_cdc.h:smc_curs_diff() {
> 	if (old->wrap != new->wrap)
> 		return max_t(int, 0,
> 			     ((size - old->count) + new->count));
>
> 	return max_t(int, 0, (new->count - old->count));
> }
>
> With old = {wrap 0, count 0} and new = {wrap 1, count 0xfffffff0} this wraps
> modulo 2^32 and yields len - 16, which passes
>
> 	diff_prod > conn->rmb_desc->len - atomic_read(&conn->bytes_to_rcv)
>
> for small bytes_to_rcv, while prod.count stays near 4G.  A variant without
> the wrap trick: a first message with count 0x80000000 gives
> max_t(int, 0, negative) == 0 and is stored, then count 0x80000001 gives
> diff_prod == 1.
>
> Nothing clamps the absolute count: smc_cdc_cursor_to_host() only rejects a
> backwards move, and smcd_cdc_msg_to_host() copies peer->prod.wrap and
> peer->prod.count with no validation.  The byte read at that offset is then
> handed to user space:
>
> net/smc/smc_rx.c:smc_rx_recv_urg() {
> 	...
> 	rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
> 	...
> }
>
> so is this an out-of-bounds read at a peer-chosen offset that either leaks
> kernel memory through recvmsg(MSG_OOB) or faults on unmapped memory?  Would
> validating prod.count < conn->rmb_desc->len alongside the new delta test be
> appropriate?
The analysis is right, but "net/smc: bound the wire-controlled producer
cursor to the RMB" already clamps prod.count at the conversion, which
seems the better place for it.  I am happy to reorder behind that series.

Thanks again.

>
>>   		/* bytes_to_rcv is decreased in smc_recvmsg */