* [PATCH 0/2] Fix use-after-free in iscsi_sw_tcp_host_get_param() @ 2021-04-07 1:24 Wenchao Hao 2021-04-07 1:24 ` [PATCH 1/2] scsi: libiscsi: Split iscsi_session_teardown() to destroy and free Wenchao Hao 2021-04-07 1:24 ` [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao 0 siblings, 2 replies; 4+ messages in thread From: Wenchao Hao @ 2021-04-07 1:24 UTC (permalink / raw) To: Lee Duncan, Chris Leech, James E . J . Bottomley, Martin K . Petersen Cc: open-iscsi, linux-scsi, linux-kernel, Wu Bo, linfeilong, Wenchao Hao Following stack is reported by KASAN: [29844.848044] sd 2:0:0:1: [sdj] Synchronizing SCSI cache [29844.923745] scsi 2:0:0:1: alua: Detached [29844.927840] ================================================================== [29844.927861] BUG: KASAN: use-after-free in iscsi_sw_tcp_host_get_param+0xf4/0x218 [iscsi_tcp] [29844.927864] Read of size 8 at addr ffff80002c0b8f68 by task iscsiadm/523945 [29844.927871] CPU: 1 PID: 523945 Comm: iscsiadm Kdump: loaded Not tainted 4.19.90.kasan.aarch64 [29844.927873] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015 [29844.927875] Call trace: [29844.927884] dump_backtrace+0x0/0x270 [29844.927886] show_stack+0x24/0x30 [29844.927895] dump_stack+0xc4/0x120 [29844.927902] print_address_description+0x68/0x278 [29844.927904] kasan_report+0x20c/0x338 [29844.927906] __asan_load8+0x88/0xb0 [29844.927910] iscsi_sw_tcp_host_get_param+0xf4/0x218 [iscsi_tcp] [29844.927932] show_host_param_ISCSI_HOST_PARAM_IPADDRESS+0x84/0xa0 [scsi_transport_iscsi] [29844.927938] dev_attr_show+0x48/0x90 [29844.927943] sysfs_kf_seq_show+0x100/0x1e0 [29844.927946] kernfs_seq_show+0x88/0xa0 [29844.927949] seq_read+0x164/0x748 [29844.927951] kernfs_fop_read+0x204/0x308 [29844.927956] __vfs_read+0xd4/0x2d8 [29844.927958] vfs_read+0xa8/0x198 [29844.927960] ksys_read+0xd0/0x180 [29844.927962] __arm64_sys_read+0x4c/0x60 [29844.927966] el0_svc_common+0xa8/0x230 [29844.927969] el0_svc_handler+0xdc/0x138 [29844.927971] el0_svc+0x10/0x218 [29844.928063] Freed by task 53358: [29844.928066] __kasan_slab_free+0x120/0x228 [29844.928068] kasan_slab_free+0x10/0x18 [29844.928069] kfree+0x98/0x278 [29844.928083] iscsi_session_release+0x84/0xa0 [scsi_transport_iscsi] [29844.928085] device_release+0x4c/0x100 [29844.928089] kobject_put+0xc4/0x288 [29844.928091] put_device+0x24/0x30 [29844.928105] iscsi_free_session+0x60/0x70 [scsi_transport_iscsi] [29844.928112] iscsi_session_teardown+0x134/0x158 [libiscsi] [29844.928116] iscsi_sw_tcp_session_destroy+0x7c/0xd8 [iscsi_tcp] [29844.928129] iscsi_if_rx+0x1538/0x1f00 [scsi_transport_iscsi] [29844.928131] netlink_unicast+0x338/0x3c8 [29844.928133] netlink_sendmsg+0x51c/0x588 [29844.928135] sock_sendmsg+0x74/0x98 [29844.928137] ___sys_sendmsg+0x434/0x470 [29844.928139] __sys_sendmsg+0xd4/0x148 [29844.928141] __arm64_sys_sendmsg+0x50/0x60 [29844.928143] el0_svc_common+0xa8/0x230 [29844.928146] el0_svc_handler+0xdc/0x138 [29844.928147] el0_svc+0x10/0x218 [29844.928148] [29844.928150] The buggy address belongs to the object at ffff80002c0b8880#012 which belongs to the cache kmalloc-2048 of size 2048 [29844.928153] The buggy address is located 1768 bytes inside of#012 2048-byte region [ffff80002c0b8880, ffff80002c0b9080) [29844.928154] The buggy address belongs to the page: [29844.928158] page:ffff7e0000b02e00 count:1 mapcount:0 mapping:ffff8000d8402600 index:0x0 compound_mapcount: 0 [29844.928902] flags: 0x7fffe0000008100(slab|head) [29844.929215] raw: 07fffe0000008100 ffff7e0003535e08 ffff7e00024a9408 ffff8000d8402600 [29844.929217] raw: 0000000000000000 00000000000f000f 00000001ffffffff 0000000000000000 [29844.929219] page dumped because: kasan: bad access detected [29844.929219] [29844.929221] Memory state around the buggy address: [29844.929223] ffff80002c0b8e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [29844.929225] ffff80002c0b8e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [29844.929227] >ffff80002c0b8f00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [29844.929228] ^ [29844.929230] ffff80002c0b8f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [29844.929232] ffff80002c0b9000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [29844.929232] ================================================================== [29844.929234] Disabling lock debugging due to kernel taint [29844.969534] scsi host2: iSCSI Initiator over TCP/IP Fix this issue by freeing iscsi session after host already removed from sysfs. Wenchao Hao (2): scsi: libiscsi: Split iscsi_session_teardown() to destroy and free scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() drivers/scsi/iscsi_tcp.c | 27 ++++++++++++++++++--------- drivers/scsi/libiscsi.c | 19 ++++++++++++------- include/scsi/libiscsi.h | 1 + 3 files changed, 31 insertions(+), 16 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] scsi: libiscsi: Split iscsi_session_teardown() to destroy and free 2021-04-07 1:24 [PATCH 0/2] Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao @ 2021-04-07 1:24 ` Wenchao Hao 2021-04-07 1:24 ` [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao 1 sibling, 0 replies; 4+ messages in thread From: Wenchao Hao @ 2021-04-07 1:24 UTC (permalink / raw) To: Lee Duncan, Chris Leech, James E . J . Bottomley, Martin K . Petersen Cc: open-iscsi, linux-scsi, linux-kernel, Wu Bo, linfeilong, Wenchao Hao Split iscsi_session_teardown() to two steps: destroy and free, so we can destroy a session without freeing it. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> Signed-off-by: Wu Bo <wubo40@huawei.com> --- drivers/scsi/libiscsi.c | 19 ++++++++++++------- include/scsi/libiscsi.h | 1 + 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 04633e5157e9..9b7eb56e3bd8 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -2929,11 +2929,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost, } EXPORT_SYMBOL_GPL(iscsi_session_setup); -/** - * iscsi_session_teardown - destroy session, host, and cls_session - * @cls_session: iscsi session - */ -void iscsi_session_teardown(struct iscsi_cls_session *cls_session) +void iscsi_session_destroy(struct iscsi_cls_session *cls_session) { struct iscsi_session *session = cls_session->dd_data; struct module *owner = cls_session->transport->owner; @@ -2957,11 +2953,20 @@ void iscsi_session_teardown(struct iscsi_cls_session *cls_session) kfree(session->portal_type); kfree(session->discovery_parent_type); - iscsi_free_session(cls_session); - iscsi_host_dec_session_cnt(shost); module_put(owner); } +EXPORT_SYMBOL_GPL(iscsi_session_destroy); + +/** + * iscsi_session_teardown - destroy session, host, and cls_session + * @cls_session: iscsi session + */ +void iscsi_session_teardown(struct iscsi_cls_session *cls_session) +{ + iscsi_session_destroy(cls_session); + iscsi_free_session(cls_session); +} EXPORT_SYMBOL_GPL(iscsi_session_teardown); /** diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h index 02f966e9358f..d8eef420766d 100644 --- a/include/scsi/libiscsi.h +++ b/include/scsi/libiscsi.h @@ -404,6 +404,7 @@ extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost, extern struct iscsi_cls_session * iscsi_session_setup(struct iscsi_transport *, struct Scsi_Host *shost, uint16_t, int, int, uint32_t, unsigned int); +extern void iscsi_session_destroy(struct iscsi_cls_session *); extern void iscsi_session_teardown(struct iscsi_cls_session *); extern void iscsi_session_recovery_timedout(struct iscsi_cls_session *); extern int iscsi_set_param(struct iscsi_cls_conn *cls_conn, -- 2.27.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() 2021-04-07 1:24 [PATCH 0/2] Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao 2021-04-07 1:24 ` [PATCH 1/2] scsi: libiscsi: Split iscsi_session_teardown() to destroy and free Wenchao Hao @ 2021-04-07 1:24 ` Wenchao Hao 2021-04-12 17:19 ` Mike Christie 1 sibling, 1 reply; 4+ messages in thread From: Wenchao Hao @ 2021-04-07 1:24 UTC (permalink / raw) To: Lee Duncan, Chris Leech, James E . J . Bottomley, Martin K . Petersen Cc: open-iscsi, linux-scsi, linux-kernel, Wu Bo, linfeilong, Wenchao Hao iscsi_sw_tcp_host_get_param() would access struct iscsi_session, while struct iscsi_session might be freed by session destroy flow in iscsi_free_session(). This commit fix this condition by freeing session after host has already been removed. Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/scsi/iscsi_tcp.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index dd33ce0e3737..d559abd3694c 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -839,6 +839,18 @@ iscsi_sw_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn, iscsi_tcp_conn_get_stats(cls_conn, stats); } +static void +iscsi_sw_tcp_session_teardown(struct iscsi_cls_session *cls_session) +{ + struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); + + iscsi_session_destroy(cls_session); + iscsi_host_remove(shost); + + iscsi_free_session(cls_session); + iscsi_host_free(shost); +} + static struct iscsi_cls_session * iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, uint16_t qdepth, uint32_t initial_cmdsn) @@ -884,12 +896,13 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, tcp_sw_host = iscsi_host_priv(shost); tcp_sw_host->session = session; - if (iscsi_tcp_r2tpool_alloc(session)) - goto remove_session; + if (iscsi_tcp_r2tpool_alloc(session)) { + iscsi_sw_tcp_session_teardown(cls_session); + return NULL; + } + return cls_session; -remove_session: - iscsi_session_teardown(cls_session); remove_host: iscsi_host_remove(shost); free_host: @@ -899,17 +912,13 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max, static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session) { - struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); struct iscsi_session *session = cls_session->dd_data; if (WARN_ON_ONCE(session->leadconn)) return; iscsi_tcp_r2tpool_free(cls_session->dd_data); - iscsi_session_teardown(cls_session); - - iscsi_host_remove(shost); - iscsi_host_free(shost); + iscsi_sw_tcp_session_teardown(cls_session); } static umode_t iscsi_sw_tcp_attr_is_visible(int param_type, int param) -- 2.27.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() 2021-04-07 1:24 ` [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao @ 2021-04-12 17:19 ` Mike Christie 0 siblings, 0 replies; 4+ messages in thread From: Mike Christie @ 2021-04-12 17:19 UTC (permalink / raw) To: Wenchao Hao, Lee Duncan, Chris Leech, James E . J . Bottomley, Martin K . Petersen Cc: open-iscsi, linux-scsi, linux-kernel, Wu Bo, linfeilong On 4/6/21 8:24 PM, Wenchao Hao wrote: > iscsi_sw_tcp_host_get_param() would access struct iscsi_session, while > struct iscsi_session might be freed by session destroy flow in > iscsi_free_session(). This commit fix this condition by freeing session > after host has already been removed. > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > --- > drivers/scsi/iscsi_tcp.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index dd33ce0e3737..d559abd3694c 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -839,6 +839,18 @@ iscsi_sw_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn, > iscsi_tcp_conn_get_stats(cls_conn, stats); > } > > +static void > +iscsi_sw_tcp_session_teardown(struct iscsi_cls_session *cls_session) > +{ > + struct Scsi_Host *shost = iscsi_session_to_shost(cls_session); > + > + iscsi_session_destroy(cls_session); > + iscsi_host_remove(shost); > + > + iscsi_free_session(cls_session); > + iscsi_host_free(shost); > +} Can you add a comment about the iscsi_tcp dependency with the host and session or maybe convert ib_iser too? ib_iser does the same session per host scheme and so if you were just scanning the code and going to make a API change, it's not really clear why the drivers do it differently. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-12 17:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-07 1:24 [PATCH 0/2] Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao 2021-04-07 1:24 ` [PATCH 1/2] scsi: libiscsi: Split iscsi_session_teardown() to destroy and free Wenchao Hao 2021-04-07 1:24 ` [PATCH 2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param() Wenchao Hao 2021-04-12 17:19 ` Mike Christie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).