[PATCH] scsi: core: Fix a use-after-free

[PATCH] scsi: core: Fix a use-after-free

[Bart Van Assche]

There are two .exit_cmd_priv implementations. Both implementations use
resources associated with the SCSI host. Make sure that these resources are
still available when .exit_cmd_priv is called by waiting inside
scsi_remove_host() until the tag set has been freed.

This patch fixes the following use-after-free:

==================================================================
BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
Read of size 8 at addr ffff888100337000 by task multipathd/16727
Call Trace:
 <TASK>
 dump_stack_lvl+0x34/0x44
 print_report.cold+0x5e/0x5db
 kasan_report+0xab/0x120
 srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
 scsi_mq_exit_request+0x4d/0x70
 blk_mq_free_rqs+0x143/0x410
 __blk_mq_free_map_and_rqs+0x6e/0x100
 blk_mq_free_tag_set+0x2b/0x160
 scsi_host_dev_release+0xf3/0x1a0
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 scsi_device_dev_release_usercontext+0x4c1/0x4e0
 execute_in_process_context+0x23/0x90
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 scsi_disk_release+0x3f/0x50
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 disk_release+0x17f/0x1b0
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 dm_put_table_device+0xa3/0x160 [dm_mod]
 dm_put_device+0xd0/0x140 [dm_mod]
 free_priority_group+0xd8/0x110 [dm_multipath]
 free_multipath+0x94/0xe0 [dm_multipath]
 dm_table_destroy+0xa2/0x1e0 [dm_mod]
 __dm_destroy+0x196/0x350 [dm_mod]
 dev_remove+0x10c/0x160 [dm_mod]
 ctl_ioctl+0x2c2/0x590 [dm_mod]
 dm_ctl_ioctl+0x5/0x10 [dm_mod]
 __x64_sys_ioctl+0xb4/0xf0
 dm_ctl_ioctl+0x5/0x10 [dm_mod]
 __x64_sys_ioctl+0xb4/0xf0
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x46/0xb0

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Li Zhijian <lizhijian@fujitsu.com>
Reported-by: Li Zhijian <lizhijian@fujitsu.com>
Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/hosts.c      | 16 +++++++++++++---
 drivers/scsi/scsi_lib.c   |  6 +++++-
 drivers/scsi/scsi_priv.h  |  2 +-
 drivers/scsi/scsi_scan.c  |  1 +
 drivers/scsi/scsi_sysfs.c |  1 +
 include/scsi/scsi_host.h  |  2 ++
 6 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 26bf3b153595..9857dba09c95 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -182,6 +182,15 @@ void scsi_remove_host(struct Scsi_Host *shost)
 	mutex_unlock(&shost->scan_mutex);
 	scsi_proc_host_rm(shost);
 
+	/*
+	 * New SCSI devices cannot be attached anymore because of the SCSI host
+	 * state so drop the tag set refcnt. Wait until the tag set refcnt drops
+	 * to zero because .exit_cmd_priv implementations may need the host
+	 * pointer.
+	 */
+	kref_put(&shost->tagset_refcnt, scsi_mq_free_tags);
+	wait_for_completion(&shost->tagset_freed);
+
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (scsi_host_set_state(shost, SHOST_DEL))
 		BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
@@ -245,6 +254,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	if (error)
 		goto fail;
 
