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

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