Re: [PATCH net v7 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 read of flexible array `body` in `smc_llc_save_add_link_rkeys()` because `qentry` is allocated without space for the LLC message extension.
--

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

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

This commit fixes a use-after-free of the LLC queue entry by detaching the
entry instead of freeing it early. The free is moved to the exit label.

Cc: [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 isn't a bug introduced by this patch, but does this allocation
and usage pattern result in an out-of-bounds read?

The commit message mentions an out-of-bounds read is fixed by a later
patch, but since it is present here, could this allocation in
smc_llc_enqueue() truncate the message?

net/smc/smc_llc.c:smc_llc_enqueue()
    qentry = kmalloc_obj(*qentry, GFP_ATOMIC);

This allocates the base size without space for the flexible array body.
Later in smc_llc_srv_add_link() when max_recv_sge == 1, it passes this
truncated queue entry downward:

net/smc/smc_llc.c:smc_llc_srv_add_link()
    u8 *llc_msg = smc_link_shared_v2_rxbuf(link) ?
        (u8 *)lgr->wr_rx_buf_v2 : (u8 *)add_llc;
    smc_llc_save_add_link_rkeys(link, link_new, llc_msg);

When smc_link_shared_v2_rxbuf is false, add_llc is used. Inside
smc_llc_save_add_link_rkeys(), it appears to calculate the extension past
the truncated bounds to access the number of routing keys:

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);

Can this lead to reading uninitialized or adjacent slab memory, resulting
in KASAN crashes or the registration of garbage memory as RDMA keys?

>  
>  	rc = smc_ib_ready_link(link_new);
>  	if (rc)
[ ... ]

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