* [PATCH 0/3] Fix a use-after-free in the SRP target driver
@ 2022-07-26 21:21 Bart Van Assche
2022-07-26 21:21 ` [PATCH 1/3] RDMA/srpt: Duplicate port name members Bart Van Assche
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Bart Van Assche @ 2022-07-26 21:21 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, linux-rdma, Li Zhijian, Hillf Danton,
Mike Christie, Bart Van Assche
Hi Jason,
A known issue in the SRP target driver is that a use-after-free is triggered
if an RDMA port is removed while a LIO target port is still associated with
that RDMA port. This patch series fixes that use-after-free.
Thanks,
Bart.
See also:
* Commit 9b64f7d0bb0a ("RDMA/srpt: Postpone HCA removal until after configfs directory removal").
* https://lore.kernel.org/all/17649b9c-7e42-1625-8bc9-8ad333ab771c@fujitsu.com/
Bart Van Assche (3):
RDMA/srpt: Duplicate port name members
RDMA/srpt: Introduce a reference count in struct srpt_device
RDMA/srpt: Fix a use-after-free
drivers/infiniband/ulp/srpt/ib_srpt.c | 151 ++++++++++++++++++--------
drivers/infiniband/ulp/srpt/ib_srpt.h | 18 ++-
2 files changed, 120 insertions(+), 49 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] RDMA/srpt: Duplicate port name members 2022-07-26 21:21 [PATCH 0/3] Fix a use-after-free in the SRP target driver Bart Van Assche @ 2022-07-26 21:21 ` Bart Van Assche 2022-07-26 21:21 ` [PATCH 2/3] RDMA/srpt: Introduce a reference count in struct srpt_device Bart Van Assche 2022-07-26 21:21 ` [PATCH 3/3] RDMA/srpt: Fix a use-after-free Bart Van Assche 2 siblings, 0 replies; 11+ messages in thread From: Bart Van Assche @ 2022-07-26 21:21 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie, Bart Van Assche Prepare for decoupling the lifetimes of struct srpt_port and struct srpt_port_id by duplicating the port name into struct srpt_port. Cc: Li Zhijian <lizhijian@fujitsu.com> Cc: Hillf Danton <hdanton@sina.com> Cc: Mike Christie <michael.christie@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/ulp/srpt/ib_srpt.c | 9 ++++++--- drivers/infiniband/ulp/srpt/ib_srpt.h | 10 +++++++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index f86ee1c4b970..8253d55b9c26 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -566,14 +566,17 @@ static int srpt_refresh_port(struct srpt_port *sport) return ret; sport->port_guid_id.wwn.priv = sport; - srpt_format_guid(sport->port_guid_id.name, - sizeof(sport->port_guid_id.name), + srpt_format_guid(sport->guid_name, ARRAY_SIZE(sport->guid_name), &sport->gid.global.interface_id); + memcpy(sport->port_guid_id.name, sport->guid_name, + ARRAY_SIZE(sport->guid_name)); sport->port_gid_id.wwn.priv = sport; - snprintf(sport->port_gid_id.name, sizeof(sport->port_gid_id.name), + snprintf(sport->gid_name, ARRAY_SIZE(sport->gid_name), "0x%016llx%016llx", be64_to_cpu(sport->gid.global.subnet_prefix), be64_to_cpu(sport->gid.global.interface_id)); + memcpy(sport->port_gid_id.name, sport->gid_name, + ARRAY_SIZE(sport->gid_name)); if (rdma_protocol_iwarp(sport->sdev->device, sport->port)) return 0; diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 76e66f630c17..3844a7058559 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -376,7 +376,7 @@ struct srpt_tpg { }; /** - * struct srpt_port_id - information about an RDMA port name + * struct srpt_port_id - LIO RDMA port information * @mutex: Protects @tpg_list changes. * @tpg_list: TPGs associated with the RDMA port name. * @wwn: WWN associated with the RDMA port name. @@ -402,8 +402,10 @@ struct srpt_port_id { * @lid: cached value of the port's lid. * @gid: cached value of the port's gid. * @work: work structure for refreshing the aforementioned cached values. - * @port_guid_id: target port GUID - * @port_gid_id: target port GID + * @guid_name: port name in GUID format. + * @port_guid_id: LIO target port information for the port name in GUID format. + * @gid_name: port name in GID format. + * @port_gid_id: LIO target port information for the port name in GID format. * @port_attrib: Port attributes that can be accessed through configfs. * @refcount: Number of objects associated with this port. * @freed_channels: Completion that will be signaled once @refcount becomes 0. @@ -419,7 +421,9 @@ struct srpt_port { u32 lid; union ib_gid gid; struct work_struct work; + char guid_name[64]; struct srpt_port_id port_guid_id; + char gid_name[64]; struct srpt_port_id port_gid_id; struct srpt_port_attrib port_attrib; atomic_t refcount; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] RDMA/srpt: Introduce a reference count in struct srpt_device 2022-07-26 21:21 [PATCH 0/3] Fix a use-after-free in the SRP target driver Bart Van Assche 2022-07-26 21:21 ` [PATCH 1/3] RDMA/srpt: Duplicate port name members Bart Van Assche @ 2022-07-26 21:21 ` Bart Van Assche 2022-07-26 21:21 ` [PATCH 3/3] RDMA/srpt: Fix a use-after-free Bart Van Assche 2 siblings, 0 replies; 11+ messages in thread From: Bart Van Assche @ 2022-07-26 21:21 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie, Bart Van Assche This will be used to keep struct srpt_device around as long as either the RDMA port exists or a LIO target port is associated with the struct srpt_device. Cc: Li Zhijian <lizhijian@fujitsu.com> Cc: Hillf Danton <hdanton@sina.com> Cc: Mike Christie <michael.christie@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/ulp/srpt/ib_srpt.c | 17 +++++++++++++++-- drivers/infiniband/ulp/srpt/ib_srpt.h | 2 ++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index 8253d55b9c26..1fbce9225424 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -3104,6 +3104,18 @@ static int srpt_use_srq(struct srpt_device *sdev, bool use_srq) return ret; } +static void srpt_free_sdev(struct kref *refcnt) +{ + struct srpt_device *sdev = container_of(refcnt, typeof(*sdev), refcnt); + + kfree(sdev); +} + +static void srpt_sdev_put(struct srpt_device *sdev) +{ + kref_put(&sdev->refcnt, srpt_free_sdev); +} + /** * srpt_add_one - InfiniBand device addition callback function * @device: Describes a HCA. @@ -3122,6 +3134,7 @@ static int srpt_add_one(struct ib_device *device) if (!sdev) return -ENOMEM; + kref_init(&sdev->refcnt); sdev->device = device; mutex_init(&sdev->sdev_mutex); @@ -3217,7 +3230,7 @@ static int srpt_add_one(struct ib_device *device) srpt_free_srq(sdev); ib_dealloc_pd(sdev->pd); free_dev: - kfree(sdev); + srpt_sdev_put(sdev); pr_info("%s(%s) failed.\n", __func__, dev_name(&device->dev)); return ret; } @@ -3261,7 +3274,7 @@ static void srpt_remove_one(struct ib_device *device, void *client_data) ib_dealloc_pd(sdev->pd); - kfree(sdev); + srpt_sdev_put(sdev); } static struct ib_client srpt_client = { diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 3844a7058559..0cb867d580f1 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -434,6 +434,7 @@ struct srpt_port { /** * struct srpt_device - information associated by SRPT with a single HCA + * @refcnt: Reference count for this device. * @device: Backpointer to the struct ib_device managed by the IB core. * @pd: IB protection domain. * @lkey: L_Key (local key) with write access to all local memory. @@ -449,6 +450,7 @@ struct srpt_port { * @port: Information about the ports owned by this HCA. */ struct srpt_device { + struct kref refcnt; struct ib_device *device; struct ib_pd *pd; u32 lkey; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-26 21:21 [PATCH 0/3] Fix a use-after-free in the SRP target driver Bart Van Assche 2022-07-26 21:21 ` [PATCH 1/3] RDMA/srpt: Duplicate port name members Bart Van Assche 2022-07-26 21:21 ` [PATCH 2/3] RDMA/srpt: Introduce a reference count in struct srpt_device Bart Van Assche @ 2022-07-26 21:21 ` Bart Van Assche 2022-07-27 9:36 ` Leon Romanovsky 2022-07-27 10:03 ` lizhijian 2 siblings, 2 replies; 11+ messages in thread From: Bart Van Assche @ 2022-07-26 21:21 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie, Bart Van Assche Change the LIO port members inside struct srpt_port from regular members into pointers. Allocate the LIO port data structures from inside srpt_make_tport() and free these from inside srpt_make_tport(). Keep struct srpt_device as long as either an RDMA port or a LIO target port is associated with it. This patch decouples the lifetime of struct srpt_port (controlled by the RDMA core) and struct srpt_port_id (controlled by LIO). This patch fixes the following KASAN complaint: BUG: KASAN: use-after-free in srpt_enable_tpg+0x31/0x70 [ib_srpt] Read of size 8 at addr ffff888141cc34b8 by task check/5093 Call Trace: <TASK> show_stack+0x4e/0x53 dump_stack_lvl+0x51/0x66 print_address_description.constprop.0.cold+0xea/0x41e print_report.cold+0x90/0x205 kasan_report+0xb9/0xf0 __asan_load8+0x69/0x90 srpt_enable_tpg+0x31/0x70 [ib_srpt] target_fabric_tpg_base_enable_store+0xe2/0x140 [target_core_mod] configfs_write_iter+0x18b/0x210 new_sync_write+0x1f2/0x2f0 vfs_write+0x3e3/0x540 ksys_write+0xbb/0x140 __x64_sys_write+0x42/0x50 do_syscall_64+0x34/0x80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 </TASK> Reported-by: Li Zhijian <lizhijian@fujitsu.com> Cc: Li Zhijian <lizhijian@fujitsu.com> Cc: Hillf Danton <hdanton@sina.com> Cc: Mike Christie <michael.christie@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/infiniband/ulp/srpt/ib_srpt.c | 133 ++++++++++++++++++-------- drivers/infiniband/ulp/srpt/ib_srpt.h | 10 +- 2 files changed, 96 insertions(+), 47 deletions(-) diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index 1fbce9225424..8d9eb1210d3b 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -565,18 +565,12 @@ static int srpt_refresh_port(struct srpt_port *sport) if (ret) return ret; - sport->port_guid_id.wwn.priv = sport; srpt_format_guid(sport->guid_name, ARRAY_SIZE(sport->guid_name), &sport->gid.global.interface_id); - memcpy(sport->port_guid_id.name, sport->guid_name, - ARRAY_SIZE(sport->guid_name)); - sport->port_gid_id.wwn.priv = sport; snprintf(sport->gid_name, ARRAY_SIZE(sport->gid_name), "0x%016llx%016llx", be64_to_cpu(sport->gid.global.subnet_prefix), be64_to_cpu(sport->gid.global.interface_id)); - memcpy(sport->port_gid_id.name, sport->gid_name, - ARRAY_SIZE(sport->gid_name)); if (rdma_protocol_iwarp(sport->sdev->device, sport->port)) return 0; @@ -2317,31 +2311,35 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, tag_num = ch->rq_size; tag_size = 1; /* ib_srpt does not use se_sess->sess_cmd_map */ - mutex_lock(&sport->port_guid_id.mutex); - list_for_each_entry(stpg, &sport->port_guid_id.tpg_list, entry) { - if (!IS_ERR_OR_NULL(ch->sess)) - break; - ch->sess = target_setup_session(&stpg->tpg, tag_num, + if (sport->guid_id) { + mutex_lock(&sport->guid_id->mutex); + list_for_each_entry(stpg, &sport->guid_id->tpg_list, entry) { + if (!IS_ERR_OR_NULL(ch->sess)) + break; + ch->sess = target_setup_session(&stpg->tpg, tag_num, tag_size, TARGET_PROT_NORMAL, ch->sess_name, ch, NULL); + } + mutex_unlock(&sport->guid_id->mutex); } - mutex_unlock(&sport->port_guid_id.mutex); - mutex_lock(&sport->port_gid_id.mutex); - list_for_each_entry(stpg, &sport->port_gid_id.tpg_list, entry) { - if (!IS_ERR_OR_NULL(ch->sess)) - break; - ch->sess = target_setup_session(&stpg->tpg, tag_num, + if (sport->gid_id) { + mutex_lock(&sport->gid_id->mutex); + list_for_each_entry(stpg, &sport->gid_id->tpg_list, entry) { + if (!IS_ERR_OR_NULL(ch->sess)) + break; + ch->sess = target_setup_session(&stpg->tpg, tag_num, tag_size, TARGET_PROT_NORMAL, i_port_id, ch, NULL); - if (!IS_ERR_OR_NULL(ch->sess)) - break; - /* Retry without leading "0x" */ - ch->sess = target_setup_session(&stpg->tpg, tag_num, + if (!IS_ERR_OR_NULL(ch->sess)) + break; + /* Retry without leading "0x" */ + ch->sess = target_setup_session(&stpg->tpg, tag_num, tag_size, TARGET_PROT_NORMAL, i_port_id + 2, ch, NULL); + } + mutex_unlock(&sport->gid_id->mutex); } - mutex_unlock(&sport->port_gid_id.mutex); if (IS_ERR_OR_NULL(ch->sess)) { WARN_ON_ONCE(ch->sess == NULL); @@ -2986,7 +2984,12 @@ static int srpt_release_sport(struct srpt_port *sport) return 0; } -static struct se_wwn *__srpt_lookup_wwn(const char *name) +struct port_and_port_id { + struct srpt_port *sport; + struct srpt_port_id **port_id; +}; + +static struct port_and_port_id __srpt_lookup_port(const char *name) { struct ib_device *dev; struct srpt_device *sdev; @@ -3001,25 +3004,38 @@ static struct se_wwn *__srpt_lookup_wwn(const char *name) for (i = 0; i < dev->phys_port_cnt; i++) { sport = &sdev->port[i]; - if (strcmp(sport->port_guid_id.name, name) == 0) - return &sport->port_guid_id.wwn; - if (strcmp(sport->port_gid_id.name, name) == 0) - return &sport->port_gid_id.wwn; + if (strcmp(sport->guid_name, name) == 0) { + kref_get(&sdev->refcnt); + return (struct port_and_port_id){ + sport, &sport->guid_id}; + } + if (strcmp(sport->gid_name, name) == 0) { + kref_get(&sdev->refcnt); + return (struct port_and_port_id){ + sport, &sport->gid_id}; + } } } - return NULL; + return (struct port_and_port_id){}; } -static struct se_wwn *srpt_lookup_wwn(const char *name) +/** + * srpt_lookup_port() - Look up an RDMA port by name + * @name: ASCII port name + * + * Increments the RDMA port reference count if an RDMA port pointer is returned. + * The caller must drop that reference count by calling srpt_port_put_ref(). + */ +static struct port_and_port_id srpt_lookup_port(const char *name) { - struct se_wwn *wwn; + struct port_and_port_id papi; spin_lock(&srpt_dev_lock); - wwn = __srpt_lookup_wwn(name); + papi = __srpt_lookup_port(name); spin_unlock(&srpt_dev_lock); - return wwn; + return papi; } static void srpt_free_srq(struct srpt_device *sdev) @@ -3198,10 +3214,6 @@ static int srpt_add_one(struct ib_device *device) sport->port_attrib.srp_sq_size = DEF_SRPT_SQ_SIZE; sport->port_attrib.use_srq = false; INIT_WORK(&sport->work, srpt_refresh_port_work); - mutex_init(&sport->port_guid_id.mutex); - INIT_LIST_HEAD(&sport->port_guid_id.tpg_list); - mutex_init(&sport->port_gid_id.mutex); - INIT_LIST_HEAD(&sport->port_gid_id.tpg_list); ret = srpt_refresh_port(sport); if (ret) { @@ -3295,6 +3307,7 @@ static int srpt_check_false(struct se_portal_group *se_tpg) static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg) { + BUG_ON(!tpg->se_tpg_wwn->priv); return tpg->se_tpg_wwn->priv; } @@ -3302,11 +3315,11 @@ static struct srpt_port_id *srpt_wwn_to_sport_id(struct se_wwn *wwn) { struct srpt_port *sport = wwn->priv; - if (wwn == &sport->port_guid_id.wwn) - return &sport->port_guid_id; - if (wwn == &sport->port_gid_id.wwn) - return &sport->port_gid_id; - WARN_ON_ONCE(true); + BUG_ON(!sport); + if (sport->guid_id && &sport->guid_id->wwn == wwn) + return sport->guid_id; + if (sport->gid_id && &sport->gid_id->wwn == wwn) + return sport->gid_id; return NULL; } @@ -3744,6 +3757,10 @@ static struct se_portal_group *srpt_make_tpg(struct se_wwn *wwn, struct srpt_tpg *stpg; int res = -ENOMEM; + if (!sport_id) { + pr_err("RDMA port does not exist\n"); + return ERR_PTR(-EINVAL); + } stpg = kzalloc(sizeof(*stpg), GFP_KERNEL); if (!stpg) return ERR_PTR(res); @@ -3790,7 +3807,28 @@ static struct se_wwn *srpt_make_tport(struct target_fabric_configfs *tf, struct config_group *group, const char *name) { - return srpt_lookup_wwn(name) ? : ERR_PTR(-EINVAL); + struct port_and_port_id papi = srpt_lookup_port(name); + struct srpt_port *sport = papi.sport; + struct srpt_port_id *port_id; + + if (!papi.port_id) + return ERR_PTR(-EINVAL); + if (*papi.port_id) + return &(*papi.port_id)->wwn; + port_id = kzalloc(sizeof(*port_id), GFP_KERNEL); + if (!port_id) { + srpt_sdev_put(sport->sdev); + return ERR_PTR(-ENOMEM); + } + mutex_init(&port_id->mutex); + INIT_LIST_HEAD(&port_id->tpg_list); + port_id->wwn.priv = sport; + memcpy(port_id->name, port_id == sport->guid_id ? sport->guid_name : + sport->gid_name, ARRAY_SIZE(port_id->name)); + + *papi.port_id = port_id; + + return &port_id->wwn; } /** @@ -3799,6 +3837,17 @@ static struct se_wwn *srpt_make_tport(struct target_fabric_configfs *tf, */ static void srpt_drop_tport(struct se_wwn *wwn) { + struct srpt_port_id *port_id = container_of(wwn, typeof(*port_id), wwn); + struct srpt_port *sport = wwn->priv; + + BUG_ON(!sport); + if (sport->guid_id == port_id) + sport->guid_id = NULL; + else if (sport->gid_id == port_id) + sport->gid_id = NULL; + + srpt_sdev_put(sport->sdev); + kfree(port_id); } static ssize_t srpt_wwn_version_show(struct config_item *item, char *buf) diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h index 0cb867d580f1..4c46b301eea1 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.h +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h @@ -393,7 +393,7 @@ struct srpt_port_id { }; /** - * struct srpt_port - information associated by SRPT with a single IB port + * struct srpt_port - SRPT RDMA port information * @sdev: backpointer to the HCA information. * @mad_agent: per-port management datagram processing information. * @enabled: Whether or not this target port is enabled. @@ -403,9 +403,9 @@ struct srpt_port_id { * @gid: cached value of the port's gid. * @work: work structure for refreshing the aforementioned cached values. * @guid_name: port name in GUID format. - * @port_guid_id: LIO target port information for the port name in GUID format. + * @guid_id: LIO target port information for the port name in GUID format. * @gid_name: port name in GID format. - * @port_gid_id: LIO target port information for the port name in GID format. + * @gid_id: LIO target port information for the port name in GID format. * @port_attrib: Port attributes that can be accessed through configfs. * @refcount: Number of objects associated with this port. * @freed_channels: Completion that will be signaled once @refcount becomes 0. @@ -422,9 +422,9 @@ struct srpt_port { union ib_gid gid; struct work_struct work; char guid_name[64]; - struct srpt_port_id port_guid_id; + struct srpt_port_id *guid_id; char gid_name[64]; - struct srpt_port_id port_gid_id; + struct srpt_port_id *gid_id; struct srpt_port_attrib port_attrib; atomic_t refcount; struct completion *freed_channels; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-26 21:21 ` [PATCH 3/3] RDMA/srpt: Fix a use-after-free Bart Van Assche @ 2022-07-27 9:36 ` Leon Romanovsky 2022-07-27 12:26 ` Christoph Lameter 2022-07-27 10:03 ` lizhijian 1 sibling, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2022-07-27 9:36 UTC (permalink / raw) To: Bart Van Assche Cc: Jason Gunthorpe, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie On Tue, Jul 26, 2022 at 02:21:56PM -0700, Bart Van Assche wrote: > Change the LIO port members inside struct srpt_port from regular members > into pointers. Allocate the LIO port data structures from inside > srpt_make_tport() and free these from inside srpt_make_tport(). Keep struct > srpt_device as long as either an RDMA port or a LIO target port is > associated with it. This patch decouples the lifetime of struct srpt_port > (controlled by the RDMA core) and struct srpt_port_id (controlled by LIO). > This patch fixes the following KASAN complaint: > > BUG: KASAN: use-after-free in srpt_enable_tpg+0x31/0x70 [ib_srpt] > Read of size 8 at addr ffff888141cc34b8 by task check/5093 > > Call Trace: > <TASK> > show_stack+0x4e/0x53 > dump_stack_lvl+0x51/0x66 > print_address_description.constprop.0.cold+0xea/0x41e > print_report.cold+0x90/0x205 > kasan_report+0xb9/0xf0 > __asan_load8+0x69/0x90 > srpt_enable_tpg+0x31/0x70 [ib_srpt] > target_fabric_tpg_base_enable_store+0xe2/0x140 [target_core_mod] > configfs_write_iter+0x18b/0x210 > new_sync_write+0x1f2/0x2f0 > vfs_write+0x3e3/0x540 > ksys_write+0xbb/0x140 > __x64_sys_write+0x42/0x50 > do_syscall_64+0x34/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > </TASK> > > Reported-by: Li Zhijian <lizhijian@fujitsu.com> > Cc: Li Zhijian <lizhijian@fujitsu.com> > Cc: Hillf Danton <hdanton@sina.com> > Cc: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/srpt/ib_srpt.c | 133 ++++++++++++++++++-------- > drivers/infiniband/ulp/srpt/ib_srpt.h | 10 +- > 2 files changed, 96 insertions(+), 47 deletions(-) Bart, Please no BUG_ON() in new code. Thanks ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-27 9:36 ` Leon Romanovsky @ 2022-07-27 12:26 ` Christoph Lameter 2022-07-27 12:31 ` Leon Romanovsky 0 siblings, 1 reply; 11+ messages in thread From: Christoph Lameter @ 2022-07-27 12:26 UTC (permalink / raw) To: Leon Romanovsky Cc: Bart Van Assche, Jason Gunthorpe, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie On Wed, 27 Jul 2022, Leon Romanovsky wrote: > Please no BUG_ON() in new code. Do we now prefer NULL pointer dereferences causing bugs? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-27 12:26 ` Christoph Lameter @ 2022-07-27 12:31 ` Leon Romanovsky 2022-07-27 17:47 ` Bart Van Assche 0 siblings, 1 reply; 11+ messages in thread From: Leon Romanovsky @ 2022-07-27 12:31 UTC (permalink / raw) To: Christoph Lameter Cc: Bart Van Assche, Jason Gunthorpe, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie On Wed, Jul 27, 2022 at 02:26:43PM +0200, Christoph Lameter wrote: > On Wed, 27 Jul 2022, Leon Romanovsky wrote: > > > Please no BUG_ON() in new code. > > Do we now prefer NULL pointer dereferences causing bugs? There are two possible scenarios. 1. "NULL can be in this flow." - In such case, the code should deal with such flow and do not crash whole machine. 2. "NULL can't be in this flow." - Put WARN_ON() which serves as a documentation/debug help to catch situations that are not possible. Thanks > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-27 12:31 ` Leon Romanovsky @ 2022-07-27 17:47 ` Bart Van Assche 0 siblings, 0 replies; 11+ messages in thread From: Bart Van Assche @ 2022-07-27 17:47 UTC (permalink / raw) To: Leon Romanovsky, Christoph Lameter Cc: Jason Gunthorpe, linux-rdma, Li Zhijian, Hillf Danton, Mike Christie On 7/27/22 05:31, Leon Romanovsky wrote: > On Wed, Jul 27, 2022 at 02:26:43PM +0200, Christoph Lameter wrote: >> On Wed, 27 Jul 2022, Leon Romanovsky wrote: >> >>> Please no BUG_ON() in new code. >> >> Do we now prefer NULL pointer dereferences causing bugs? > > There are two possible scenarios. > 1. "NULL can be in this flow." - In such case, the code should deal with > such flow and do not crash whole machine. > 2. "NULL can't be in this flow." - Put WARN_ON() which serves as a > documentation/debug help to catch situations that are not possible. Hi Leon and Christoph, Thanks for having taken a look. Since the wwn.priv pointers cannot be NULL, I will remove the BUG_ON() statements that check these pointers. Bart. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-26 21:21 ` [PATCH 3/3] RDMA/srpt: Fix a use-after-free Bart Van Assche 2022-07-27 9:36 ` Leon Romanovsky @ 2022-07-27 10:03 ` lizhijian 2022-07-27 17:48 ` Bart Van Assche 1 sibling, 1 reply; 11+ messages in thread From: lizhijian @ 2022-07-27 10:03 UTC (permalink / raw) To: Bart Van Assche, Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Hillf Danton, Mike Christie On 27/07/2022 05:21, Bart Van Assche wrote: > Change the LIO port members inside struct srpt_port from regular members > into pointers. Allocate the LIO port data structures from inside > srpt_make_tport() and free these from inside srpt_make_tport(). Keep struct > srpt_device as long as either an RDMA port or a LIO target port is > associated with it. This patch decouples the lifetime of struct srpt_port > (controlled by the RDMA core) and struct srpt_port_id (controlled by LIO). > This patch fixes the following KASAN complaint: > > BUG: KASAN: use-after-free in srpt_enable_tpg+0x31/0x70 [ib_srpt] > Read of size 8 at addr ffff888141cc34b8 by task check/5093 > > Call Trace: > <TASK> > show_stack+0x4e/0x53 > dump_stack_lvl+0x51/0x66 > print_address_description.constprop.0.cold+0xea/0x41e > print_report.cold+0x90/0x205 > kasan_report+0xb9/0xf0 > __asan_load8+0x69/0x90 > srpt_enable_tpg+0x31/0x70 [ib_srpt] > target_fabric_tpg_base_enable_store+0xe2/0x140 [target_core_mod] > configfs_write_iter+0x18b/0x210 > new_sync_write+0x1f2/0x2f0 > vfs_write+0x3e3/0x540 > ksys_write+0xbb/0x140 > __x64_sys_write+0x42/0x50 > do_syscall_64+0x34/0x80 > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > </TASK> I tested patches with blktests/srp, it works well and the UAF is gone. > Reported-by: Li Zhijian <lizhijian@fujitsu.com> > Cc: Li Zhijian <lizhijian@fujitsu.com> > Cc: Hillf Danton <hdanton@sina.com> > Cc: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/infiniband/ulp/srpt/ib_srpt.c | 133 ++++++++++++++++++-------- > drivers/infiniband/ulp/srpt/ib_srpt.h | 10 +- > 2 files changed, 96 insertions(+), 47 deletions(-) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 1fbce9225424..8d9eb1210d3b 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -565,18 +565,12 @@ static int srpt_refresh_port(struct srpt_port *sport) > if (ret) > return ret; > > - sport->port_guid_id.wwn.priv = sport; > srpt_format_guid(sport->guid_name, ARRAY_SIZE(sport->guid_name), > &sport->gid.global.interface_id); > - memcpy(sport->port_guid_id.name, sport->guid_name, > - ARRAY_SIZE(sport->guid_name)); > - sport->port_gid_id.wwn.priv = sport; > snprintf(sport->gid_name, ARRAY_SIZE(sport->gid_name), > "0x%016llx%016llx", > be64_to_cpu(sport->gid.global.subnet_prefix), > be64_to_cpu(sport->gid.global.interface_id)); > - memcpy(sport->port_gid_id.name, sport->gid_name, > - ARRAY_SIZE(sport->gid_name)); > > if (rdma_protocol_iwarp(sport->sdev->device, sport->port)) > return 0; > @@ -2317,31 +2311,35 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > tag_num = ch->rq_size; > tag_size = 1; /* ib_srpt does not use se_sess->sess_cmd_map */ > > - mutex_lock(&sport->port_guid_id.mutex); > - list_for_each_entry(stpg, &sport->port_guid_id.tpg_list, entry) { > - if (!IS_ERR_OR_NULL(ch->sess)) > - break; > - ch->sess = target_setup_session(&stpg->tpg, tag_num, > + if (sport->guid_id) { > + mutex_lock(&sport->guid_id->mutex); > + list_for_each_entry(stpg, &sport->guid_id->tpg_list, entry) { > + if (!IS_ERR_OR_NULL(ch->sess)) > + break; > + ch->sess = target_setup_session(&stpg->tpg, tag_num, > tag_size, TARGET_PROT_NORMAL, > ch->sess_name, ch, NULL); > + } > + mutex_unlock(&sport->guid_id->mutex); > } > - mutex_unlock(&sport->port_guid_id.mutex); > > - mutex_lock(&sport->port_gid_id.mutex); > - list_for_each_entry(stpg, &sport->port_gid_id.tpg_list, entry) { > - if (!IS_ERR_OR_NULL(ch->sess)) > - break; > - ch->sess = target_setup_session(&stpg->tpg, tag_num, > + if (sport->gid_id) { > + mutex_lock(&sport->gid_id->mutex); > + list_for_each_entry(stpg, &sport->gid_id->tpg_list, entry) { > + if (!IS_ERR_OR_NULL(ch->sess)) > + break; > + ch->sess = target_setup_session(&stpg->tpg, tag_num, > tag_size, TARGET_PROT_NORMAL, i_port_id, > ch, NULL); > - if (!IS_ERR_OR_NULL(ch->sess)) > - break; > - /* Retry without leading "0x" */ > - ch->sess = target_setup_session(&stpg->tpg, tag_num, > + if (!IS_ERR_OR_NULL(ch->sess)) > + break; > + /* Retry without leading "0x" */ > + ch->sess = target_setup_session(&stpg->tpg, tag_num, > tag_size, TARGET_PROT_NORMAL, > i_port_id + 2, ch, NULL); > + } > + mutex_unlock(&sport->gid_id->mutex); > } > - mutex_unlock(&sport->port_gid_id.mutex); > > if (IS_ERR_OR_NULL(ch->sess)) { > WARN_ON_ONCE(ch->sess == NULL); > @@ -2986,7 +2984,12 @@ static int srpt_release_sport(struct srpt_port *sport) > return 0; > } > > -static struct se_wwn *__srpt_lookup_wwn(const char *name) > +struct port_and_port_id { > + struct srpt_port *sport; > + struct srpt_port_id **port_id; > +}; > + > +static struct port_and_port_id __srpt_lookup_port(const char *name) > { > struct ib_device *dev; > struct srpt_device *sdev; > @@ -3001,25 +3004,38 @@ static struct se_wwn *__srpt_lookup_wwn(const char *name) > for (i = 0; i < dev->phys_port_cnt; i++) { > sport = &sdev->port[i]; > > - if (strcmp(sport->port_guid_id.name, name) == 0) > - return &sport->port_guid_id.wwn; > - if (strcmp(sport->port_gid_id.name, name) == 0) > - return &sport->port_gid_id.wwn; > + if (strcmp(sport->guid_name, name) == 0) { > + kref_get(&sdev->refcnt); > + return (struct port_and_port_id){ > + sport, &sport->guid_id}; > + } > + if (strcmp(sport->gid_name, name) == 0) { > + kref_get(&sdev->refcnt); > + return (struct port_and_port_id){ > + sport, &sport->gid_id}; > + } > } > } > > - return NULL; > + return (struct port_and_port_id){}; > } > > -static struct se_wwn *srpt_lookup_wwn(const char *name) > +/** > + * srpt_lookup_port() - Look up an RDMA port by name > + * @name: ASCII port name > + * > + * Increments the RDMA port reference count if an RDMA port pointer is returned. > + * The caller must drop that reference count by calling srpt_port_put_ref(). > + */ > +static struct port_and_port_id srpt_lookup_port(const char *name) > { > - struct se_wwn *wwn; > + struct port_and_port_id papi; > > spin_lock(&srpt_dev_lock); > - wwn = __srpt_lookup_wwn(name); > + papi = __srpt_lookup_port(name); > spin_unlock(&srpt_dev_lock); > > - return wwn; > + return papi; > } > > static void srpt_free_srq(struct srpt_device *sdev) > @@ -3198,10 +3214,6 @@ static int srpt_add_one(struct ib_device *device) > sport->port_attrib.srp_sq_size = DEF_SRPT_SQ_SIZE; > sport->port_attrib.use_srq = false; > INIT_WORK(&sport->work, srpt_refresh_port_work); > - mutex_init(&sport->port_guid_id.mutex); > - INIT_LIST_HEAD(&sport->port_guid_id.tpg_list); > - mutex_init(&sport->port_gid_id.mutex); > - INIT_LIST_HEAD(&sport->port_gid_id.tpg_list); > > ret = srpt_refresh_port(sport); > if (ret) { > @@ -3295,6 +3307,7 @@ static int srpt_check_false(struct se_portal_group *se_tpg) > > static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg) > { > + BUG_ON(!tpg->se_tpg_wwn->priv); > return tpg->se_tpg_wwn->priv; > } > > @@ -3302,11 +3315,11 @@ static struct srpt_port_id *srpt_wwn_to_sport_id(struct se_wwn *wwn) > { > struct srpt_port *sport = wwn->priv; > > - if (wwn == &sport->port_guid_id.wwn) > - return &sport->port_guid_id; > - if (wwn == &sport->port_gid_id.wwn) > - return &sport->port_gid_id; > - WARN_ON_ONCE(true); > + BUG_ON(!sport); > + if (sport->guid_id && &sport->guid_id->wwn == wwn) > + return sport->guid_id; > + if (sport->gid_id && &sport->gid_id->wwn == wwn) > + return sport->gid_id; > return NULL; > } > > @@ -3744,6 +3757,10 @@ static struct se_portal_group *srpt_make_tpg(struct se_wwn *wwn, > struct srpt_tpg *stpg; > int res = -ENOMEM; > > + if (!sport_id) { > + pr_err("RDMA port does not exist\n"); > + return ERR_PTR(-EINVAL); > + } > stpg = kzalloc(sizeof(*stpg), GFP_KERNEL); > if (!stpg) > return ERR_PTR(res); > @@ -3790,7 +3807,28 @@ static struct se_wwn *srpt_make_tport(struct target_fabric_configfs *tf, > struct config_group *group, > const char *name) > { > - return srpt_lookup_wwn(name) ? : ERR_PTR(-EINVAL); > + struct port_and_port_id papi = srpt_lookup_port(name); > + struct srpt_port *sport = papi.sport; > + struct srpt_port_id *port_id; > + > + if (!papi.port_id) > + return ERR_PTR(-EINVAL); > + if (*papi.port_id) > + return &(*papi.port_id)->wwn; > + port_id = kzalloc(sizeof(*port_id), GFP_KERNEL); > + if (!port_id) { > + srpt_sdev_put(sport->sdev); > + return ERR_PTR(-ENOMEM); > + } > + mutex_init(&port_id->mutex); > + INIT_LIST_HEAD(&port_id->tpg_list); > + port_id->wwn.priv = sport; > + memcpy(port_id->name, port_id == sport->guid_id ? sport->guid_name : > + sport->gid_name, ARRAY_SIZE(port_id->name)); > + > + *papi.port_id = port_id; > + > + return &port_id->wwn; > } > > /** > @@ -3799,6 +3837,17 @@ static struct se_wwn *srpt_make_tport(struct target_fabric_configfs *tf, > */ > static void srpt_drop_tport(struct se_wwn *wwn) > { > + struct srpt_port_id *port_id = container_of(wwn, typeof(*port_id), wwn); > + struct srpt_port *sport = wwn->priv; > + > + BUG_ON(!sport); > + if (sport->guid_id == port_id) > + sport->guid_id = NULL; > + else if (sport->gid_id == port_id) > + sport->gid_id = NULL; > + > + srpt_sdev_put(sport->sdev); > + kfree(port_id); > } > > static ssize_t srpt_wwn_version_show(struct config_item *item, char *buf) > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h > index 0cb867d580f1..4c46b301eea1 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.h > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.h > @@ -393,7 +393,7 @@ struct srpt_port_id { > }; > > /** > - * struct srpt_port - information associated by SRPT with a single IB port > + * struct srpt_port - SRPT RDMA port information > * @sdev: backpointer to the HCA information. > * @mad_agent: per-port management datagram processing information. > * @enabled: Whether or not this target port is enabled. > @@ -403,9 +403,9 @@ struct srpt_port_id { > * @gid: cached value of the port's gid. > * @work: work structure for refreshing the aforementioned cached values. > * @guid_name: port name in GUID format. > - * @port_guid_id: LIO target port information for the port name in GUID format. > + * @guid_id: LIO target port information for the port name in GUID format. > * @gid_name: port name in GID format. > - * @port_gid_id: LIO target port information for the port name in GID format. > + * @gid_id: LIO target port information for the port name in GID format. > * @port_attrib: Port attributes that can be accessed through configfs. > * @refcount: Number of objects associated with this port. > * @freed_channels: Completion that will be signaled once @refcount becomes 0. > @@ -422,9 +422,9 @@ struct srpt_port { > union ib_gid gid; > struct work_struct work; > char guid_name[64]; > - struct srpt_port_id port_guid_id; > + struct srpt_port_id *guid_id; > char gid_name[64]; > - struct srpt_port_id port_gid_id; > + struct srpt_port_id *gid_id; > struct srpt_port_attrib port_attrib; > atomic_t refcount; > struct completion *freed_channels; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-27 10:03 ` lizhijian @ 2022-07-27 17:48 ` Bart Van Assche 2022-07-28 5:22 ` lizhijian 0 siblings, 1 reply; 11+ messages in thread From: Bart Van Assche @ 2022-07-27 17:48 UTC (permalink / raw) To: lizhijian, Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Hillf Danton, Mike Christie On 7/27/22 03:03, lizhijian@fujitsu.com wrote: > I tested patches with blktests/srp, it works well and the UAF is gone. Hi Li, Thank you for having tested these patches so quickly :-) I assume that the above counts as a Tested-by? Bart. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free 2022-07-27 17:48 ` Bart Van Assche @ 2022-07-28 5:22 ` lizhijian 0 siblings, 0 replies; 11+ messages in thread From: lizhijian @ 2022-07-28 5:22 UTC (permalink / raw) To: Bart Van Assche, Jason Gunthorpe Cc: Leon Romanovsky, linux-rdma, Hillf Danton, Mike Christie On 28/07/2022 01:48, Bart Van Assche wrote: > On 7/27/22 03:03, lizhijian@fujitsu.com wrote: >> I tested patches with blktests/srp, it works well and the UAF is gone. > > Hi Li, > > Thank you for having tested these patches so quickly :-) I assume that the above counts as a Tested-by? Sure :) Thanks Zhijian > > Bart. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-28 5:22 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-26 21:21 [PATCH 0/3] Fix a use-after-free in the SRP target driver Bart Van Assche 2022-07-26 21:21 ` [PATCH 1/3] RDMA/srpt: Duplicate port name members Bart Van Assche 2022-07-26 21:21 ` [PATCH 2/3] RDMA/srpt: Introduce a reference count in struct srpt_device Bart Van Assche 2022-07-26 21:21 ` [PATCH 3/3] RDMA/srpt: Fix a use-after-free Bart Van Assche 2022-07-27 9:36 ` Leon Romanovsky 2022-07-27 12:26 ` Christoph Lameter 2022-07-27 12:31 ` Leon Romanovsky 2022-07-27 17:47 ` Bart Van Assche 2022-07-27 10:03 ` lizhijian 2022-07-27 17:48 ` Bart Van Assche 2022-07-28 5:22 ` lizhijian
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.