Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Bryam Vargas]

Hidayath,

> I have a standalone net-next patch that aborts the connection when
> bytes_to_rcv + diff_prod exceeds rmb_desc->len.  The check sits before
> the atomic_add(), so the accumulator is never written with an out-of-range
> value

That covers the follow-up I said I would send, and the placement is better than
what I described: I had said a check after the atomic_add, which only notices
the counter is already out of range. Yours doesn't let it get there. Consider my
follow-up withdrawn -- I am not sending a competing patch.

If it is useful for the Fixes decision: I ran the wrap++/count==0 vector on the
real SMC-D path under KASAN while working on the cursor series. With only the
per-cursor bound applied, bytes_to_rcv reaches 6*len and smc_rx_recvmsg() trips
slab-out-of-bounds on a read of 5*len; each CDC advances exactly len, so
diff == len and an advance-bound does not fire -- it's the accumulation that
overruns, which is what your check catches. Logs on request if you want them in
the commit message.

Two heads-up on collisions, since both are in flight this week rather than
merged:

smc_cdc_msg_recv_action() is also touched by "net/smc: order the CDC receive
path against buffer publication" (v4, 20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me),
which hoists sndbuf_desc to the top of the function and gates the tx-trigger on
it. Your hunk sits just above that gate, so whichever lands second will want a
look rather than a blind rebase. I'd rather flag it now than after a conflict.

And you mentioned running the abort_work cancel for both transports in v2 --
that edits smc_conn_free()'s SMC-D branch, which "net/smc: unregister the
connection before draining the rx tasklet"
(20260808-b4-disp-22f119e6-v2-1-61647601a6f3@proton.me) also rewrites: it drops
the !list_empty guard around smc_ism_unset_conn(), moves the drain ahead of the
detach, and clears conn->sndbuf_desc before freeing it. Same branch, same week.

On the shared bitfield -- agreed it needs a layout change rather than something
folded into a fix, and it's yours; I'd noted it and left it alone for the
same reason.

Thanks,
Bryam

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Hidayath Khan]

On 08/08/26 1:42 pm, Bryam Vargas wrote:
> Hidayath,
>
>> I have a standalone net-next patch that aborts the connection when
>> bytes_to_rcv + diff_prod exceeds rmb_desc->len.  The check sits before
>> the atomic_add(), so the accumulator is never written with an out-of-range
>> value
> That covers the follow-up I said I would send, and the placement is better than
> what I described: I had said a check after the atomic_add, which only notices
> the counter is already out of range. Yours doesn't let it get there. Consider my
> follow-up withdrawn -- I am not sending a competing patch.
>
> If it is useful for the Fixes decision: I ran the wrap++/count==0 vector on the
> real SMC-D path under KASAN while working on the cursor series. With only the
> per-cursor bound applied, bytes_to_rcv reaches 6*len and smc_rx_recvmsg() trips
> slab-out-of-bounds on a read of 5*len; each CDC advances exactly len, so
> diff == len and an advance-bound does not fire -- it's the accumulation that
> overruns, which is what your check catches. Logs on request if you want them in
> the commit message.
Hi Bryam,

Yes, please send the logs.  I would like to put the splat in the commit
message and credit you for the reproduction.  Right now my changelog only
talks about the accounting damage -- SIOCINQ showing a length that is not
there, and poll() staying readable with nothing to read.  A real
slab-out-of-bounds read is a much stronger claim.  Any format is fine, I
will trim it.

Your point that diff == len on every message, so an advance bound never
fires, is also worth putting in the changelog.  I had argued that only from
the arithmetic, not from a run.
>
> Two heads-up on collisions, since both are in flight this week rather than
> merged:
>
> smc_cdc_msg_recv_action() is also touched by "net/smc: order the CDC receive
> path against buffer publication" (v4, 20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me),
> which hoists sndbuf_desc to the top of the function and gates the tx-trigger on
> it. Your hunk sits just above that gate, so whichever lands second will want a
> look rather than a blind rebase. I'd rather flag it now than after a conflict.
Agreed. From reading the code they are in different parts of the function
-- my hunk sits above the tx-trigger gate you add.

But my v2 also adds an out_of_sync check to
smcd_cdc_rx_tsklet(), and your v4 edits the same line:

   yours: keeps "if (!conn || conn->killed)" and adds the rmb_desc
          smp_load_acquire() after it
   mine:  rewrites it to "if (!conn || conn->killed || conn->out_of_sync)"

