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

[Simon Horman <horms@kernel.org>]

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 */