Re: [PATCH 3/3] scsi: leapraid: check set_identify result in update_links

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: leapraid: UAF on card_phy after dropping topo_node_lock
- [High] scsi: leapraid: out-of-bounds array access via unvalidated phy_index
- [High] scsi: leapraid: lockless traversal of card_port_list causes UAF
--

commit 4abc139d948d5c745212e888718ff5b93bc978e0
Author: Shirong Zhao <shxzhaosr@163.com>

scsi: leapraid: check set_identify result in update_links

leapraid_transport_update_links() ignores the return value of
leapraid_transport_set_identify(), which leaves the identify struct
untouched on FW query failure. Fold the call into the if condition so a
failed query takes the existing zeroing else path instead of attaching
with stale data.

> diff --git a/drivers/scsi/leapraid/leapraid_transport.c b/drivers/scsi/leapraid/leapraid_transport.c
> index 77ffbd6ddce4c..38e63327d89a1 100644
> --- a/drivers/scsi/leapraid/leapraid_transport.c
> +++ b/drivers/scsi/leapraid/leapraid_transport.c
> @@ -1150,9 +1150,9 @@ void leapraid_transport_update_links(
>  	card_phy->attached_hdl = hdl;

[Severity: High]
Does this code overflow the topo_node->card_phy array in
leapraid_transport_update_links() in drivers/scsi/leapraid/leapraid_transport.c?

Callers like leapraid_sas_pd_add() and leapraid_scan_sas_dev_after_reset()
pass the hardware-provided sas_dev_p0.phy_num directly as phy_index without
validating it against topo_node->phys_num. A malicious or malfunctioning
HBA returning an unexpectedly large phy_num could cause an out-of-bounds
write on card_phy->attached_hdl here.

>  	spin_unlock_irqrestore(&adapter->dev_topo.topo_node_lock, flags);
>  
> -	if (hdl && link_rate >= LEAPRAID_SAS_NEG_LINK_RATE_1_5) {
> -		leapraid_transport_set_identify(adapter, hdl,
> -						&card_phy->remote_identify);
> +	if (hdl && link_rate >= LEAPRAID_SAS_NEG_LINK_RATE_1_5 &&
> +	    !leapraid_transport_set_identify(adapter, hdl,
> +					     &card_phy->remote_identify)) {

[Severity: High]
Can this dereference a freed card_phy pointer?

By dropping adapter->dev_topo.topo_node_lock before calling the
blocking hardware query leapraid_transport_set_identify(), the safe lifetime
of topo_node and card_phy ends.

Concurrently, teardown paths like leapraid_exp_rm() can acquire the
topo_node_lock, remove the node, drop the lock, and call kfree() on both
topo_node_exp->card_phy and topo_node_exp. When leapraid_transport_set_identify()
executes, it would write to the freed card_phy->remote_identify.

>  		if (topo_node->hdl <= adapter->dev_topo.card.phys_num &&
>  		    adapter->adapter_attr.enable_mp) {
>  			list_for_each_entry(card_port,

[Severity: High]
Is it safe to iterate adapter->dev_topo.card_port_list without holding
the topo_node_lock?

Since the lock was dropped earlier in this function, this list is traversed
locklessly. Concurrently, leapraid_cleanup_card_port_and_vphys() can be
called (e.g. from leapraid_transport_port_remove()), which removes and
calls kfree() on elements from this same list, potentially causing a
use-after-free in this iterator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009102313.18-1-shxzhaosr@163.com?part=3