| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <41517c0e-acde-25ce-b4d0-7a32499009f3@acm.org>
Date: Mon, 27 Jun 2022 10:28:54 -0700
From: Bart Van Assche <bvanassche@....org>
To: "lizhijian@...itsu.com" <lizhijian@...itsu.com>,
Jason Gunthorpe <jgg@...dia.com>
Cc: Leon Romanovsky <leon@...nel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH] RDMA/srp: Fix use-after-free in srp_exit_cmd_priv
On 6/26/22 23:47, lizhijian@...itsu.com wrote:
> On 25/06/2022 07:47, Jason Gunthorpe wrote:
>> On Fri, Jun 24, 2022 at 04:26:06PM -0700, Bart Van Assche wrote:
>>> On 6/24/22 15:59, Jason Gunthorpe wrote:
>>>> I don't even understand how get_device() prevents this call chain??
>>>>
>>>> It looks to me like the problem is srp_remove_one() is not waiting for
>>>> or canceling some outstanding work.
>>> Hi Jason,
>>>
>>> My conclusions from the call traces in Li's email are as follows:
>>> * scsi_host_dev_release() can get called after srp_remove_one().
>>> * srp_exit_cmd_priv() uses the ib_device pointer. If srp_remove_one() is
>>> called before srp_exit_cmd_priv() then a use-after-free is triggered.
>> Shouldn't srp_remove_one() wait for the scsi_host_dev to complete
>> destruction? Clearly it cannot continue to exist once the IB device
>> has been removed
> Yes, that match my first thought, but i didn't know the exact way to notify scsi side to destroy
> itself but scsi_host_put() which already called once in below chains:
>
> srp_remove_one()
> -> srp_queue_remove_work()
> -> srp_remove_target()
> -> scsi_remove_host()
> -> scsi_host_put()
>
> that means scsi_host_dev is still referenced by other components that we have to notify.
How about the patch below (should be sent to the SCSI maintainer)?
Subject: [PATCH] scsi: core: Call blk_mq_free_tag_set() earlier
scsi_mq_exit_request() is called by blk_mq_free_tag_set(). Since
scsi_mq_exit_request() implementations may need resources that are freed
after scsi_remove_host() has been called and before the host reference
count drops to zero, call blk_mq_free_tag_set() before the host
reference count drops to zero. blk_mq_free_tag_set() can be called
immediately after scsi_forget_host() has returned since scsi_forget_host()
drains all the request queues that use the host tag set.
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
CPU: 0 PID: 16727 Comm: multipathd Not tainted 5.19.0-rc1-roce-flush+ #78
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-27-g64f37cc530f1-prebuilt.qemu.org 04/01/2014
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
Reported-by: Li Zhijian <lizhijian@...itsu.com>
Cc: Christoph Hellwig <hch@....de>
Cc: Ming Lei <ming.lei@...hat.com>
Cc: John Garry <john.garry@...wei.com>
Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
Signed-off-by: Bart Van Assche <bvanassche@....org>
---
drivers/scsi/hosts.c | 10 +++++-----
drivers/scsi/scsi_lib.c | 3 +++
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index ef6c0e37acce..74bfa187fe19 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -182,6 +182,8 @@ void scsi_remove_host(struct Scsi_Host *shost)
mutex_unlock(&shost->scan_mutex);
scsi_proc_host_rm(shost);
+ scsi_mq_destroy_tags(shost);
+
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));
@@ -295,8 +297,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
return error;
/*
- * Any host allocation in this function will be freed in
- * scsi_host_dev_release().
+ * Any resources associated with the SCSI host in this function except
+ * the tag set will be freed by scsi_host_dev_release().
*/
out_del_dev:
device_del(&shost->shost_dev);
@@ -312,6 +314,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);
+ scsi_mq_destroy_tags(shost);
fail:
return error;
}
@@ -345,9 +348,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 6ffc9e4258a8..1aa1a279f8f3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
void scsi_mq_destroy_tags(struct Scsi_Host *shost)
{
+ if (!shost->tag_set.tags)
+ return;
blk_mq_free_tag_set(&shost->tag_set);
+ WARN_ON_ONCE(shost->tag_set.tags);
}
/**
Powered by blists - more mailing lists