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

[Li Zhijian <lizhijian@xxxxxxxxxxx>]

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@xxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Mike Christie <michael.christie@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Cc: John Garry <john.garry@xxxxxxxxxx>
> Cc: Li Zhijian <lizhijian@xxxxxxxxxxx>
> Reported-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
> Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>

I tested this after reverted previous 4 patches(https://lore.kernel.org/linux-scsi/20220821220502.13685-1-bvanassche@xxxxxxx/).
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;
>