Re: [PATCH v3 10/57] scsi: aha152x: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting list corruption vulnerability due to unlocked accesses to DISCONNECTED_SC.
- [High] Possible NULL pointer dereference in SCSI phase handlers if CURRENT_SC is NULL due to unexpected target behavior.
- [High] Calling INIT_WORK dynamically in an interrupt handler for a global work_struct corrupts the workqueue.
- [High] The global `aha152x_host_list` is modified and traversed concurrently without any locking.
- [High] The driver fails to cancel the global workqueue during device release or module unload.
--

--- Patch [10]: [PATCH v3 10/57] scsi: aha152x: Prepare for enabling lock context analysis ---
commit 4e1483c06dfed36c67b1da76d5119c68317479aa
Author: Bart Van Assche <bvanassche@acm.org>

scsi: aha152x: Prepare for enabling lock context analysis

Annotate is_complete() with __context_unsafe() because it performs
conditional locking.

> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index e3ccb6bb62c00..c16dcb9274eb8 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -2319,6 +2319,7 @@ static void rsti_run(struct Scsi_Host *shpnt)
>   *
>   */
>  static void is_complete(struct Scsi_Host *shpnt)
> +	__context_unsafe(conditional locking)
>  {
>  	int dataphase;
>  	unsigned long flags;

[Severity: High]
This is a pre-existing issue, but is there a risk of list corruption when
accessing DISCONNECTED_SC?

In is_complete(), the host lock is dropped before calling phase handlers
such as busfree_run():

	DO_UNLOCK(flags);
	...
	if(states[STATE].run)
		states[STATE].run(shpnt);

Then busfree_run() mutates the list without holding the QLOCK:

	append_SC(&DISCONNECTED_SC, CURRENT_SC);

Concurrently, SCSI error handler callbacks like aha152x_abort() modify the
exact same list while holding the lock:

	disconnected = issued && remove_SC(&DISCONNECTED_SC, SCpnt);

Can this concurrent modification lead to list corruption?

[Severity: High]
This is a pre-existing issue, but could there be a NULL pointer dereference
if a target behaves unexpectedly?

In is_complete(), the state machine transitions to phases like status or cmd
based on hardware bus signals via update_state():

	switch(GETPORT(SCSISIG) & P_MASK) {
	case P_MSGI:    STATE=msgi;    break;
	...

This occurs without verifying if a command is currently active. The state
handlers then unconditionally dereference CURRENT_SC:

status_run():
	aha152x_priv(CURRENT_SC)->status = GETPORT(SCSIDAT);

cmd_init():
	if (aha152x_priv(CURRENT_SC)->sent_command) {

If the hardware reports a phase while no command is active, would this crash
the kernel?

[Severity: High]
This is a pre-existing issue, but does dynamically initializing the global
work item in the interrupt handler corrupt the workqueue?

The interrupt handler intr() calls INIT_WORK() before schedule_work():

	INIT_WORK(&aha152x_tq, run);
	schedule_work(&aha152x_tq);

Since aha152x_tq is a global variable, if a second SCSI host adapter
triggers an interrupt while the work is already queued by the first adapter,
would INIT_WORK() overwrite the list pointers of the pending work?

[Severity: High]
This is a pre-existing issue, but is it safe to traverse and modify the
global aha152x_host_list locklessly?

Nodes are added and removed from the list locklessly:

aha152x_probe_one():
	list_add_tail(&HOSTDATA(shpnt)->host_list, &aha152x_host_list);

aha152x_release():
	list_del(&HOSTDATA(shpnt)->host_list);

Concurrently, the workqueue callback run() iterates over the list locklessly:

	list_for_each_entry(hd, &aha152x_host_list, host_list) {

If aha152x_release() removes a host while the workqueue thread is traversing
the list, could this lead to following invalid pointers and a use-after-free?

[Severity: High]
This is a pre-existing issue, but could pending work items execute after
the module is unloaded or devices are freed?

Neither aha152x_release() nor aha152x_exit() appears to cancel the global
workqueue:

aha152x_release():
	void aha152x_release(struct Scsi_Host *shpnt)

aha152x_exit():
	static void __exit aha152x_exit(void)

If an interrupt schedules the work and the module is immediately unloaded,
will the kernel attempt to execute run() after the module's code segment
has been unmapped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785451174.git.bvanassche@acm.org?part=10