All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dust Li <dust.li@linux.alibaba.com>
To: sashiko-reviews@lists.linux.dev
Cc: linux-s390@vger.kernel.org, Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>
Subject: Re: [PATCH net] net/smc: ignore peer-supplied rmbe_idx and dmbe_idx
Date: Tue, 7 Jul 2026 11:32:47 +0800	[thread overview]
Message-ID: <akxzX5lh6fRQZmaF@linux.alibaba.com> (raw)
In-Reply-To: <20260703171239.625D21F000E9@smtp.kernel.org>

On 2026-07-03 17:12:38, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
>New issues:
>- [High] Hardcoding `peer_rmbe_idx` and `tx_off` violates the SMC protocol and breaks interoperability by ignoring the peer-supplied buffer index.
>
>Pre-existing issues:
>- [Critical] Missing bounds check in `dibs_lo_move_data` allows an arbitrary out-of-bounds write in the kernel.
>--
>
>commit 60af5c80d503d1904779918ca5eeba8800e8bbe9
>Author: Dust Li <dust.li@linux.alibaba.com>
>
>net/smc: ignore peer-supplied rmbe_idx and dmbe_idx
>
>This commit modifies the smc connection setup to ignore the peer-supplied
>rmbe and dmbe indices from the clc accept/confirm messages. It hardcodes
>the peer_rmbe_idx to 1 for smc-r and 0 for smc-d, and unconditionally sets
>tx_off to 0, assuming the peer allocates exactly one buffer.
>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -729,11 +729,15 @@ static void smcr_conn_save_peer_info(struct smc_sock *smc,
>>  {
>>  	int bufsize = smc_uncompress_bufsize(clc->r0.rmbe_size);
>>  
>> -	smc->conn.peer_rmbe_idx = clc->r0.rmbe_idx;
>> +	/* Linux uses exactly one RMBE per RMB (always index 1); ignore the
>> +	 * peer-supplied rmbe_idx to prevent a malicious peer from setting an
>> +	 * out-of-bounds tx_off.
>> +	 */
>> +	smc->conn.peer_rmbe_idx = 1;
>>  	smc->conn.local_tx_ctrl.token = ntohl(clc->r0.rmbe_alert_token);
>>  	smc->conn.peer_rmbe_size = bufsize;
>>  	atomic_set(&smc->conn.peer_rmbe_space, smc->conn.peer_rmbe_size);
>> -	smc->conn.tx_off = bufsize * (smc->conn.peer_rmbe_idx - 1);
>> +	smc->conn.tx_off = 0;
>
>[Severity: High]
>Does hardcoding the peer_rmbe_idx and tx_off violate the smc protocol and
>break interoperability with standard-compliant peers?

Thanks for the reporting !

You are right, althrough we only support 1 RMBE/DMBE per RMB/DMB, but this
breaks SMC protocol.

>
>If a remote peer multiplexes multiple connections into a single registered
>memory buffer and assigns an index greater than 1, ignoring this index could
>cause the Linux client to write its rdma payloads to the wrong remote offset.
>
>Could this corrupt the memory of other connections on the remote peer?
>
>>  }
>>  
>>  static void smcd_conn_save_peer_info(struct smc_sock *smc,
>> @@ -741,12 +745,16 @@ static void smcd_conn_save_peer_info(struct smc_sock *smc,
>>  {
>>  	int bufsize = smc_uncompress_bufsize(clc->d0.dmbe_size);
>>  
>> -	smc->conn.peer_rmbe_idx = clc->d0.dmbe_idx;
>> +	/* Linux uses exactly one DMBE per DMB (always index 0); ignore the
>> +	 * peer-supplied dmbe_idx to prevent a malicious peer from deriving an
>> +	 * out-of-bounds tx_off that causes an OOB write.
>> +	 */
>> +	smc->conn.peer_rmbe_idx = 0;
>>  	smc->conn.peer_token = ntohll(clc->d0.token);
>>  	/* msg header takes up space in the buffer */
>>  	smc->conn.peer_rmbe_size = bufsize - sizeof(struct smcd_cdc_msg);
>>  	atomic_set(&smc->conn.peer_rmbe_space, smc->conn.peer_rmbe_size);
>> -	smc->conn.tx_off = bufsize * smc->conn.peer_rmbe_idx;
>> +	smc->conn.tx_off = 0;
>
>[Severity: High]
>Similarly for smc-d, does discarding the peer-supplied dmbe_idx cause ism
>writes to target the wrong remote offset if the remote peer uses an index
>other than 0?
>
>[Severity: Critical]
>This isn't a bug introduced by this patch, but there appears to be a missing
>bounds check in the smc loopback driver:
>
>drivers/dibs/dibs_loopback.c:dibs_lo_move_data() {
>    ...
>    memcpy((char *)rmb_node->cpu_addr + offset, data, size);
>    ...
>}
>
>If a malicious local process acting as an smc loopback peer allocates a
>small dmb buffer but sends a forged clc accept message over the tcp socket
>with a deceptively large dmbe_size, could this trigger an out-of-bounds
>write?
>
>The local smc client would compute a massive peer_rmbe_size and pass a large
>size and offset to smcd->ops->move_data() when transmitting data. Without
>verifying that the requested offset + size fits within the actually
>allocated rmb_node->len, it seems this might overwrite kernel memory.


This is the root cause, I will just send a patch fixing this.

Best regards,
Dust

>
>>  }
>
>-- 
>Sashiko AI review · https://sashiko.dev/#/patchset/20260702171137.1099051-2-dust.li@linux.alibaba.com?part=1

  reply	other threads:[~2026-07-07  3:32 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-02 17:11 [PATCH net] net/smc: ignore peer-supplied rmbe_idx and dmbe_idx Dust Li
2026-07-03 17:12 ` sashiko-bot
2026-07-07  3:32   ` Dust Li [this message]
2026-07-07  3:56 ` Dust Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=akxzX5lh6fRQZmaF@linux.alibaba.com \
    --to=dust.li@linux.alibaba.com \
    --cc=agordeev@linux.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=linux-s390@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.