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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Processing of stale memory due to incorrect bounds checking based on maximum buffer capacity.

Pre-existing issues:
- [Critical] Hardware DMA race condition overwrites shared receive spillover buffers.
- [High] Data race corrupting rtokens due to missing rmbs_lock in remote flow handlers.
--

--- Patch [2]: [PATCH net v5 2/3] net/smc: bound the peer rkey counts in SMC-Rv2 LLC messages ---
commit b119f15f6831509347ac2042d2fc186b18c74eae
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

On a link whose device has max_recv_sge == 1 there is no shared v2 receive
buffer, and smc_llc_save_add_link_rkeys() takes the v2 extension from 44
bytes past the start of the queue entry's inline message.

[ ... ]

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 36c254f257350..c9bf938f1ee2a 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1000,13 +1000,21 @@ 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;
>  
> +	/* Without a shared v2 receive buffer the extension is not copied
> +	 * into the queue entry, so not even ext->num_rkeys is there.
> +	 */
> +	if (!smc_link_shared_v2_rxbuf(link))
> +		return;
>  	ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
>  						     SMC_WR_TX_SIZE);

[Severity: Critical]
This is a pre-existing issue, but does reading from this shared spillover buffer
expose us to a hardware DMA race condition?

All receive work requests are initialized with their second SGE pointing to the
exact same shared physical memory address:

net/smc/smc_wr.c:smc_wr_init_sge() {
    ...
    lnk->wr_rx_sges[x + 1].addr = lnk->wr_rx_v2_dma_addr + SMC_WR_TX_SIZE;
    ...
}

Because hardware DMA operates independently of the CPU, if two packets larger
than 44 bytes arrive back-to-back, won't the network card silently overwrite the
first packet's spillover data with the second packet's spillover data?

When the deferred workqueue finally reads the first packet's extension from
lgr->wr_rx_buf_v2 here in smc_llc_save_add_link_rkeys(), it seems it could read
corrupted or overwritten data from the second packet, allowing a remote peer to
induce cross-message fragment mixing.

>  	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]));

[Severity: High]
Does this bounds check allow processing of stale memory?

The bounds check uses the maximum buffer capacity (SMC_WR_BUF_V2_SIZE) rather
than the actual message length received by the hardware. Because the shared
spillover buffer lgr->wr_rx_buf_v2 is not zeroed between receives, could a
remote attacker send a short packet with a maliciously large num_rkeys value?

If so, the bounds check would allow the loop to run past the end of the newly
received data, reading and processing stale memory contents left over from
previous messages as valid rkeys.

>  	down_write(&lgr->rmbs_lock);
>  	for (i = 0; i < max; i++) {
>  		smc_rtoken_set(lgr, link->link_idx, link_new->link_idx,

[ ... ]

> @@ -1813,17 +1821,25 @@ 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));
>  			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);
>  		}
>  		llcv2->num_inval_rkeys = 0;
>  
>  		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
> +		/* bound by the buffer llcv2 points at */
> +		max = min_t(u32, max, (buf_len - rkey_off) /
> +				      sizeof(llcv2->rkey[0]));

[Severity: High]
Is it safe to bound this by buf_len instead of the actual received length?

Similar to smc_llc_save_add_link_rkeys(), this uses the maximum capacity
(SMC_WR_BUF_V2_SIZE or sizeof(qentry->msg)) rather than the actual message
length. A short packet with a large num_rkeys could cause this loop to
process stale data left in the buffer from previous messages.

>  		for (i = 0; i < max; i++) {
>  			if (smc_rtoken_delete(link, llcv2->rkey[i]))

[Severity: High]
This is a pre-existing issue, but is there a data race here that could corrupt
rtokens?

The remote flow handlers like smc_llc_rmt_delete_rkey() execute in the context
of smc_llc_event_work. They call smc_rtoken_delete() and smc_rtoken_add(),
modifying the shared lgr->rtokens array and rtokens_used_mask without acquiring
down_write(&lgr->rmbs_lock).

Because local flow handlers (like smc_llc_add_link_work) can run concurrently
on another CPU and do take rmbs_lock to modify the same structures, won't this
result in a data race that can corrupt the token arrays and mask?

>  				llcv2->num_inval_rkeys++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801094208.1937951-1-yhlee@isslab.korea.ac.kr?part=2