scsi: ufs: core: Fix task management completion timeout race

__ufshcd_issue_tm_cmd() clears req->end_io_data after timing out, which races with the completion function ufshcd_tmc_handler() which expects req->end_io_data to have a value.

Note __ufshcd_issue_tm_cmd() and ufshcd_tmc_handler() are already synchronized using hba->tmf_rqs and hba->outstanding_tasks under the host_lock spinlock.

It is also not necessary (nor typical) to clear req->end_io_data because the block layer does it before allocating out requests e.g. via blk_get_request().

So fix by not clearing it.

Fixes f5ef336fd2e4c3 ("scsi: ufs: core: Fix task management completion")

Signed-off-by Adrian Hunter <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c | 5 -----
  1 file changed, 5 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index 95be7ecdfe10..f34b3994d1aa 100644
@@ -6550,11 +6550,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
  	err = wait_for_completion_io_timeout(&wait,

Quoted 30 lines · Adrian Hunter

__ufshcd_issue_tm_cmd() clears req->end_io_data after timing out, which races with the completion function ufshcd_tmc_handler() which expects req->end_io_data to have a value.

Note __ufshcd_issue_tm_cmd() and ufshcd_tmc_handler() are already synchronized using hba->tmf_rqs and hba->outstanding_tasks under the host_lock spinlock.

It is also not necessary (nor typical) to clear req->end_io_data because the block layer does it before allocating out requests e.g. via blk_get_request().

So fix by not clearing it.

Fixes f5ef336fd2e4c3 ("scsi: ufs: core: Fix task management completion")

Signed-off-by Adrian Hunter <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c | 5 -----

  1 file changed, 5 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -2

index 95be7ecdfe10..f34b3994d1aa 100644
@@ -6550,11 +6550,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
       err = wait_for_completion_io_timeout(&wait,
               msecs_to_jiffies(TM_CMD_TIMEOUT));
       if (!err) {
-        /*
-         * Make sure that ufshcd_compl_tm() does not trigger a

drivers/scsi/ufs/ufshcd.c static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, · line 6555

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +0 -3

-         * use-after-free.
-         */
-        req->end_io_data = NULL;
           ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_ERR);
           dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
                   __func__, tm_function);

With this patch applied ufshcd_tmc_handler() can trigger a use-after-free of the stack memory used for the 'wait' completion.

Quoted 3 lines · Bart Van Assche

Wouldn't it be better to keep the code that clears req->end_io_data and to change complete(c) into if(c) complete(c) in ufshcd_tmc_handler()?

If that were needed, it would imply the synchronization was broken i.e. why are we referencing a request that has already been through blk_put_request()?

Quoted 8 lines · Adrian Hunter

Quoted 3 lines · Bart Van Assche

Wouldn't it be better to keep the code that clears req->end_io_data and to change complete(c) into if(c) complete(c) in ufshcd_tmc_handler()?

If that were needed, it would imply the synchronization was broken i.e. why are we referencing a request that has already been through blk_put_request()?

The scenario I'm worried about is as follows:
* __ufshcd_issue_tm_cmd() issues a task management function.
* No completion is received before TM_CMD_TIMEOUT has expired (100 ms).
* ufshcd_clear_tm_cmd() fails.
* The TMF completes, ufshcd_tmc_handler() is called and that function calls complete(req->end_io_data).

Can this happen?

I agree that this scenario involves completion of a request that has already been through blk_put_request().

Thanks,

Bart.

Quoted 16 lines · Bart Van Assche

Quoted 8 lines · Adrian Hunter

Quoted 3 lines · Bart Van Assche

Wouldn't it be better to keep the code that clears req->end_io_data and to change complete(c) into if(c) complete(c) in ufshcd_tmc_handler()?

If that were needed, it would imply the synchronization was broken i.e. why are we referencing a request that has already been through blk_put_request()?

The scenario I'm worried about is as follows:
* __ufshcd_issue_tm_cmd() issues a task management function.
* No completion is received before TM_CMD_TIMEOUT has expired (100 ms).
* ufshcd_clear_tm_cmd() fails.
* The TMF completes, ufshcd_tmc_handler() is called and that function calls complete(req->end_io_data).

Quoted 1 line

Can this happen?

No because the tag's bit is cleared from outstanding_tasks before blk_put_request() and access to outstanding_tasks is protected by host_lock in both __ufshcd_issue_tm_cmd() and ufshcd_clear_tm_cmd().

Quoted 7 lines

I agree that this scenario involves completion of a request that has already been through blk_put_request().

Thanks,

Bart.

__ufshcd_issue_tm_cmd() clears req->end_io_data after timing out, which races with the completion function ufshcd_tmc_handler() which expects req->end_io_data to have a value.

Note __ufshcd_issue_tm_cmd() and ufshcd_tmc_handler() are already synchronized using hba->tmf_rqs and hba->outstanding_tasks under the host_lock spinlock.

It is also not necessary (nor typical) to clear req->end_io_data because the block layer does it before allocating out requests e.g. via blk_get_request().

So fix by not clearing it.

Fixes f5ef336fd2e4c3 ("scsi: ufs: core: Fix task management completion")

Signed-off-by Adrian Hunter <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c | 5 -----
  1 file changed, 5 deletions(-)

drivers/scsi/ufs/ufshcd.c +0 -0

index 95be7ecdfe10..f34b3994d1aa 100644
@@ -6550,11 +6550,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
  	err = wait_for_completion_io_timeout(&wait,