Re: [PATCH net v4 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Concurrent `MSG_PEEK` and `recvmsg` calls can cause `readable` to become negative, which the patch incorrectly clamps to the ring buffer size (`rmb_desc->len`). This transforms a temporary race condition into a massive read of stale data. Furthermore, because the loop continues without correctly bounding `peeked_bytes`, `smc_curs_add` is eventually called with a `peeked_bytes` value exceeding `2 * rmb_desc->len`. This triggers an underflow in `smc_curs_add()`'s wrap-around logic, causing the consumer cursor count to exceed the buffer length. The subsequent subtraction `conn->rmb_desc->len - cons.count` produces a negative integer that is cast to a huge `size_t` value, entirely bypassing bounds checks and allowing an unprivileged local user to read contiguous adjacent kernel memory.
- [Critical] Socket reference count underflow in `smc_rx_splice()` on successful multi-page splice. When a virtually contiguous RMB is used, `smc_rx_splice()` can push multiple pages into the pipe in a single `splice_to_pipe()` call. It increments the socket reference count (`sock_hold()`) exactly once. However, the pipe mechanism will eventually call the release callback `smc_rx_pipe_buf_release()` individually for each pipe buffer (page) that was pushed. `smc_rx_pipe_buf_release()` unconditionally calls `sock_put(sk)` on the socket. If `N` pages are spliced, `sock_hold()` is called once, but `sock_put()` is called `N` times, leading to a socket reference count underflow and Use-After-Free of the `smc_sock` structure.
- [High] Page reference count underflow and memory leak in `smc_rx_splice()` error handling. When `splice_to_pipe()` fails to push all pages (e.g., if the pipe becomes full midway), it synchronously calls the pipe buffer release callback `smc_rx_spd_release()` on the rejected pages. This callback unconditionally calls `put_page()`. However, `smc_rx_splice()` only increments the page reference count (`get_page()`) AFTER `splice_to_pipe()` returns, and only for the pages successfully pushed. This causes a page reference count underflow (Use-After-Free) for the rejected pages. Additionally, the corresponding `smc_spd_priv` structures allocated for the rejected pages are leaked, as `smc_rx_spd_release()` does not free them.
--

commit eb0d2eff9f593a1bd0c0a49450526ace209f8db1
Author: Bryam Vargas <hexlabsecurity@proton.me>

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

This commit bounds the readable length to rmb_desc->len in smc_rx_recvmsg()
to prevent a forged cursor from driving bytes_to_rcv past the RMB. It treats
negative values as out of range as well to prevent wrap-around copies from
exceeding the ring buffer.

Cc: stable@vger.kernel.org

> 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
[ ... ]
> +		 * 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: Critical]
This isn't a bug introduced by this patch, but does this clamp allow a
massive read of stale data when readable is negative?

If concurrent MSG_PEEK and recvmsg calls cause bytes_to_rcv - peeked_bytes
to become negative, clamping it to the full rmb_desc->len here allows the
copy loop to accumulate peeked_bytes indefinitely.

Because the loop continues without correctly bounding peeked_bytes,
smc_curs_add() is eventually called with a peeked_bytes value exceeding
2 * rmb_desc->len.

If value >= 2 * size, the wrap-around logic in smc_curs_add() fails to
wrap correctly:

net/smc/smc_cdc.h:smc_curs_add() {
    ...
    if (curs->count >= size) {
        curs->wrap++;
        curs->count -= size;
    }
    ...
}

Does this cause an underflow in the bounds check calculation
conn->rmb_desc->len - cons.count, resulting in a negative integer cast
to a huge size_t that bypasses bounds checks?


While reviewing this, I also noticed a couple of preexisting issues in
smc_rx_splice() and its pipe buffer callbacks:

[Severity: High]
This is a pre-existing issue, but does smc_rx_splice() leak memory and
cause a page reference count underflow if splice_to_pipe() fails partially?

When splice_to_pipe() rejects pages (e.g., pipe is nearly full), it calls
the release callback which unconditionally drops a reference:

net/smc/smc_rx.c:smc_rx_spd_release() {
    ...
    put_page(spd->pages[i]);
}

However, in smc_rx_splice(), get_page() is only called AFTER
splice_to_pipe() returns, and only for the successfully pushed pages:

net/smc/smc_rx.c:smc_rx_splice() {
    ...
    bytes = splice_to_pipe(pipe, &spd);
    if (bytes > 0) {
        sock_hold(&smc->sk);
        if (!lgr->is_smcd && smc->conn.rmb_desc->is_vm) {
            for (i = 0; i < PAGE_ALIGN(bytes + offset) / PAGE_SIZE; i++)
                get_page(pages[i]);
        } else {
            get_page(smc->conn.rmb_desc->pages);
        }
    ...
}

Does this result in an immediate page reference count underflow for the
rejected pages? Furthermore, are the corresponding smc_spd_priv structures
for the rejected pages leaked, since smc_rx_spd_release() doesn't free them?


[Severity: Critical]
This is a pre-existing issue, but does a successful multi-page splice
cause a socket reference count underflow?

In smc_rx_splice(), when pushing multiple pages into the pipe, sock_hold()
is called exactly once for the entire successful splice_to_pipe() operation:

net/smc/smc_rx.c:smc_rx_splice() {
    ...
    bytes = splice_to_pipe(pipe, &spd);
    if (bytes > 0) {
        sock_hold(&smc->sk);
    ...
}

However, as the pipe consumes the buffers, it calls the release callback
individually for every single pipe buffer (page), and unconditionally calls
sock_put() every time:

net/smc/smc_rx.c:smc_rx_pipe_buf_release() {
    ...
out:
    kfree(priv);
    put_page(buf->page);
    sock_put(sk);
}

If N pages are spliced (when using virtually contiguous memory,
is_vm == true), is sock_put() called N times for a single sock_hold(),
leading to a use-after-free of the smc_sock structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-b4-disp-28a1bbca-v4-0-be089b98acc6@proton.me?part=2