Re: [PATCH net v4 2/3] net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages

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

New issues:
- [High] The patch breaks SMC-R V2 connection setups and rkey deletions for devices with `max_recv_sge == 1` by intentionally discarding valid extension data under a false assumption.

Pre-existing issues:
- [High] The parsing loops process uninitialized or stale memory because they validate against buffer capacity rather than actual received network length.
- [High] Lockless modification of shared `lgr->rtokens` array and bitmap by `smc_rtoken_delete` and `smc_rtoken_add`.
- [High] CPU writes to a buffer mapped for DMA_FROM_DEVICE that is actively exposed to the hardware receive queue.
--

--- Patch [2]: [PATCH net v4 2/3] net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages ---
commit 3af7503159894578c914029204c5f338ebbaaf3d
Author: Yehyeong Lee <[email protected]>

net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages

This patch bounds the rkey extraction loops in SMC-Rv2 LLC messages to
the buffer size to prevent slab-out-of-bounds reads. It also skips the
extension processing entirely for devices without a shared v2 receive
buffer, under the assumption that nothing beyond the 44 inline bytes
was received.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 90746cd1e29a3..606d2dc8c7ece 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1000,13 +1000,22 @@ static void smc_llc_save_add_link_rkeys(struct smc_link *link,
>  					struct smc_link *link_new,
>  					u8 *llc_msg)
>  {
> +	const u32 rt_off = offsetof(struct smc_llc_msg_add_link_v2_ext, rt);
>  	struct smc_llc_msg_add_link_v2_ext *ext;
>  	struct smc_link_group *lgr = link->lgr;
>  	int max, i;
>  
> +	/* The extension follows the 44 bytes of the LLC message.  Without a
> +	 * shared v2 receive buffer nothing beyond those 44 bytes was received,
> +	 * so not even ext->num_rkeys is there to be read.
> +	 */
> +	if (!smc_link_shared_v2_rxbuf(link))
> +		return;

[Severity: High]
Does this early return cause a permanent protocol failure for devices with a
single receive SGE?

It appears that when max_recv_sge == 1, lnk->wr_rx_buflen is configured to
8192 bytes, meaning the single SGE actually does receive the full extension
data from the hardware. 

By dropping the data here under the assumption that it wasn't received, does
this silently break the protocol and cause peers to get Remote Access Errors
during RDMA writes because the rkeys are never registered?

>  	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
>  						     SMC_WR_TX_SIZE);
>  	max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> +	max = min_t(u32, max, (SMC_WR_BUF_V2_SIZE - SMC_WR_TX_SIZE - rt_off) /
> +			      sizeof(ext->rt[0]));
>  	down_write(&lgr->rmbs_lock);
>  	for (i = 0; i < max; i++) {
>  		smc_rtoken_set(lgr, link->link_idx, link_new->link_idx,

[Severity: High]
This isn't a bug introduced by this patch, but could this loop read
uninitialized or stale memory if a peer sends a truncated packet?

The loop in smc_llc_save_add_link_rkeys() calculates max based on the buffer
capacity (SMC_WR_BUF_V2_SIZE) instead of the actual received network length. 

If a peer sends a short packet (e.g., 44 bytes) but specifies a large
num_rkeys in the header, could this read beyond the received data into stale
buffer memory, resulting in the host registering arbitrary memory regions?

[ ... ]

> @@ -1813,17 +1822,27 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>  	link = qentry->link;
>  
>  	if (lgr->smc_version == SMC_V2) {
> +		const u32 rkey_off =
> +			offsetof(struct smc_llc_msg_delete_rkey_v2, rkey);
>  		struct smc_llc_msg_delete_rkey_v2 *llcv2;
> +		u32 buf_len;
>  
>  		if (smc_link_shared_v2_rxbuf(link)) {
>  			memcpy(lgr->wr_rx_buf_v2, llc, sizeof(*llc));

[Severity: High]
This is a pre-existing issue, but does this memcpy() write to a buffer that
is actively mapped and exposed to the hardware?

Writing to lgr->wr_rx_buf_v2 (a DMA_FROM_DEVICE buffer) without unmapping or
syncing violates the DMA API. On non-cache-coherent architectures, dirtying
the cacheline here can cause conflicts when the cache is later invalidated to
read the hardware's data, which might destroy either the CPU's write or the
incoming hardware data.

>  			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;
> +			buf_len = SMC_WR_BUF_V2_SIZE;
>  		} else {
>  			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
> +			buf_len = sizeof(qentry->msg);
>  		}

[Severity: High]
Similar to the issue above, bounding buf_len to sizeof(qentry->msg) for
devices without a shared v2 receive buffer forces a limit of 44 bytes. Does
this silently truncate and ignore valid deletion requests from the sender when
the underlying single SGE actually received the full payload?

>  		llcv2->num_inval_rkeys = 0;
>  
>  		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> +		/* the rkeys live in the buffer llcv2 points at, and nothing
> +		 * beyond it was received
> +		 */
> +		max = min_t(u32, max, (buf_len - rkey_off) /
> +				      sizeof(llcv2->rkey[0]));
>  		for (i = 0; i < max; i++) {
>  			if (smc_rtoken_delete(link, llcv2->rkey[i]))
>  				llcv2->num_inval_rkeys++;

[Severity: High]
This is a pre-existing issue, but can this call to smc_rtoken_delete() cause
a data race?

It appears smc_llc_rmt_delete_rkey() calls smc_rtoken_delete() inside this
loop without holding lgr->rmbs_lock. Because smc_rtoken_delete() locklessly
modifies the shared lgr->rtokens array and the rtokens_used_mask bitmap,
could this race with concurrent modifications in smc_rtoken_add() and lead to
leaked rtokens or corrupted memory registrations?

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