Re: [PATCH 04/17] mpi3mr: Fix target device reference leak in BSG task management

On Tue, Aug 18, 2026 at 11:43 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Use-after-free of SCSI target private data (`stgt_priv`) during completion of BSG task management commands if the device is hot-removed.
> - [High] Unprotected lockless access to `tgtdev->starget` can lead to a NULL pointer dereference or Use-After-Free.
> - [High] Missing bounds validation on the BSG `request_len` allows an Out-of-Bounds read when accessing the variable-length buffer entry list.
> - [High] Little-endian `__le16` fields from the user request are assigned to CPU-endian `u16` variables without `le16_to_cpu()` conversion.
> --
>
> --- Patch [4]: [PATCH 04/17] mpi3mr: Fix target device reference leak in BSG task management ---
> (Note: The format of this review has been altered to a free-form summary due
> to automated recitation restrictions. The original patch diff is not quoted.)
>
> commit c103d1267a7a6a9328e7aab3dbe852bbbb68d3e6
> Author: Chandrakanth Patil <[email protected]>
>
> mpi3mr: Fix target device reference leak in BSG task management
>
> This commit fixes a reference leak for the target device during BSG task
> management command processing. The reference is now dropped whenever the
> lookup succeeds, regardless of whether the SCSI target data is fully setup.
>
> [Severity: High]
> This isn't a bug introduced by this patch, but there appears to be a use
> after free risk regarding the SCSI target private data. In
> mpi3mr_bsg_process_mpt_cmds(), the block_io counter is incremented on
> stgt_priv, and then the target device reference is dropped before sleeping
> via wait_for_completion_timeout(). If the target is hot-removed during this
> sleep, mpi3mr_target_destroy() will free starget->hostdata. Upon waking,
> does the atomic_dec() on stgt_priv->block_io write to freed memory?
>
> [Severity: High]
> This is a pre-existing issue, but the access to the target device's starget
> pointer seems unprotected. In mpi3mr_bsg_process_mpt_cmds(), tgtdev->starget
> is checked for NULL and then dereferenced locklessly to access hostdata.
> Since mpi3mr_target_destroy() can concurrently set tgtdev->starget to NULL
> under the tgtdev_lock, could this result in a NULL pointer dereference if
> the compiler emits multiple loads and the pointer becomes NULL between the
> check and the dereference?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but there might be an out of
> bounds read when iterating over the BSG buffer entries. In
> mpi3mr_bsg_process_mpt_cmds(), the loop reads buf_type and buf_len from the
> buf_entries list. Is there any validation that the number of entries
> provided by the user does not exceed the bounds of the allocated
> job->request_len? Could a small request length with a large entry count read
> arbitrary slab memory?
>
> [Severity: High]
> This is a pre-existing issue, but is there a missing endianness conversion
> when parsing the device handle? In mpi3mr_bsg_process_mpt_cmds(),
> tm_req->dev_handle is an __le16 field, but it is assigned directly to a u16
> local variable without le16_to_cpu(). Similarly,
> mpi_header->function_dependent is passed directly to mpi3mr_issue_tm(). On
> big-endian architectures, will this block IO on the wrong device and fail
> to block IO on the actual target of the task management command?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

Thanks for the review.

New issues:
- None. This patch specifically fixes a target device reference leak
in BSG task management by ensuring mpi3mr_tgtdev_put() is called
whenever tgtdev lookup succeeds.

Pre-existing issues:
- The reported issues (stgt_priv UAF, lockless starget access,
request_len bounds check, and dev_handle endianness conversion) are
pre-existing in the driver and will
  be addressed in a separate follow-up series.