Re: [RFC]usb: uas: cancel outstanding commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [High] usb: uas: double free of killer URB on bailout in
  uas_abort_complete()
- [High] usb: uas: NULL dereference on cmdinfo->killer in
  uas_abort_complete()
- [High] usb: uas: stack waitqueue corruption on futile path in
  uas_eh_abort_handler()
- [High] usb: uas: false SUCCESS return on URB error wakeup in
  uas_eh_abort_handler()
- [High] usb: uas: leak of unsubmitted URBs and unstopped work in
  uas_eh_abort_handler()
- [High] usb: uas: premature tag release and missing active check in
  uas_eh_abort_handler()
- [High] usb: uas: undersized dummy buffer and IU misrouting in
  uas_abort_complete()
- [High] usb: uas: uas_zap_pending() clears COMMAND_ABORTED on EH-owned
  commands
- [Medium] usb: uas: cleanly_cancelled set on RC_TMF_SUCCEEDED instead
  of IU_ID_STATUS
--

commit 40b90ce0c743eab1a72ea58e353c48af3de8be9d
Author: Oliver Neukum <oneukum@suse.com>

usb: uas: cancel outstanding commands

This commit implements command abort handling in uas_eh_abort_handler() by
submitting a duplicate TEST_UNIT_READY command with the same tag to force
an overlapped-tag error on the device.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..99ded579349a3 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
[ ... ]
> @@ -166,7 +170,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
>  		cmdinfo = scsi_cmd_priv(cmnd);
>  		uas_log_cmd_state(cmnd, __func__, 0);
>  		/* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
> -		cmdinfo->state &= ~COMMAND_INFLIGHT;
> +		cmdinfo->state &= ~(COMMAND_INFLIGHT | COMMAND_ABORTED);
>  		cmnd->result = result << 16;
>  		err = uas_try_complete(cmnd, __func__);
>  		WARN_ON(err != 0);

[Severity: High]
Can clearing COMMAND_ABORTED here cause a double completion of cmnd and a
double free of its unsubmitted URBs?

Once a command enters uas_eh_abort_handler(), it is owned by SCSI error
recovery and COMMAND_ABORTED is set so uas_try_complete() will not call
uas_free_unsubmitted_urbs() and scsi_done().  If uas_disconnect() or
uas_eh_host_reset_handler() runs while uas_eh_abort_handler() is still
active on cmnd (for instance, before devinfo->cmnd[tag - 1] is cleared),
uas_zap_pending() clears COMMAND_ABORTED and completes cmnd via
uas_try_complete() while uas_eh_abort_handler() and SCSI EH still own it.

> @@ -274,6 +278,7 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
>  
>  static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *cmnd)
>  {
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
>  	u8 response_code = riu->response_code;
>  
>  	switch (response_code) {
> @@ -282,10 +287,17 @@ static bool uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *
>  		break;
>  	case RC_TMF_SUCCEEDED:
>  		set_host_byte(cmnd, DID_OK);
> +		/* in case we win the race */
> +		cmdinfo->cleanly_cancelled = 1;
>  		break;
>  	case RC_TMF_NOT_SUPPORTED:
>  		set_host_byte(cmnd, DID_BAD_TARGET);
>  		break;
> +	case RC_OVERLAPPED_TAG:
> +		set_host_byte(cmnd, DID_ABORT);
> +		cmdinfo->cleanly_cancelled = 1;
> +		wake_up_all(&cmdinfo->cancel);
> +		break;

[Severity: Medium]
Should cmdinfo->cleanly_cancelled be set in the IU_ID_STATUS case of
uas_stat_cmplt() instead of RC_TMF_SUCCEEDED?

uas_evaluate_response_iu() only runs for IU_ID_RESPONSE (Task Management
response IUs), which the uas driver never sends.  When the command being
aborted finishes normally before the duplicate tag arrives, it completes
through IU_ID_STATUS in uas_stat_cmplt(), leaving cleanly_cancelled unset
and cmdinfo->cancel unwoken.

