[PATCH 0/3] Fix a use-after-free in the SRP target driver

[PATCH 0/3] Fix a use-after-free in the SRP target driver

[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(-)

[PATCH 1/3] RDMA/srpt: Duplicate port name members

[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;

[PATCH 2/3] RDMA/srpt: Introduce a reference count in struct srpt_device

[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;

[PATCH 3/3] RDMA/srpt: Fix a use-after-free

[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;

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[Leon Romanovsky]

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

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[lizhijian]

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;

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[Christoph Lameter]

On Wed, 27 Jul 2022, Leon Romanovsky wrote:

> Please no BUG_ON() in new code.

Do we now prefer NULL pointer dereferences causing bugs?

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[Leon Romanovsky]

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

> 

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[Bart Van Assche]

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.

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[Bart Van Assche]

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.

Re: [PATCH 3/3] RDMA/srpt: Fix a use-after-free

[lizhijian]

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.