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

[sashiko-bot@kernel.org]

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