So they conflict.
>
> And you mentioned running the abort_work cancel for both transports in v2 --
> that edits smc_conn_free()'s SMC-D branch, which "net/smc: unregister the
> connection before draining the rx tasklet"
> (20260808-b4-disp-22f119e6-v2-1-61647601a6f3@proton.me) also rewrites: it drops
> the !list_empty guard around smc_ism_unset_conn(), moves the drain ahead of the
> detach, and clears conn->sndbuf_desc before freeing it. Same branch, same week.
Confirmed, that one conflicts too.  My v2 hunk moves
cancel_work_sync(&conn->abort_work) out of the non-SMC-D branch so it runs
for both transports.

So my v2 now conflicts with both of your patches.  Both of yours are posted
and mine is not, so I will rebase on top of both rather than ask you to
work around me.

If you would rather take the abort_work cancel into your
teardown 1/2 while you are already in that branch, please say so and I will
drop that hunk.
>
> On the shared bitfield -- agreed it needs a layout change rather than something
> folded into a fix, and it's yours; I'd noted it and left it alone for the
> same reason.
Thanks, I will send it separately.

One more thing, for information.  I have sent "net/smc: fix use-after-free
in smc_rx_pipe_buf_release()" to the list.  It clears conn->rmb_desc in
smc_buf_unuse(), which is the rmb version of the sndbuf_desc clear in your
teardown 1/2.  These are different functions, so from inspection they
should not conflict.

>
> Thanks,
> Bryam
Thanks,
Hidayath
>

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Bryam Vargas]

Hidayath,

Sorry this sat for two months -- a major earthquake here, then
wrapping up my postgrad program.

> Yes, please send the logs.

Log below, from today's re-run on v7.3-rc6. One note on the label
first: it isn't stable across runs. My Aug 8 mail said
slab-out-of-bounds, which is what the Jul 23 run printed; the Jul 11
run said slab-use-after-free and today's says use-after-free, all on
the same 327520-byte read (5 * len). The second chunk is read from ring
offset 0, so its first 65504 bytes are still inside the RMB and the
remaining 262016 run past it, and KASAN names it after whatever sits
past the RMB. If the commit message names the bug type, take it from
the log you paste.

Repro: two AF_SMC sockets over SMC-D loopback, v7.3-rc6 with KASAN,
rmb_desc->len 65504. The sender puts its producer cursor on the wire
as wrap++ with count 0, six times. Each CDC passes every per-cursor
bound and smc_curs_diff() returns len for each, so bytes_to_rcv
reaches 393024 (6 * len). recv() returns 393024 and the second chunk
is a 327520-byte read:

  BUG: KASAN: use-after-free in _copy_to_iter+0x183/0x1390
  Read of size 327520 at addr ffff88814a0f0020 by task smc_forge_test/1695
  Call Trace:
   _copy_to_iter+0x183/0x1390
   smc_rx_recvmsg+0xbe0/0x27a0 [smc]
   smc_recvmsg+0x1c9/0x3a0 [smc]
   sock_recvmsg+0x14b/0x190
   __sys_recvfrom+0x190/0x2a0
   __x64_sys_recvfrom+0xdb/0x1b0
   do_syscall_64+0xdd/0x4a0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
  The buggy address belongs to the physical page:
  head: order:4 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
  Memory state around the buggy address:
   ffff88814a0fff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  >ffff88814a100000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

Trimmed: KASAN's own frames, the "?" frames, registers and the page
dump; full log on request. With our v6 cursor series applied (2/3
bounds the receive length) the same run returns 65504 and KASAN stays
quiet, and an unforged transfer is clean. I haven't run it against
your patch. FWIW the forging is a test knob on the sender; on the rx
side the test module only adds a read-only readback of bytes_to_rcv,
which the test polls instead of sleeping, and a clamp toggle that
stays off in this run.

Ibrahim Hashimov raised the same accumulator gap on his "validate peer
CDC cursor" thread in July and accepted the Suggested-by I offered him
for the follow-up I had planned then. Your patch covers that
follow-up, so I'm passing it on; your call:

https://lore.kernel.org/all/20260724072117.73038-1-security@auditcode.ai/

Of the two changes of mine you planned to rebase on, "net/smc:
unregister the connection before draining the rx tasklet" is in
mainline (36cdf5d48ca1), and "net/smc: order the CDC receive path
against buffer publication" is not merged; its last posting is v4:

https://lore.kernel.org/all/20260728-b4-disp-52ee4e7d-v4-1-0dda94b0f397@proton.me/

Thanks,
Bryam