[ ... ]
> @@ -730,35 +743,197 @@ static enum scsi_qc_status uas_queuecommand_lck(struct scsi_cmnd *cmnd)
>  static DEF_SCSI_QCMD(uas_queuecommand)
>  
>  /*
> - * For now we do not support actually sending an abort to the device, so
> - * this eh always fails. Still we must define it to make sure that we've
> - * dropped all references to the cmnd in question once this function exits.
> + * We abort a command by reusing the tag, thus forcing an error
>   */
> +
> +static struct command_iu *allocate_double_command(int tag, struct scsi_device *sdev)
> +{
> +	struct command_iu *test_unit_ready;
> +
> +	test_unit_ready = kzalloc(sizeof(*test_unit_ready), GFP_ATOMIC);
> +	if (!test_unit_ready)
> +		return NULL;
> +
> +	test_unit_ready->iu_id = IU_ID_COMMAND;
> +	test_unit_ready->tag = cpu_to_be16(tag);
> +	test_unit_ready->prio_attr = UAS_SIMPLE_TAG;
> +	int_to_scsilun(sdev->lun, &test_unit_ready->lun);
> +	/*
> +	 * We need something that does not do harm
> +	 * if the command to be aborted does finish
> +	 * regularly
> +	 */
> +	test_unit_ready->cdb[0] = TEST_UNIT_READY;
> +
> +	return test_unit_ready;
> +}
> +
> +static void uas_dummy_sense(struct urb *u)
> +{
> +	struct scsi_cmnd *cmnd = u->context;
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
> +	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	int tag = cmdinfo->uas_tag;
> +	unsigned long flags;
> +	/*
> +	 * An error implies success
> +	 */
> +	int status = u->status;
> +
> +	/*
> +	 * The tag must be freed in every case
> +	 */
> +	spin_lock_irqsave(&devinfo->lock, flags);
> +	devinfo->cmnd[tag - 1] = NULL;
> +	spin_unlock_irqrestore(&devinfo->lock, flags);
> +
> +	kfree(u->transfer_buffer);
> +	wake_up_all(&cmdinfo->cancel);
> +	dev_info(&u->dev->dev, "Dummy sense completed with status %d\n", status);
> +}
> +
> +static void uas_abort_complete(struct urb *u)
> +{
> +	int status = u->status;
> +	struct scsi_cmnd *cmnd = u->context;
> +	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
> +	struct usb_device *udev = devinfo->udev;
> +	struct response_iu *dummy;
> +	int tag = cmdinfo->uas_tag;
> +	unsigned long flags;
> +	int rv;
> +
> +	switch (status) {
> +	default:
> +		/*
> +		 * immediate cleanup
> +		 */
> +		kfree(u->transfer_buffer);
> +		spin_lock_irqsave(&devinfo->lock, flags);
> +		devinfo->cmnd[tag - 1] = NULL;
> +		spin_unlock_irqrestore(&devinfo->lock, flags);
> +		wake_up_all(&cmdinfo->cancel);
> +		break;
> +	case 0:
> +		kfree(u->transfer_buffer);
> +		dummy = kzalloc(sizeof(*dummy), GFP_ATOMIC);
> +		if (!dummy) {
> +			spin_lock_irqsave(&devinfo->lock, flags);
> +			goto bailout;
> +		}
> +		usb_fill_bulk_urb(cmdinfo->killer, udev, devinfo->status_pipe,
> +				dummy, sizeof(*dummy), uas_dummy_sense, cmnd);

[Severity: High]
Isn't struct response_iu (8 bytes) too small for the status_pipe transfer
here?

allocate_double_command() sends a TEST_UNIT_READY Command IU.  If the
original command finishes just before TEST_UNIT_READY reaches the device,
the device executes TEST_UNIT_READY as a normal command and returns a
struct sense_iu (at least 16 bytes) on status_pipe, overflowing the 8-byte
buffer with -EOVERFLOW.

In addition, on USB 2.0 (!devinfo->use_streams), all tags share a single
FIFO queue on status_pipe without stream IDs, and uas_stat_cmplt() matches
completions by reading iu->tag.  Submitting a status URB with
uas_dummy_sense(), which ignores iu->tag and uses u->context, will blindly
consume whatever status or ready IU arrives next on status_pipe for any
other in-flight command.

> +		if (devinfo->use_streams)
> +			cmdinfo->killer->stream_id = cmdinfo->uas_tag;
> +
> +		spin_lock_irqsave(&devinfo->lock, flags);
> +		usb_anchor_urb(u, &devinfo->killer_urbs);
> +		rv = usb_submit_urb(cmdinfo->killer, GFP_ATOMIC);

[Severity: High]
Can cmdinfo->killer be NULL here when uas_abort_complete() races with
uas_eh_abort_handler()?

CPU0                              CPU1
uas_eh_abort_handler()            <softirq>
  schedule_timeout(HZ)            uas_stat_cmplt()
                                    uas_evaluate_response_iu()
                                      wake_up_all(&cmdinfo->cancel)
  spin_lock_irq(&devinfo->lock)
  cmdinfo->killer = NULL
  spin_unlock_irq(&devinfo->lock)
                                  uas_abort_complete(u)
                                    usb_fill_bulk_urb(cmdinfo->killer, ...)
                                      <-- NULL pointer dereference
  usb_kill_urb(killer)

Because RC_OVERLAPPED_TAG is delivered on the original sense URB on
status_pipe, uas_stat_cmplt() can wake uas_eh_abort_handler() before the
killer Command IU URB completion finishes running uas_abort_complete().
uas_eh_abort_handler() clears cmdinfo->killer to NULL before calling
usb_kill_urb(killer), while uas_abort_complete() uses cmdinfo->killer
instead of u.

> +		if (rv < 0) {
> +bailout:
> +			devinfo->cmnd[tag - 1] = NULL;
> +			spin_unlock_irqrestore(&devinfo->lock, flags);
> +			usb_unanchor_urb(u);
> +			kfree(dummy);
> +			usb_free_urb(u);

[Severity: High]
Does calling usb_free_urb(u) in the bailout path cause a use-after-free
and double free of killer?

killer is allocated in uas_eh_abort_handler() with one initial reference
and unconditionally freed at the end of uas_eh_abort_handler() via
usb_kill_urb(killer) and usb_free_urb(killer).  Dropping a reference here
in the completion callback (when kzalloc() fails or usb_submit_urb()
returns an error, such as when usb_kill_urb() sets killer->reject) causes
__usb_hcd_giveback_urb() to drop the last reference and free the URB before
uas_eh_abort_handler() kills and frees it again.  The bailout path also
misses wake_up_all(&cmdinfo->cancel), leaving uas_eh_abort_handler() to
sleep for the full 1-second timeout.

> +		} else {
> +			spin_unlock_irqrestore(&devinfo->lock, flags);
> +			dev_info(&u->dev->dev, "Submitted dummy tag %d\n", tag -1);
> +		}
> +		break;
> +	}
> +}
> +
>  static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
>  {
> +	DECLARE_WAITQUEUE(wait, current);
>  	struct uas_cmd_info *cmdinfo = scsi_cmd_priv(cmnd);
>  	struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
> +	struct usb_device *udev = devinfo->udev;
>  	struct urb *data_in_urb = NULL;
>  	struct urb *data_out_urb = NULL;
> -	unsigned long flags;
> +	struct urb *killer = NULL;
> +	struct command_iu *bullet = NULL;
> +	int target_tag, rv;
> +	int result = FAILED;
>  
> -	spin_lock_irqsave(&devinfo->lock, flags);
> +	killer = usb_alloc_urb(0, GFP_NOIO);
>  
> -	uas_log_cmd_state(cmnd, __func__, 0);
> +	set_current_state(TASK_UNINTERRUPTIBLE);
> +	add_wait_queue(&cmdinfo->cancel, &wait);
> +	spin_lock_irq(&devinfo->lock);
> +	if (devinfo->resetting)
> +		goto futile;
> +	target_tag = cmdinfo->uas_tag;
>  
>  	/* Ensure that try_complete does not call scsi_done */
>  	cmdinfo->state |= COMMAND_ABORTED;

[Severity: High]
What happens if cmnd completed normally just before uas_eh_abort_handler()
acquired devinfo->lock, or if the tag is reused as soon as the abort
callbacks clear devinfo->cmnd[tag - 1]?

First, if cmnd completed via uas_try_complete() before devinfo->lock was
taken, devinfo->cmnd[target_tag - 1] was cleared and uas_queuecommand_lck()
may have already assigned target_tag to a newly queued command.  Without
checking that devinfo->cmnd[target_tag - 1] == cmnd, uas_eh_abort_handler()
sends an overlapped tag for target_tag and clears the slot, aborting and
orphaning the new command.

Second, uas_dummy_sense() and uas_abort_complete() set
devinfo->cmnd[tag - 1] = NULL before uas_eh_abort_handler() kills
data_in_urb and data_out_urb, and the original sense URB in
devinfo->sense_urbs is never killed at all.  uas_queuecommand_lck() can
reassign the tag and stream ID to a new command while the old command's
data and sense URBs are still in flight, so when the old sense URB
completes in uas_stat_cmplt(), it looks up devinfo->cmnd[idx] by tag and
operates on the new command.

> -	/* Drop all refs to this cmnd, kill data urbs to break their ref */
> -	devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
> -	if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> -		data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> -	if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> -		data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
> -
> -	uas_free_unsubmitted_urbs(cmnd);
> +	if (!killer)
> +		goto admit_failure;
> +	cmdinfo->killer = killer;
> +	bullet = allocate_double_command(target_tag, cmnd->device);
> +	if (!bullet)
> +		goto admit_failure;
> +
> +	usb_fill_bulk_urb(killer, udev, devinfo->cmd_pipe,
> +			bullet, sizeof(*bullet), uas_abort_complete, cmnd);
> +
> +	cmdinfo->killer = killer;
> +
> +	usb_anchor_urb(killer, &devinfo->killer_urbs);
> +	rv = usb_submit_urb(killer, GFP_ATOMIC);
> +	if (rv < 0) {
> +admit_failure:
> +		/* Drop all refs to this cmnd, kill data urbs to break their ref */
> +		devinfo->cmnd[cmdinfo->uas_tag - 1] = NULL;
> +		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> +			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> +		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> +			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
>  
> -	spin_unlock_irqrestore(&devinfo->lock, flags);
> +		uas_free_unsubmitted_urbs(cmnd);
> +		spin_unlock_irq(&devinfo->lock);
> +		usb_unanchor_urb(killer);
> +		kfree(bullet);
> +	} else {
> +		spin_unlock_irq(&devinfo->lock);
> +		/*
> +		 * if something woke us up the command
> +		 * has been cancelled.
> +		 * If we do take the timeout we have lost
> +		 * the race against the hardware or
> +		 * the hardware is broken. If the
> +		 * command succeeded we can count this
> +		 * as success. Otherwise we've failed.
> +		 *
> +		 * We will kill the URB later, thereby
> +		 * freeing the tag
> +		 */
> +		rv = schedule_timeout(HZ);
> +		if (rv)
> +			result = SUCCESS;
> +		else
> +			result = cmdinfo->cleanly_cancelled ? SUCCESS : FAILED;

[Severity: High]
Does checking `if (rv)` cause uas_eh_abort_handler() to return SUCCESS
when an abort URB fails?

schedule_timeout(HZ) returns the remaining jiffies (rv > 0) whenever woken
before the timeout expires.  Both the error path in uas_abort_complete()
and uas_dummy_sense() call wake_up_all(&cmdinfo->cancel) on any URB error
(such as -EPROTO, -EPIPE, or -ENOENT when killed) without setting
cmdinfo->cleanly_cancelled.  Because rv is non-zero, uas_eh_abort_handler()
reports SUCCESS to SCSI EH even though the abort failed.

> +		spin_lock_irq(&devinfo->lock);
> +		if (cmdinfo->state & DATA_IN_URB_INFLIGHT)
> +			data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
> +		if (cmdinfo->state & DATA_OUT_URB_INFLIGHT)
> +			data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
> +		spin_unlock_irq(&devinfo->lock);
> +	}

[Severity: High]
Does this branch leak unsubmitted URBs when usb_submit_urb(killer)
succeeds?

Setting COMMAND_ABORTED prevents uas_try_complete() from calling
uas_free_unsubmitted_urbs(cmnd), and unlike the admit_failure branch, this
branch never calls uas_free_unsubmitted_urbs(cmnd) either.  Any allocated
URBs that were not yet submitted (such as data_in_urb or data_out_urb on
USB 2.0 before READ_READY/WRITE_READY, or URBs queued on devinfo->work) are
leaked.  In addition, because neither uas_do_work() nor the
IU_ID_READ_READY/IU_ID_WRITE_READY handlers in uas_stat_cmplt() check
COMMAND_ABORTED, they can submit URBs for the aborted command after this
lock is dropped.

> +	set_current_state(TASK_RUNNING);
> +	remove_wait_queue(&cmdinfo->cancel, &wait);
> +	/*
> +	 * we know a callback will free the tag
> +	 */
> +	spin_lock_irq(&devinfo->lock);
> +	cmdinfo->killer = NULL;
> +futile:
> +	spin_unlock_irq(&devinfo->lock);
> +	usb_kill_urb(killer);
> +	usb_free_urb(killer);

[Severity: High]
Does jumping to futile when devinfo->resetting is set leave the stack
waitqueue entry linked in cmdinfo->cancel and the task state set to
TASK_UNINTERRUPTIBLE?

set_current_state(TASK_UNINTERRUPTIBLE) and
add_wait_queue(&cmdinfo->cancel, &wait) run before checking
devinfo->resetting, while the futile label is placed after
set_current_state(TASK_RUNNING) and remove_wait_queue().  Taking the
futile jump returns from uas_eh_abort_handler() with stack-allocated wait
still on cmdinfo->cancel and current in TASK_UNINTERRUPTIBLE, and skips
setting COMMAND_ABORTED, freeing unsubmitted URBs, and killing in-flight
data URBs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112133926.878432-1-oneukum@suse.com?part=1