diff mbox series

[2/2] scsi: iscsi_tcp: Fix use-after-free in iscsi_sw_tcp_host_get_param()

Message ID 20210407012450.97754-3-haowenchao@huawei.com
State New
Headers show
Series Fix use-after-free in iscsi_sw_tcp_host_get_param() | expand

Commit Message

Wenchao Hao April 7, 2021, 1:24 a.m. UTC
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(-)

Comments

Mike Christie April 12, 2021, 5:19 p.m. UTC | #1
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.
diff mbox series

Patch

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)