Re: [PATCH net] net/smc: fix out-of-bounds read of rkey array in SMC-Rv2 LLC processing

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Remote NULL pointer dereference in `smc_llc_rx_handler` when receiving an SMC-Rv2 packet on an SMC-Rv1 link group.
- [High] Data race and payload corruption due to lockless shared buffer overwrite in tasklet context.
- [High] Processing uninitialized or stale memory for V1 messages on V2 link groups.

Pre-existing issues:
- [High] Incomplete fix for out-of-bounds read translates the vulnerability into stale memory processing.
--

--- Patch [1]: [PATCH net] net/smc: fix out-of-bounds read of rkey array in SMC-Rv2 LLC processing ---
commit aefbf14dc11357a5a9793f782a4910a8949cde03
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

net/smc: fix out-of-bounds read of rkey array in SMC-Rv2 LLC processing

This commit addresses an out-of-bounds read in SMC-Rv2 LLC processing by
copying the full message to wr_rx_buf_v2 for the non-shared layout. The
subsequent processing functions are then updated to read the rkey array
from this buffer, bounding the reads to SMC_WR_BUF_V2_SIZE.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index 954b2ff1815c2..1c32bf9700989 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c

[ ... ]

> @@ -1812,12 +1810,15 @@ static void smc_llc_rmt_delete_rkey(struct smc_link_group *lgr)
>  	if (lgr->smc_version == SMC_V2) {
>  		struct smc_llc_msg_delete_rkey_v2 *llcv2;
>  
> -		if (smc_link_shared_v2_rxbuf(link)) {
> +		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;
> -		} else {
> -			llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;
> -		}
> +		/* The full message, including the rkey array, has been placed in
> +		 * wr_rx_buf_v2 for both rxbuf layouts (shared: header copy above
> +		 * + spillover DMA; non-shared: full copy in smc_llc_rx_handler()).
> +		 * Reading it from the fixed-size qentry copy would run past its
> +		 * end.
> +		 */
> +		llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)lgr->wr_rx_buf_v2;

[Severity: High]
If a peer sends a V1 message (llc_version == 0) over a V2 link, the memcpy
in smc_llc_rx_handler() is bypassed. However, since the link group version
(lgr->smc_version) is SMC_V2, we reach this point and read from
lgr->wr_rx_buf_v2.

Doesn't this mean we are reading and processing stale memory left over from
a previous V2 message, rather than the V1 message just received?

[Severity: High]
Since smc_llc_rx_handler() unconditionally overwrites lgr->wr_rx_buf_v2
from a tasklet without restoring the header from the queued llc copy for
the non-shared layout, what happens if concurrent LLC messages are received?

If a large message (like DELETE_RKEY_V2) is quickly followed by a short
message (like TEST_LINK), couldn't the short message overwrite the first
few bytes of lgr->wr_rx_buf_v2 before the worker thread runs? This would
cause the worker thread to parse corrupted headers for the first message.

>  		llcv2->num_inval_rkeys = 0;
>  
>  		max = min_t(u8, llcv2->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);

[Severity: High]
This is a pre-existing issue, but this loop relies on the untrusted
num_rkeys without verifying if the received packet is actually large enough
to contain them.

If a truncated message is received with a large num_rkeys,
smc_llc_rx_handler() only copies up to wc->byte_len bytes into
lgr->wr_rx_buf_v2.

Doesn't this loop then read past the newly copied bytes into stale memory
from previous messages left in the buffer?

[ ... ]

> @@ -2104,6 +2105,16 @@ static void smc_llc_rx_handler(struct ib_wc *wc, void *buf)
>  	} else {
>  		if (llc->raw.hdr.length_v2 < sizeof(*llc))
>  			return; /* invalid message */
> +		/* For the non-shared v2 rxbuf layout the received message lives
> +		 * only in the per-WR receive buffer, which is reposted as soon as
> +		 * this handler returns.  The LLC event handlers run later from a
> +		 * work item and read the message from wr_rx_buf_v2, so the whole
> +		 * message must be preserved there now, mirroring the shared-rxbuf
> +		 * layout where the body arrives in wr_rx_buf_v2 via spillover DMA.
> +		 */
> +		if (!smc_link_shared_v2_rxbuf(link))
> +			memcpy(link->lgr->wr_rx_buf_v2, llc,
> +			       min_t(u32, wc->byte_len, SMC_WR_BUF_V2_SIZE));

[Severity: Critical]
If a remote peer sends an LLC message with a non-zero llc_version on an
SMC-Rv1 link group, we enter this else block.

For V1 links, smc_link_shared_v2_rxbuf(link) evaluates to false because
lnk->wr_rx_sge_cnt is 1, so we execute this memcpy. However,
lgr->wr_rx_buf_v2 is only allocated for V2 link groups and will be NULL
for V1 links.

Could this result in a NULL pointer dereference in the tasklet context if
an attacker sends such a packet on a V1 link?

>  	}
>  
>  	smc_llc_enqueue(link, llc);

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