Re: [PATCH v2 10/17] mpi3mr: Fix out-of-bounds read in SAS topology change events

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

New issues:
- [High] Compiler-inserted bounds checks from __counted_by bypass the local caching of num_entries, defeating the TOCTOU mitigation and risking spurious kernel panics.

Pre-existing issues:
- [High] The bounds check aborts the top-half handler but fails to prevent the malformed event from reaching the bottom-half handler, resulting in a heap out-of-bounds read.
- [High] Lockless access of tgtdev->starget in event handlers leads to Use-After-Free racing with mpi3mr_target_destroy().
--

commit 9d8265fdf635644a7462d4ecd8e229ec48572ac3
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix out-of-bounds read in SAS topology change events

The number of entries in a SAS topology change event is used to walk
the entry array without being compared against the amount of event data
that was received, so the walk can run past the end of the buffer.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 9aeec6146acbb..6a80b784200b3 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2827,12 +2827,28 @@ static void mpi3mr_sastopochg_evt_th(struct mpi3mr_ioc *mrioc,
[ ... ]
> +	if (num_entries > max_entries) {
> +		ioc_err(mrioc, "SAS topology event: num_entries(%d) exceeds max(%d)\n",
> +			num_entries, max_entries);
> +		return;

[Severity: High]
This is a pre-existing issue, but returning early here aborts top-half
processing without clearing the caller's process_evt_bh flag.

In drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_os_handle_events(), process_evt_bh
is set to 1 before calling mpi3mr_sastopochg_evt_th(). If we return early,
the event is unconditionally scheduled for bottom-half processing.

Since the bottom-half blindly loops using the attacker-controlled num_entries
on the fwevt buffer:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_bh() {
    ...
    for (i = 0; i < event_data->num_entries; i++) {
    ...
}

Could this leave the bottom-half exposed to a heap out-of-bounds read when
processing a malformed event?

> +	}
> +
> +	for (i = 0; i < num_entries; i++) {
>  		handle = le16_to_cpu(topo_evt->phy_entry[i].attached_dev_handle);

[Severity: High]
Since phy_entry is annotated with __counted_by(num_entries), won't the
compiler-inserted bounds check for topo_evt->phy_entry[i] directly re-fetch
topo_evt->num_entries from DMA memory?

If the firmware alters topo_evt->num_entries to be less than the loop index i
during the loop in mpi3mr_sastopochg_evt_th(), wouldn't this bypass the local
num_entries cache and trigger an array bounds sanitizer violation panic?

[Severity: High]
This isn't a bug introduced by this patch, but further down in
mpi3mr_sastopochg_evt_th() there is a lockless access to tgtdev->starget
that can race with device removal:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sastopochg_evt_th() {
    ...
    if (tgtdev && tgtdev->starget && tgtdev->starget->hostdata)
        scsi_tgt_priv_data = (struct mpi3mr_stgt_priv_data *)
            tgtdev->starget->hostdata;
    ...
    case MPI3_EVENT_SAS_TOPO_PHY_RC_TARG_NOT_RESPONDING:
        if (scsi_tgt_priv_data) {
            scsi_tgt_priv_data->dev_removed = 1;
    ...
}

Is it possible that if this executes concurrently with mpi3mr_target_destroy(),
tgtdev->starget could be cleared under lock and the scsi_target freed by the
SCSI midlayer, causing a Use-After-Free when writing to
scsi_tgt_priv_data->dev_removed?

>  		if (!handle)
>  			continue;

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