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

Pre-existing issues:
- [High] Out-of-bounds read of `qentry` in `smc_llc_save_add_link_rkeys`
- [High] Data race on `flow->qentry` leading to memory leak
--

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

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

This patch resolves a use-after-free vulnerability by changing the code to
detach the LLC queue entry instead of freeing it early. It explicitly
frees the entry on all exit paths to ensure safe memory handling.

> diff --git a/net/smc/smc_llc.c b/net/smc/smc_llc.c
> index aa6d83af55eda..36c254f257350 100644
> --- a/net/smc/smc_llc.c
> +++ b/net/smc/smc_llc.c
> @@ -1481,7 +1481,7 @@ int smc_llc_srv_add_link(struct smc_link *link,
>  	}
>  	add_llc = &qentry->msg.add_link;
>  	if (add_llc->hd.flags & SMC_LLC_FLAG_ADD_LNK_REJ) {
> -		smc_llc_flow_qentry_del(&lgr->llc_flow_lcl);
> +		smc_llc_flow_qentry_clr(&lgr->llc_flow_lcl);

[Severity: High]
This is a pre-existing issue, but does calling smc_llc_flow_qentry_clr() here
without holding llc_flow_lock create a data race that can leak memory?

If a remote peer sends an unexpected or duplicate LLC response,
smc_llc_rx_response() running in softirq context could assign a newly
allocated qentry:

net/smc/smc_llc.c:smc_llc_rx_response() {
    ...
    smc_llc_flow_qentry_set(&link->lgr->llc_flow_lcl, qentry);
    ...
}

Concurrently, the process context calls smc_llc_flow_qentry_clr(), which
clears the pointer without holding the lock:

net/smc/smc_llc.c:smc_llc_flow_qentry_clr() {
    ...
    flow->qentry = NULL;
    ...
}

Could this allow the softirq to assign a new qentry right after the process
context clears it? If so, when process context later calls
smc_llc_flow_stop() and overwrites the flow state via
memset(flow, 0, sizeof(*flow)), wouldn't that leak the newly allocated
72-byte qentry object?

>  		rc = -ENOLINK;
>  		goto out_err;
>  	}

[ ... ]

> @@ -1512,6 +1513,7 @@ int smc_llc_srv_add_link(struct smc_link *link,
>  	rc = smc_llc_srv_conf_link(link, link_new, lgr_new_t);
>  	if (rc)
>  		goto out_err;
> +	kfree(qentry);

[Severity: High]
This isn't a bug introduced by this patch, but does the processing of
this qentry allocation cause an out-of-bounds read?

When an SMC V2 ADD LINK message is received and the link does not have a
shared v2 receive buffer, the pointer passed to smc_llc_save_add_link_rkeys()
points into the 72-byte qentry slab allocation.

Inside smc_llc_save_add_link_rkeys(), does this result in dereferencing
memory past the allocation?

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

Since ext->num_rkeys is read from an offset past the end of the slab object,
could this allow a remote attacker to manipulate the kernel into accepting
random slab data or adjacent heap data as RDMA tokens?

>  	kfree(ini);
>  	return 0;
>  out_err:

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