+	kref_init(&shost->tagset_refcnt);
+	init_completion(&shost->tagset_freed);
+
 	/*
 	 * Increase usage count temporarily here so that calling
 	 * scsi_autopm_put_host() will trigger runtime idle if there is
@@ -317,6 +329,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	pm_runtime_disable(&shost->shost_gendev);
 	pm_runtime_set_suspended(&shost->shost_gendev);
 	pm_runtime_put_noidle(&shost->shost_gendev);
+	kref_put(&shost->tagset_refcnt, scsi_mq_free_tags);
  fail:
 	return error;
 }
@@ -350,9 +363,6 @@ static void scsi_host_dev_release(struct device *dev)
 		kfree(dev_name(&shost->shost_dev));
 	}
 
-	if (shost->tag_set.tags)
-		scsi_mq_destroy_tags(shost);
-
 	kfree(shost->shost_data);
 
 	ida_free(&host_index_ida, shost->host_no);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4dbd29ab1dcc..1f30e0c54e55 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1976,9 +1976,13 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 	return blk_mq_alloc_tag_set(tag_set);
 }
 
-void scsi_mq_destroy_tags(struct Scsi_Host *shost)
+void scsi_mq_free_tags(struct kref *kref)
 {
+	struct Scsi_Host *shost = container_of(kref, typeof(*shost),
+					       tagset_refcnt);
+
 	blk_mq_free_tag_set(&shost->tag_set);
+	complete(&shost->tagset_freed);
 }
 
 /**
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 429663bd78ec..f385b3f04d6e 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -94,7 +94,7 @@ extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern void scsi_requeue_run_queue(struct work_struct *work);
 extern void scsi_start_queue(struct scsi_device *sdev);
 extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
-extern void scsi_mq_destroy_tags(struct Scsi_Host *shost);
+extern void scsi_mq_free_tags(struct kref *kref);
 extern void scsi_exit_queue(void);
 extern void scsi_evt_thread(struct work_struct *work);
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 91ac901a6682..5d27f5196de6 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -340,6 +340,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 		kfree(sdev);
 		goto out;
 	}
+	kref_get(&sdev->host->tagset_refcnt);
 	sdev->request_queue = q;
 	q->queuedata = sdev;
 	__scsi_init_queue(sdev->host, q);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index aa70d9282161..5d61f58399dc 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1476,6 +1476,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	mutex_unlock(&sdev->state_mutex);
 
 	blk_mq_destroy_queue(sdev->request_queue);
+	kref_put(&sdev->host->tagset_refcnt, scsi_mq_free_tags);
 	cancel_work_sync(&sdev->requeue_work);
 
 	if (sdev->host->hostt->slave_destroy)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b6e41ee3d566..9b0a028bf053 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -557,6 +557,8 @@ struct Scsi_Host {
 	struct scsi_host_template *hostt;
 	struct scsi_transport_template *transportt;
 
+	struct kref		tagset_refcnt;
+	struct completion	tagset_freed;
 	/* Area to keep a shared tag map */
 	struct blk_mq_tag_set	tag_set;
 

Re: [PATCH] scsi: core: Fix a use-after-free

[Ming Lei]

On Thu, Aug 25, 2022 at 05:26:34PM -0700, Bart Van Assche wrote:
> There are two .exit_cmd_priv implementations. Both implementations use
> resources associated with the SCSI host. Make sure that these resources are
> still available when .exit_cmd_priv is called by waiting inside
> scsi_remove_host() until the tag set has been freed.
> 
> This patch fixes the following use-after-free:
> 
> ==================================================================
> BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
> Read of size 8 at addr ffff888100337000 by task multipathd/16727
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x34/0x44
>  print_report.cold+0x5e/0x5db
>  kasan_report+0xab/0x120
>  srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>  scsi_mq_exit_request+0x4d/0x70
>  blk_mq_free_rqs+0x143/0x410
>  __blk_mq_free_map_and_rqs+0x6e/0x100
>  blk_mq_free_tag_set+0x2b/0x160
>  scsi_host_dev_release+0xf3/0x1a0

The trace must be triggered on old kernel, cause this issue is fixed by
upstream since commit f323896fe6fa ("scsi: core: Call blk_mq_free_tag_set() earlier")
from you, :-)


Thanks,
Ming

Re: [PATCH] scsi: core: Fix a use-after-free

[Bart Van Assche]

On 8/28/22 18:18, Ming Lei wrote:
> On Thu, Aug 25, 2022 at 05:26:34PM -0700, Bart Van Assche wrote:
>> There are two .exit_cmd_priv implementations. Both implementations use
>> resources associated with the SCSI host. Make sure that these resources are
>> still available when .exit_cmd_priv is called by waiting inside
>> scsi_remove_host() until the tag set has been freed.
>>
>> This patch fixes the following use-after-free:
>>
>> ==================================================================
>> BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>> Read of size 8 at addr ffff888100337000 by task multipathd/16727
>> Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0x34/0x44
>>   print_report.cold+0x5e/0x5db
>>   kasan_report+0xab/0x120
>>   srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>>   scsi_mq_exit_request+0x4d/0x70
>>   blk_mq_free_rqs+0x143/0x410
>>   __blk_mq_free_map_and_rqs+0x6e/0x100
>>   blk_mq_free_tag_set+0x2b/0x160
>>   scsi_host_dev_release+0xf3/0x1a0
> 
> The trace must be triggered on old kernel, cause this issue is fixed by
> upstream since commit f323896fe6fa ("scsi: core: Call blk_mq_free_tag_set() earlier")
> from you, :-)

