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

On Mon, Jul 27, 2026 at 10:15:11AM +0800, D. Wythe wrote:

Could we consider attaching a full copy of the message body directly to
the qentry (e.g. kmalloc(wc->byte_len, GFP_ATOMIC)) for V2 messages that
exceed the 44-byte inline union smc_llc_msg? The only V2 types that grow
beyond the header are DELETE_RKEY_V2 (batch-delete, rare),
ADD_LINK_RESP_V2 with ext (link setup, rare), and REQ_ADD_LINK_V2
(~hundreds of bytes). Memory pressure should be acceptable, and this
also unifies the shared/non-shared paths.

Yes, I agree, and I will send a v3 that does this.

It is better than what I posted for a reason I underweighted. In my v2
the body still lives in the per-link-group lgr->wr_rx_buf_v2, which
every LLC message on that link group shares, so a later v2 message can
overwrite it between reception and the llc_event_work item that consumes
it. My v2 protects the header from exactly that by restoring it from the
per-qentry copy, and the commit message says so - but it leaves the body
exposed and I did not state that plainly. Copying the body onto the
qentry removes the exposure rather than bounding it: the buffer then has
the same lifetime as the request that owns it.

One refinement. Since union smc_llc_msg is the last member of struct
smc_llc_qentry, the body does not need a second allocation and a pointer
to free; it can travel in the same allocation as a trailing flexible
array:

        struct smc_llc_qentry {
                struct list_head list;
                struct smc_link *link;
                u16 body_len;
                union smc_llc_msg msg;
                u8 body[] __counted_by(body_len);
        };

I measured the layout rather than assuming it: msg sits at offset 28 and
ends at 72, which is where body[] begins, so header and body stay
contiguous and the existing v2 casts become correct instead of out of
bounds - the non-shared line

        llcv2 = (struct smc_llc_msg_delete_rkey_v2 *)llc;

stands as it is today, and the shared/non-shared branch in
smc_llc_rmt_delete_rkey() and the two ternaries at the
smc_llc_save_add_link_rkeys() call sites are deleted rather than
rewritten. smc_llc.c already asserts the invariant this relies on:

        BUILD_BUG_ON_MSG(sizeof(union smc_llc_msg) != SMC_WR_TX_SIZE,

Every existing kfree(qentry) site also stays correct unchanged, since
there is still exactly one allocation, so the flexible array adds no
paired-free surface. One lifetime question does surface, though it is
not created by this change: smc_llc_srv_add_link() calls
smc_llc_flow_qentry_del() before the v2 rkeys are read, so the
non-shared path already dereferences add_llc after the qentry has been
freed. My v2 hid that by reading wr_rx_buf_v2 instead; reading from the
qentry makes it load-bearing, so v3 keeps the qentry alive until the
rkeys have been consumed.

One consequence is worth stating, because it changes what the patch has
to contain. Once the body lives on the qentry, the backing allocation is
exactly as large as what arrived, so the loop bound has to come from
what arrived as well. Today the count is capped only by
SMC_LLC_RKEYS_PER_MSG_V2:

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

In my v2 that was sufficient for memory safety only by accident of the
destination: the reads always landed in the 8 KiB wr_rx_buf_v2, and
num_rkeys = 255 reaches offset 1028. With a per-qentry body, a 44-byte
DELETE_RKEY_V2 declaring num_rkeys = 255 gives body_len = 0 and the loop
walks off the allocation again. The same applies to ext->num_rkeys on
the ADD_LINK path, where even reading the count needs 28 bytes of body.

So v3 bounds both counts by the received length instead of only by
SMC_LLC_RKEYS_PER_MSG_V2. That is the validation I deferred as separate
hardening in the v2 commit message; under this design it stops being
hardening and becomes part of the fix, which I take as an argument for
the design rather than against it.

To be precise about unifying the two layouts: every consumer unifies,
but smc_llc_enqueue() still needs one branch, because in the shared
layout the body was DMA'd to lgr->wr_rx_buf_v2 + SMC_WR_TX_SIZE by the
spillover SGE and is not contiguous with the header it copies from,
while in the non-shared layout it is. That is one if/else at the single
copy site instead of a branch in each consumer.

Two things I would like your view on:

- smc_llc_enqueue() already returns without queueing when the qentry
  allocation fails, so the LLC request is dropped and the flow times
  out. I plan to keep exactly that behaviour for the larger allocation
  rather than adding a fallback.

- The allocation grows from 72 bytes to at most SMC_WR_BUF_V2_SIZE per
  queued request. Those three message types are rare, as you say, but
  they are peer-driven. Is the llc_event_q depth bounded anywhere, or is
  LLC flow control what limits it?

I will keep the Fixes: tag and target net, since the out-of-bounds read
is still what the patch fixes. Tell me if you would rather see the
restructuring go to net-next with a minimal fix for net.

Best regards,
Yehyeong Lee