| Message ID | 20220826002635.919423-1-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: core: Fix a use-after-free | expand |
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
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.
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; >
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
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;
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(-)