Hi Ming,

Did you perhaps overlook the patch series "[PATCH 0/4] Revert "Call 
blk_mq_free_tag_set() earlier"" 
(https://lore.kernel.org/linux-scsi/20220821220502.13685-1-bvanassche@acm.org/)? 
This patch reworks the patch series "Call blk_mq_free_tag_set() earlier" 
but without triggering the deadlock reported by syzbot and also here: 
https://lore.kernel.org/all/Yv%2FMKymRC9O04Nqu@google.com/

Thanks,

Bart.

Re: [PATCH] scsi: core: Fix a use-after-free

[Li Zhijian]

On 26/08/2022 08:26, Bart Van Assche wrote:
> There are two .exit_cmd_priv implementations. Both implementations use
> resources associated with the SCSI host. Make sure that these resources are
> still available when .exit_cmd_priv is called by waiting inside
> scsi_remove_host() until the tag set has been freed.
>
> This patch fixes the following use-after-free:
>
> ==================================================================
> BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
> Read of size 8 at addr ffff888100337000 by task multipathd/16727
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x34/0x44
>   print_report.cold+0x5e/0x5db
>   kasan_report+0xab/0x120
>   srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>   scsi_mq_exit_request+0x4d/0x70
>   blk_mq_free_rqs+0x143/0x410
>   __blk_mq_free_map_and_rqs+0x6e/0x100
>   blk_mq_free_tag_set+0x2b/0x160
>   scsi_host_dev_release+0xf3/0x1a0
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   scsi_device_dev_release_usercontext+0x4c1/0x4e0
>   execute_in_process_context+0x23/0x90
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   scsi_disk_release+0x3f/0x50
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   disk_release+0x17f/0x1b0
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   dm_put_table_device+0xa3/0x160 [dm_mod]
>   dm_put_device+0xd0/0x140 [dm_mod]
>   free_priority_group+0xd8/0x110 [dm_multipath]
>   free_multipath+0x94/0xe0 [dm_multipath]
>   dm_table_destroy+0xa2/0x1e0 [dm_mod]
>   __dm_destroy+0x196/0x350 [dm_mod]
>   dev_remove+0x10c/0x160 [dm_mod]
>   ctl_ioctl+0x2c2/0x590 [dm_mod]
>   dm_ctl_ioctl+0x5/0x10 [dm_mod]
>   __x64_sys_ioctl+0xb4/0xf0
>   dm_ctl_ioctl+0x5/0x10 [dm_mod]
>   __x64_sys_ioctl+0xb4/0xf0
>   do_syscall_64+0x3b/0x90
>   entry_SYSCALL_64_after_hwframe+0x46/0xb0
>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

I tested this after reverted previous 4 patches(https://lore.kernel.org/linux-scsi/20220821220502.13685-1-bvanassche@acm.org/).
Above UFA is gone, feel free to add my tested-by :)

Thanks
Zhijian


> ---
>   drivers/scsi/hosts.c      | 16 +++++++++++++---
>   drivers/scsi/scsi_lib.c   |  6 +++++-
>   drivers/scsi/scsi_priv.h  |  2 +-
>   drivers/scsi/scsi_scan.c  |  1 +
>   drivers/scsi/scsi_sysfs.c |  1 +
>   include/scsi/scsi_host.h  |  2 ++
>   6 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 26bf3b153595..9857dba09c95 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -182,6 +182,15 @@ void scsi_remove_host(struct Scsi_Host *shost)
>   	mutex_unlock(&shost->scan_mutex);
>   	scsi_proc_host_rm(shost);
>   
> +	/*
> +	 * New SCSI devices cannot be attached anymore because of the SCSI host
> +	 * state so drop the tag set refcnt. Wait until the tag set refcnt drops
> +	 * to zero because .exit_cmd_priv implementations may need the host
> +	 * pointer.
> +	 */
> +	kref_put(&shost->tagset_refcnt, scsi_mq_free_tags);
> +	wait_for_completion(&shost->tagset_freed);
> +
>   	spin_lock_irqsave(shost->host_lock, flags);
>   	if (scsi_host_set_state(shost, SHOST_DEL))
>   		BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
> @@ -245,6 +254,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	if (error)
>   		goto fail;
>   
> +	kref_init(&shost->tagset_refcnt);
> +	init_completion(&shost->tagset_freed);
> +
>   	/*
>   	 * Increase usage count temporarily here so that calling
>   	 * scsi_autopm_put_host() will trigger runtime idle if there is
> @@ -317,6 +329,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	pm_runtime_disable(&shost->shost_gendev);
>   	pm_runtime_set_suspended(&shost->shost_gendev);
>   	pm_runtime_put_noidle(&shost->shost_gendev);
> +	kref_put(&shost->tagset_refcnt, scsi_mq_free_tags);
>    fail:
>   	return error;
>   }
> @@ -350,9 +363,6 @@ static void scsi_host_dev_release(struct device *dev)
>   		kfree(dev_name(&shost->shost_dev));
>   	}
>   
> -	if (shost->tag_set.tags)
> -		scsi_mq_destroy_tags(shost);
> -
>   	kfree(shost->shost_data);
>   
>   	ida_free(&host_index_ida, shost->host_no);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4dbd29ab1dcc..1f30e0c54e55 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1976,9 +1976,13 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>   	return blk_mq_alloc_tag_set(tag_set);
>   }
>   
> -void scsi_mq_destroy_tags(struct Scsi_Host *shost)
> +void scsi_mq_free_tags(struct kref *kref)
>   {
> +	struct Scsi_Host *shost = container_of(kref, typeof(*shost),
> +					       tagset_refcnt);
> +
>   	blk_mq_free_tag_set(&shost->tag_set);
> +	complete(&shost->tagset_freed);
>   }
>   
>   /**
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 429663bd78ec..f385b3f04d6e 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -94,7 +94,7 @@ extern void scsi_run_host_queues(struct Scsi_Host *shost);
>   extern void scsi_requeue_run_queue(struct work_struct *work);
>   extern void scsi_start_queue(struct scsi_device *sdev);
>   extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
> -extern void scsi_mq_destroy_tags(struct Scsi_Host *shost);
> +extern void scsi_mq_free_tags(struct kref *kref);
>   extern void scsi_exit_queue(void);
>   extern void scsi_evt_thread(struct work_struct *work);
>   
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 91ac901a6682..5d27f5196de6 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -340,6 +340,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
>   		kfree(sdev);
>   		goto out;
>   	}
> +	kref_get(&sdev->host->tagset_refcnt);
>   	sdev->request_queue = q;
>   	q->queuedata = sdev;
>   	__scsi_init_queue(sdev->host, q);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index aa70d9282161..5d61f58399dc 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1476,6 +1476,7 @@ void __scsi_remove_device(struct scsi_device *sdev)
>   	mutex_unlock(&sdev->state_mutex);
>   
>   	blk_mq_destroy_queue(sdev->request_queue);
> +	kref_put(&sdev->host->tagset_refcnt, scsi_mq_free_tags);
>   	cancel_work_sync(&sdev->requeue_work);
>   
>   	if (sdev->host->hostt->slave_destroy)
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b6e41ee3d566..9b0a028bf053 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -557,6 +557,8 @@ struct Scsi_Host {
>   	struct scsi_host_template *hostt;
>   	struct scsi_transport_template *transportt;
>   
> +	struct kref		tagset_refcnt;
> +	struct completion	tagset_freed;
>   	/* Area to keep a shared tag map */
>   	struct blk_mq_tag_set	tag_set;
>   

Re: [PATCH] scsi: core: Fix a use-after-free

[Martin K. Petersen]

On Thu, 25 Aug 2022 17:26:34 -0700, Bart Van Assche wrote:

> There are two .exit_cmd_priv implementations. Both implementations use
> resources associated with the SCSI host. Make sure that these resources are
> still available when .exit_cmd_priv is called by waiting inside
> scsi_remove_host() until the tag set has been freed.
> 
> This patch fixes the following use-after-free:
> 
> [...]

Applied to 6.0/scsi-fixes, thanks!

[1/1] scsi: core: Fix a use-after-free
      https://git.kernel.org/mkp/scsi/c/8fe4ce5836e9

-- 
Martin K. Petersen	Oracle Linux Engineering