Re: [PATCH net v7 2/4] net/smc: bound the receive length to the RMB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/smc: missing bounds check on bytes_to_rcv in poll/ioctl
- [High] net: smc: UAF in smc_rx_recvmsg() if rmb_desc is freed during unlocked wait
--

commit 724586149d84b7c3037623750dccc8a10739e047
Author: Bryam Vargas <hexlabsecurity@proton.me>
Subject: net/smc: bound the receive length to the RMB

This commit fixes an issue where a forged producer cursor can drive the
receive byte count past the buffer size or into negative values. It
bounds the data available in the receive buffer to the cached RMB length,
preventing out-of-bounds memory accesses during receive operations.

Link: https://sashiko.dev/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c@proton.me?part=2
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=2
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-b4-disp-f7cd3d96-v6-0-fba616c0920c%40proton.me?part=1

[ ... ]

> diff --git a/net/smc/smc_rx.h b/net/smc/smc_rx.h
> index 994f5e42d1ba2..6bf44e3cc33a0 100644
> --- a/net/smc/smc_rx.h
> +++ b/net/smc/smc_rx.h
> @@ -25,7 +25,16 @@ int smc_rx_wait(struct smc_sock *smc, long *timeo, size_t peeked,
>  		int (*fcrit)(struct smc_connection *conn, size_t baseline));
>  static inline int smc_rx_data_available(struct smc_connection *conn, size_t peeked)
>  {
> -	return atomic_read(&conn->bytes_to_rcv) - peeked;
> +	int avail = atomic_read(&conn->bytes_to_rcv);
> +
> +	/* bytes_to_rcv follows the peer's producer cursor and can be past
> +	 * the RMB or negative. Report no more than fits in the RMB behind the
> +	 * bytes already spliced or peeked. Compare before subtracting: near
> +	 * INT_MIN, avail - peeked wraps. This also runs unlocked, possibly
> +	 * after rmb_desc was freed, hence rmb_len.
> +	 */

[Severity: High]
If this runs after rmb_desc was freed during an unlocked wait in
smc_rx_recvmsg(), and returns true because avail is greater than peeked,
won't the reader proceed to use the freed buffer?

Looking at smc_rx_recvmsg() in net/smc/smc_rx.c, the base pointer is
calculated before the wait:

    rcvbuf_base = conn->rx_off + conn->rmb_desc->cpu_addr;

If the connection is closed and rmb_desc is freed during the wait, and
smc_rx_recvmsg_data_available() returns true here, the code jumps directly
to the copy phase:

    if (smc_rx_recvmsg_data_available(smc, peeked_bytes))
        goto copy;

Can this lead to a use-after-free when dereferencing the freed
conn->rmb_desc->len in smc_curs_add() and reading from the stale
rcvbuf_base pointer?

[Severity: High]
While this handles the negative value in recvmsg() by clamping it to 0, what
happens to other readers of bytes_to_rcv when a forged cursor in
smc_cdc_msg_recv_action() drives it negative?

For instance, in smc_poll() in net/smc/af_smc.c:

    if (atomic_read(&smc->conn.bytes_to_rcv))
        mask |= EPOLLIN | EPOLLRDNORM;

Does this evaluate to true for negative values, potentially causing an
infinite epoll_wait loop?

Also, in __smc_getsockopt() in net/smc/af_smc.c:

    answ = atomic_read(&smc->conn.bytes_to_rcv);

Could this leak the negative byte count to userspace via the SIOCINQ ioctl?

> +	avail = min(avail, conn->rmb_len - atomic_read(&conn->splice_pending));
> +	return avail > (int)peeked ? avail - (int)peeked : 0;
>  }
>  
>  #endif /* SMC_RX_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-disp-f345196c-v7-0-ef3dddfbfa68@proton.me?part=2