Re: [PATCH v2] scsi: aic79xx: Fix NULL pointer dereference in device reset

[Hannes Reinecke]

On 10/23/26 7:03 PM, Andrew Luebke wrote:
> Commit c67e63800446 ("scsi: aic79xx: Do not reference SCSI command when
> resetting device") makes ahd_linux_dev_reset() build its TARGET RESET
> SCB with io_ctx set to NULL, but the function still calls
> ahd_set_residual(reset_scb, 0) a few lines later, which is
> scsi_set_resid() on the NULL command. Every call of the device reset
> handler therefore oopses and kills the SCSI error handler thread; all
> further I/O on that host hangs until the machine is power cycled.
>
> Hit on an Intel SE7501WV2S board (on-board AIC-7902, i686, v7.2.9) when
> a failing Ultra320 disk made the error handler escalate to a device
> reset. Both disks on that channel then stayed in D state:
>
>    sd 1:0:0:0: [sda] tag#287 Attempting to queue a TARGET RESET message:
>    BUG: kernel NULL pointer dereference, address: 0000009c
>    #PF: supervisor write access in kernel mode
>    EIP: ahd_linux_dev_reset.cold+0xee/0x26e
>    Call Trace:
>     scsi_eh_ready_devs+0x538/0x8d0
>     scsi_error_handler+0x3a2/0x3b0
>     kthread+0xd2/0x120
>    note: scsi_eh_1[78] exited with irqs disabled
>
> (0x9c is resid_len in struct scsi_cmnd on i386.)
>
> Making that call safe is not enough on its own. When the target accepts
> the reset, ahd_handle_nonpkt_busfree() (or, in packetized mode, the task
> management completion) calls ahd_handle_devreset(), and its
> ahd_abort_scbs() walks the target's pending SCBs - the reset SCB
> included - reading and setting their transaction status and freezing
> them through the wrappers in aic79xx_osm.h, which dereference io_ctx.
> The same walk runs if the reset times out and the error handler
> escalates to a bus reset.
>
> Commit c7f4c5dec651 ("scsi: aic79xx: Fix up NULL command in ahd_done()")
> taught ahd_done() about the NULL command, but not these wrappers.
>
> Let every accessor that dereferences the command accept an SCB without
> one. The ahd_cmd_*() helpers for the transaction status and the SCSI
> status check for a NULL command: the transaction status then reads as
> CAM_REQ_INPROG and the SCSI status as SAM_STAT_GOOD, and setting either
> does nothing. ahd_get_transfer_dir(), ahd_get_residual(),
> ahd_set_residual() and ahd_freeze_scb() have no such helper underneath,
> so they check scb->io_ctx themselves: the transfer direction reads as
> DMA_NONE and the residual as 0, while setting the residual and freezing
> the SCB do nothing. The ahd_set_residual(reset_scb, 0) call in
> ahd_linux_dev_reset() is then harmless, and the reset SCB completes
> through ahd_done() as that commit intended. The SCSI status, residual
> and transfer direction accessors get the check so that none of them can
> be handed the reset SCB and crash; the only path from the reset SCB to
> them that is apparent is ahd_handle_scsi_status(), should a packetized
> LUN RESET end in a status error.
>
> ahd_perform_autosense() also declines autosense for an SCB without a
> command. Otherwise a target that answered the reset with CHECK
> CONDITION would have the reset SCB requeued for REQUEST SENSE, and with
> negotiation pending ahd_handle_scsi_status() clears SCB_DEVICE_RESET on
> the way, so ahd_done() would later dereference the missing command.
> Declining leaves such a reset SCB to the reset timeout and the error
> handler's escalation, which completes it through ahd_abort_scbs().
>
> Tested on the SE7501WV2S with this patch applied to v7.2.11 (i686), on
> the disks of its second AIC-7902 channel (160 MB/s DT, not packetized).
> Device resets sent with SG_SCSI_RESET_DEVICE to a Seagate ST373453LW log
> "Bus Device Reset Message Sent" and "Device reset returning 0x2002"
> (SUCCESS), three of them while the disk was being read; the reads
> complete, scsi_eh_1 survives and the disk keeps working. scsi_eh_1 also
> survives a device reset of the channel's other disk - the one behind the
> crash above, which accepts the reset but no longer spins up - and a bus
> reset sent with SG_SCSI_RESET_BUS, after which the first disk still
> works. The device resets go through ahd_handle_devreset() and
> ahd_abort_scbs(), so they exercise the transaction status and freeze
> accessors on the reset SCB. Not tested: the packetized (IU) LUN RESET
> path, a device reset that times out and escalates to a bus reset, and
> the autosense change, since a target here answers TARGET RESET with a
> bus free as SCSI requires. The oops was not reproduced on purpose on an
> unpatched kernel; the crash above came from the failing disk.
>
> Fixes: c67e63800446 ("scsi: aic79xx: Do not reference SCSI command when resetting device")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Andrew Luebke <andrew.luebke@xxxxxxxxx>
> Assisted-by: LLM
> ---
>
> Notes:
>      v2:
>       - Keep ahd_set_residual(reset_scb, 0) in ahd_linux_dev_reset(): with
>         the check in ahd_set_residual() the call is safe, and the patch now
>         touches only aic79xx_osm.h (Hannes).
>       - Move the NULL checks for the transaction status and the SCSI status
>         into ahd_cmd_set_transaction_status(), ahd_cmd_set_scsi_status() and
>         the two ahd_cmd_get_*() helpers (Hannes). ahd_set_residual() keeps
>         its check in the SCB wrapper: it calls the midlayer's
>         scsi_set_resid() directly, with no ahd_cmd_*() helper underneath.
>         The same goes for ahd_get_residual(), ahd_get_transfer_dir() and
>         ahd_freeze_scb().
>       - ahd_perform_autosense() returns 0 for an SCB without a command, so a
>         device reset that ends in CHECK CONDITION is not requeued for REQUEST
>         SENSE with SCB_DEVICE_RESET cleared (found by the Sashiko AI review of
>         v1).
>      v1: https://patchwork.kernel.org/project/linux-scsi/patch/20261020221530.2606178-1-andrew.luebke@xxxxxxxxx/
>      
>      This analysis, the patch and this changelog were prepared with an LLM coding
>      assistant (Claude Opus 5.5, Anthropic) at my direction; I reviewed them and
>      ran the hardware tests described above. Build-tested with W=1 for
>      x86_64 and i386 on mkp/scsi.git for-next (db8b4d2bf2d3): no new warnings;
>      checkpatch clean. sparse and smatch were not run.
>
>   drivers/scsi/aic7xxx/aic79xx_osm.h | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
Sigh. It is a really bad sign if the patch description is far longer 
than the patch itself. Might be okay for really intricate things, but
adding NULL checks to functions certainly is not.
But this doesn't change the validity of the patch, so:

Reviewed-by: Hannes Reinecke <hare@xxxxxxxxxx>

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxxx                               +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich