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

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

[Andrew Luebke]

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@vger.kernel.org
Signed-off-by: Andrew Luebke <andrew.luebke@gmail.com>
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@gmail.com/
    
    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(-)

diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.h b/drivers/scsi/aic7xxx/aic79xx_osm.h
index c6eb2b97e..1facfd238 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.h
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.h
@@ -501,6 +501,8 @@ int	ahd_linux_show_info(struct seq_file *,struct Scsi_Host *);
 static inline
 void ahd_cmd_set_transaction_status(struct scsi_cmnd *cmd, uint32_t status)
 {
+	if (!cmd)
+		return;
 	cmd->result &= ~(CAM_STATUS_MASK << 16);
 	cmd->result |= status << 16;
 }
@@ -514,6 +516,8 @@ void ahd_set_transaction_status(struct scb *scb, uint32_t status)
 static inline
 void ahd_cmd_set_scsi_status(struct scsi_cmnd *cmd, uint32_t status)
 {
+	if (!cmd)
+		return;
 	cmd->result &= ~0xFFFF;
 	cmd->result |= status;
 }
@@ -527,6 +531,8 @@ void ahd_set_scsi_status(struct scb *scb, uint32_t status)
 static inline
 uint32_t ahd_cmd_get_transaction_status(struct scsi_cmnd *cmd)
 {
+	if (!cmd)
+		return CAM_REQ_INPROG;
 	return ((cmd->result >> 16) & CAM_STATUS_MASK);
 }
 
@@ -539,6 +545,8 @@ uint32_t ahd_get_transaction_status(struct scb *scb)
 static inline
 uint32_t ahd_cmd_get_scsi_status(struct scsi_cmnd *cmd)
 {
+	if (!cmd)
+		return SAM_STAT_GOOD;
 	return (cmd->result & 0xFFFF);
 }
 
@@ -566,13 +574,16 @@ u_long ahd_get_transfer_length(struct scb *scb)
 static inline
 int ahd_get_transfer_dir(struct scb *scb)
 {
+	if (!scb->io_ctx)
+		return DMA_NONE;
 	return (scb->io_ctx->sc_data_direction);
 }
 
 static inline
 void ahd_set_residual(struct scb *scb, u_long resid)
 {
-	scsi_set_resid(scb->io_ctx, resid);
+	if (scb->io_ctx)
+		scsi_set_resid(scb->io_ctx, resid);
 }
 
 static inline
@@ -584,6 +595,8 @@ void ahd_set_sense_residual(struct scb *scb, u_long resid)
 static inline
 u_long ahd_get_residual(struct scb *scb)
 {
+	if (!scb->io_ctx)
+		return 0;
 	return scsi_get_resid(scb->io_ctx);
 }
 
@@ -597,11 +610,12 @@ static inline
 int ahd_perform_autosense(struct scb *scb)
 {
 	/*
-	 * We always perform autosense in Linux.
-	 * On other platforms this is set on a
-	 * per-transaction basis.
+	 * We always perform autosense in Linux, except for the device
+	 * reset SCB, which has no command to return sense data to.
 	 */
-	return (1);
+	if (!scb->io_ctx)
+		return 0;
+	return 1;
 }
 
 static inline uint32_t
@@ -631,6 +645,8 @@ void	ahd_platform_freeze_devq(struct ahd_softc *ahd, struct scb *scb);
 static inline void
 ahd_freeze_scb(struct scb *scb)
 {
+	if (!scb->io_ctx)
+		return;
 	if ((scb->io_ctx->result & (CAM_DEV_QFRZN << 16)) == 0) {
 		scb->io_ctx->result |= CAM_DEV_QFRZN << 16;
 		scb->platform_data->dev->qfrozen++;

base-commit: db8b4d2bf2d3e02b4ff78bbf69d00e11795840de
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: aic79xx: orphaned reset_scb when ahd_perform_autosense() returns 0
--

commit 1d9ef5977ae546b04b224d841fbb2babca4f0e30
Author: Andrew Luebke <andrew.luebke@gmail.com>

scsi: aic79xx: Fix NULL pointer dereference in device reset

This commit adds NULL checks to the command and SCB accessor wrappers in
aic79xx_osm.h so that device reset SCBs created without an associated
scsi_cmnd do not trigger NULL pointer dereferences. It also changes
ahd_perform_autosense() to return 0 when scb->io_ctx is NULL.

> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.h b/drivers/scsi/aic7xxx/aic79xx_osm.h
> index c6eb2b97e929c..1facfd238f895 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_osm.h
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.h

[ ... ]

> @@ -597,11 +610,12 @@ static inline
>  int ahd_perform_autosense(struct scb *scb)
>  {
>  	/*
> -	 * We always perform autosense in Linux.
> -	 * On other platforms this is set on a
> -	 * per-transaction basis.
> +	 * We always perform autosense in Linux, except for the device
> +	 * reset SCB, which has no command to return sense data to.
>  	 */
> -	return (1);
> +	if (!scb->io_ctx)
> +		return 0;
> +	return 1;
>  }

[Severity: Medium]
Does returning 0 here leave reset_scb uncompleted and orphaned on
ahd->pending_scbs when a target responds to a device reset with
SAM_STAT_CHECK_CONDITION or SAM_STAT_COMMAND_TERMINATED?

In drivers/scsi/aic7xxx/aic79xx_core.c, ahd_handle_scsi_status() is called
from ahd_run_qoutfifo() -> ahd_complete_scb() after the sequencer has
already completed the SCB and popped it from qoutfifo. When
ahd_perform_autosense(scb) returns 0, the SAM_STAT_CHECK_CONDITION /
SAM_STAT_COMMAND_TERMINATED case breaks out of the switch without
requeuing scb or calling ahd_done(ahd, scb):

drivers/scsi/aic7xxx/aic79xx_core.c:ahd_handle_scsi_status() {
	...
	case SAM_STAT_COMMAND_TERMINATED:
	case SAM_STAT_CHECK_CONDITION:
		if ((scb->flags & SCB_SENSE) == 0) {
			...
			if (ahd_perform_autosense(scb) == 0)
				break;
			...
		}
		ahd_done(ahd, scb);
		break;
	...
}

Because ahd_done() is skipped, reset_scb remains on ahd->pending_scbs even
though it is no longer on the controller. This stalls ahd_linux_dev_reset()
in drivers/scsi/aic7xxx/aic79xx_osm.c for the full 5-second timeout and
causes ahd_reset_cmds_pending() to overcount hardware CMDS_PENDING.

Furthermore, if the reset was triggered via scsi_ioctl_reset() in
drivers/scsi/scsi_error.c with SG_SCSI_RESET_DEVICE |
SG_SCSI_RESET_NO_ESCALATE:

drivers/scsi/scsi_error.c:scsi_ioctl_reset() {
	...
	case SG_SCSI_RESET_DEVICE:
		rtn = scsi_try_bus_device_reset(scmd);
		if (rtn == SUCCESS || (val & SG_SCSI_RESET_NO_ESCALATE))
			break;
	...
}

error recovery does not escalate to a bus reset and ahd_abort_scbs() never
runs. Doesn't that permanently leak reset_scb and leave dev->active
incremented and dev->openings decremented from ahd_linux_dev_reset(),
which also causes subsequent ahd_suspend() calls to fail with -EBUSY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023170330.269844-1-andrew.luebke@gmail.com?part=1

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@vger.kernel.org
> Signed-off-by: Andrew Luebke <andrew.luebke@gmail.com>
> 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@gmail.com/
>      
>      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@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.com                               +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