Re: [PATCH net v5 2/3] net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch fails to fully prevent out-of-bounds reads because uncorrected negative `bytes_to_rcv` allows `MSG_PEEK` accumulations to exceed `2 * rmb_desc->len`, triggering a wrapping logic bug in `smc_curs_add()`.
- [Critical] In `smc_rx_splice()`, socket and page references are acquired *after* `splice_to_pipe()` returns, creating a race condition (Use-After-Free) and a refcount underflow for multi-page splices.
- [High] The patch prevents an immediate out-of-bounds read but fails to correct the corrupted (negative) `bytes_to_rcv` state, leading to a persistent denial of service, infinite garbage data reads, and corrupted `FIONREAD` ioctl results.
- [High] If `memcpy_to_msg()` or `smc_rx_splice()` fails on the second chunk of a wrapped read, `smc_rx_recvmsg()` returns success for the first chunk but fails to update the consumer cursor.
- [Medium] The fallback release function `smc_rx_spd_release()` fails to free the dynamically allocated `smc_spd_priv` object when `splice_to_pipe()` encounters an error or partial push.
--

--- Patch [2]: [PATCH net v5 2/3] net/smc: bound the receive length to the RMB in smc_rx_recvmsg() ---
commit 5ea7024a42bcb5dcdb8d823a5c82e7b6634aa7d2
Author: Bryam Vargas <hexlabsecurity@proton.me>

net/smc: bound the receive length to the RMB in smc_rx_recvmsg()

This commit bounds the receive length to the RMB length to prevent
out-of-bounds reads when a forged cursor causes a negative or oversized
accumulator. It clamps the local readable variable to the RMB size.

> diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
> index c1d9b923938da..f461cf10b0855 100644
> --- a/net/smc/smc_rx.c
> +++ b/net/smc/smc_rx.c
[ ... ]
> @@ -442,6 +442,18 @@ int smc_rx_recvmsg(struct smc_sock *smc, struct msghdr *msg,
>  		/* initialize variables for 1st iteration of subsequent loop */
>  		/* could be just 1 byte, even after waiting on data above */
>  		readable = smc_rx_data_available(conn, peeked_bytes);
> +		/* bytes_to_rcv is accumulated from the peer's wire-controlled
> +		 * producer cursor; a forged cursor can drive it past the RMB,
> +		 * or overflow the signed accumulator to a negative value across
> +		 * many CDC messages (which a plain "> len" check would miss
> +		 * before the size_t cast below turns it huge).  Bound it to the
> +		 * RMB in either case so the wrap-around copy cannot run past
> +		 * rmb_desc->len.  This enforces the documented
> +		 * 0 <= bytes_to_rcv <= rmb_desc->len invariant at the consumer,
> +		 * race-free against the producer update in the receive tasklet.
> +		 */
> +		if (readable < 0 || readable > conn->rmb_desc->len)
> +			readable = conn->rmb_desc->len;

[Severity: High]
This isn't a bug introduced by this patch, but does this clamping leave the
underlying atomic counter corrupted?

By clamping the local readable variable here, the immediate read is bounded,
but the atomic counter conn->bytes_to_rcv remains negative. Later in
smc_rx_recvmsg(), the code does:

    atomic_sub(copylen, &conn->bytes_to_rcv);

Doesn't this subtract the clamped copylen from an already negative value,
driving it further away from the intended invariant?

If the counter remains negative, won't functions like smc_poll() continuously
see a non-zero value and signal EPOLLIN, causing an infinite read loop?
Additionally, won't the SIOCINQ ioctl in smc_ioctl() return this negative
value directly to userspace?

[Severity: Critical]
This is also a pre-existing issue, but does leaving bytes_to_rcv negative
allow a MSG_PEEK operation to trigger an out-of-bounds read?

If a caller uses MSG_PEEK, the atomic_sub() is skipped, and peeked_bytes
can continuously grow by rmb_desc->len. When peeked_bytes is added to the
cursor:

    if ((flags & MSG_PEEK) && peeked_bytes)
        smc_curs_add(conn->rmb_desc->len, &cons, peeked_bytes);

Looking at smc_curs_add():

    curs->count += value;
    if (curs->count >= size) {
        curs->wrap++;
        curs->count -= size;
    }

Because this uses a single if statement rather than a loop or modulo, if the
added value is greater than or equal to 2 * size, doesn't curs->count remain
larger than size (rmb_desc->len)?

If cons.count exceeds rmb_desc->len, the calculation for chunk_len:

    chunk_len = min_t(size_t, copylen, conn->rmb_desc->len - cons.count);

would underflow, resulting in a massive size_t value and bypassing bounds
checks for the subsequent copy.

[ ... ]
>  		chunk_len_sum = chunk_len;
>  		chunk_off = cons.count;
>  		smc_rmb_sync_sg_for_cpu(conn);
>  		for (chunk = 0; chunk < 2; chunk++) {
>  			if (!(flags & MSG_TRUNC)) {
>  				if (msg) {
>  					rc = memcpy_to_msg(msg, rcvbuf_base +
>  							   chunk_off,
>  							   chunk_len);
>  				} else {
>  					rc = smc_rx_splice(pipe, rcvbuf_base +
>  							chunk_off, chunk_len,
>  							smc);
>  				}

[Severity: Critical]
This is a pre-existing issue, but does smc_rx_splice() have a race condition
and refcount mismatch?

Inside smc_rx_splice(), the code calls splice_to_pipe() before acquiring
references:

    bytes = splice_to_pipe(pipe, &spd);
    if (bytes > 0) {
        sock_hold(&smc->sk);
        ...
        get_page(smc->conn.rmb_desc->pages);
    }

Once splice_to_pipe() exposes the pages to the pipe, a concurrent pipe
reader can consume the buffer and trigger the asynchronous release handler,
smc_rx_pipe_buf_release():

    out:
        kfree(priv);
        put_page(buf->page);
        sock_put(sk);

Since the pipe reader can invoke the release handler before the producing
thread acquires the references with sock_hold() and get_page(), can this
cause a use-after-free?

Additionally, smc_rx_pipe_buf_release() drops the socket reference once per
page, but smc_rx_splice() only takes the socket reference once per splice.
Doesn't this guarantee a reference count underflow for multi-page splices?

[Severity: Medium]
This is a pre-existing issue, but does the fallback release function leak
the private data?

When splice_to_pipe() encounters an error or partial push, it invokes the
fallback handler smc_rx_spd_release():

    static void smc_rx_spd_release(struct splice_pipe_desc *spd,
                                   unsigned int i)
    {
        put_page(spd->pages[i]);
    }

This drops the page reference, but it lacks the kfree() for the dynamically
allocated private data (such as kfree((struct smc_spd_priv *)
spd->partial[i].private)). Will this cause a memory leak if the pipe is full?

>  				if (rc < 0) {
>  					if (!read_done)
>  						read_done = -EFAULT;
>  					goto out;
>  				}

[Severity: High]
This is a pre-existing issue, but does this error path cause silent data
corruption?

When reading across a ring buffer wrap, the copy is split into two chunks.
If the first chunk succeeds but the second chunk fails (for example, if a
local application provides a buffer that causes a page fault, returning
-EFAULT), the code takes this goto out path.

Because it jumps to out, it returns read_done (the successfully read first
chunk) but completely skips the cursor update logic at the end of the loop.

Since the socket's internal cursor is not advanced, won't the exact same data
be duplicated to the user on the next read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-b4-disp-0d07164f-v5-0-6a9e235dbc4e@proton.me?part=2