Re: [PATCH net v6 1/3] net/smc: fix use-after-free of the LLC qentry in smc_llc_srv_add_link()

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

Pre-existing issues:
- [High] Out-of-bounds memory read occurs when processing SMC-R V2 ADD_LINK messages on interfaces with max_recv_sge == 1, accessing V2 extension fields beyond the allocated size of the queue entry.
--

--- Patch [1]: [PATCH net v6 1/3] net/smc: fix use-after-free of the LLC qentry in smc_llc_srv_add_link() ---
commit b6d39643c17322840e48f9be07db4e2efaa719d8
Author: Yehyeong Lee <[email protected]>

net/smc: fix use-after-free of the LLC qentry in smc_llc_srv_add_link()

smc_llc_srv_add_link() keeps add_llc pointing into the queue entry:

  add_llc = &qentry->msg.add_link;                      smc_llc.c:1482
  ...
  smc_llc_save_add_link_info(link_new, add_llc);        smc_llc.c:1494
  smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);          smc_llc.c:1495
  ...
  u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
        (u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;        smc_llc.c:1504
  smc_llc_save_add_link_rkeys(link, link_new, llc_msg); smc_llc.c:1506

smc_llc_flow_qentry_del() kfree()s the entry, so on a link without a shared
v2 receive buffer the pointer handed to smc_llc_save_add_link_rkeys() is
already freed.  Before the Fixes: commit that branch always used
lgr->wr_rx_buf_v2 and add_llc was not used after the free.

[ ... ]

The offset is past the 72-byte queue entry because the out-of-bounds read
fixed by the next patch is on the same line; what this patch removes is the
free at smc_llc_srv_add_link+0x9a8 happening before the read at +0xaa2.

Detach the entry instead of freeing it there, and free it at the single
exit label.  The reject path has to detach as well, otherwise it would be
freed twice.

This changes only the lifetime of the entry.  The same read still runs past
its end until the next two patches bound it, so a backport wants all three.

Fixes: 27ef6a9981fe ("net/smc: support SMC-R V2 for rdma devices with max_recv_sge equals to 1")
Cc: [email protected]
Signed-off-by: Yehyeong Lee <[email protected]>

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index aa6d83af55eda..7282f8ad7593d 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
[ ... ]
> @@ -1492,7 +1492,8 @@ int smc_llc_srv_add_link(struct smc_link *link,
>  		lgr_new_t = SMC_LGR_ASYMMETRIC_PEER;
>  	}
>  	smc_llc_save_add_link_info(link_new, add_llc);
> -	smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
> +	/* add_llc still points into qentry, so only detach it here */
> +	smc_llc_flow_qentry_clr(&lgr->llc_flow_lcl);

[Severity: High]
This is a pre-existing issue, but does reading the V2 extension payload from
qentry result in an out-of-bounds read?

As noted in the commit message, this patch fixes a use-after-free on this
struct but explicitly leaves an out-of-bounds read to be fixed in subsequent
patches.

When a peer sends an SMC-R V2 ADD_LINK message with a V2 extension to an
interface where max_recv_sge == 1, smc_llc_srv_add_link() (and its client
equivalent smc_llc_cli_add_link()) does not use a shared V2 receive buffer.
Instead, it computes llc_msg + SMC_WR_TX_SIZE:

net/smc/smc_llc.c:smc_llc_save_add_link_rkeys() {
    ...
    ext = (struct smc_llc_msg_add_link_v2_ext *)(llc_msg +
                                                 SMC_WR_TX_SIZE);
    max = min_t(u8, ext->num_rkeys, SMC_LLC_RKEYS_PER_MSG_V2);
    ...
}

However, smc_llc_enqueue() allocates only enough space for the base structure
(including the 44-byte msg union):

net/smc/smc_llc.c:smc_llc_enqueue() {
    ...
    qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
    ...
    memcpy(&qentry->msg, llc, sizeof(union smc_llc_msg));
    ...
}

This truncates any received V2 extension payload during copying. Since
SMC_WR_TX_SIZE is 44 bytes, the ext pointer is placed out of bounds of the
qentry structure, causing ext->num_rkeys to read uninitialized slab memory or
KASAN redzones.

Since this can be triggered remotely by a network peer over the RoCE fabric,
is there a concern that applying this patch without the subsequent fixes
leaves a remotely reachable out-of-bounds read?

>  
>  	rc = smc_ib_ready_link(link_new);
>  	if (rc)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1