[PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly

[Wenchao Hao]

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

The origin implement do device setup in iscsi_create_conn() which
bind the alloc/init and add in one function; do device teardown in
iscsi_destroy_conn() which bind remove and free in one function.

This implement makes it impossible to initialize resources of device
before add it to sysfs during setup.

So this patchset splict both the setup and teradown of iscsi_cls_conn to
2 steps.

For setup flow, we should call iscsi_alloc_conn() and initialize some
resources, then call iscsi_add_conn().

For teradown flow, we should call iscsi_remove_conn() to remove device
and free resources which related to iscsi_cls_conn, then call
iscsi_put_conn() to free iscsi_cls_conn.

V2 -> V3:
* Fix some bugs and optimization the code implement.

V1 -> V2:
* add two more iscsi_free_conn() and iscsi_remove_conn() than V1
* change the teardown flow of iscsi_cls_conn

Wenchao Hao (3):
scsi: iscsi: Add helper functions to manage iscsi_cls_conn
scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
scsi:libiscsi: teradown iscsi_cls_conn gracefully

drivers/scsi/libiscsi.c | 23 +++++---
drivers/scsi/scsi_transport_iscsi.c | 90 +++++++++++++++--------------
include/scsi/scsi_transport_iscsi.h | 5 +-
3 files changed, 66 insertions(+), 52 deletions(-)

--
2.32.0

[Wenchao Hao]

commit 1b8d0300a3e9 ("scsi: libiscsi: Fix UAF in
iscsi_conn_get_param()/iscsi_conn_teardown()") fixed an UAF in
iscsi_conn_get_param() and introduced 2 tmp_xxx varibles, the
implement looks ugly.

We can fix this UAF with the help of device_del() gracefully.
Call iscsi_remove_conn() at the beginning of iscsi_conn_teardown would
make userspace unable to see iscsi_cls_conn any more, then we can free
memory safely.
And remove iscsi_destroy_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---
drivers/scsi/libiscsi.c | 10 +++++-----
drivers/scsi/scsi_transport_iscsi.c | 27 +++++----------------------
include/scsi/scsi_transport_iscsi.h | 1 -
3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 38e68b449c08..da583a39b307 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3109,8 +3109,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
{
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_session *session = conn->session;
- char *tmp_persistent_address = conn->persistent_address;
- char *tmp_local_ipaddr = conn->local_ipaddr;
+
+ iscsi_remove_conn(cls_conn);

del_timer_sync(&conn->transport_timer);

@@ -3132,6 +3132,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
spin_lock_bh(&session->frwd_lock);
free_pages((unsigned long) conn->data,
get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+ kfree(conn->persistent_address);
+ kfree(conn->local_ipaddr);
/* regular RX path uses back_lock */
spin_lock_bh(&session->back_lock);
kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
@@ -3142,9 +3144,7 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
spin_unlock_bh(&session->frwd_lock);
mutex_unlock(&session->eh_mutex);

- iscsi_destroy_conn(cls_conn);
- kfree(tmp_persistent_address);
- kfree(tmp_local_ipaddr);
+ iscsi_put_conn(cls_conn);
}
EXPORT_SYMBOL_GPL(iscsi_conn_teardown);

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index fc33000a6af9..d0f996d14782 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2165,7 +2165,11 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, void *data)
{
if (!iscsi_is_conn_dev(dev))
return 0;
- return iscsi_destroy_conn(iscsi_dev_to_conn(dev));
+
+ iscsi_remove_conn(iscsi_dev_to_conn(dev));
+ iscsi_put_conn(iscsi_dev_to_conn(dev));
+
+ return 0;
}

void iscsi_remove_session(struct iscsi_cls_session *session)
@@ -2437,27 +2441,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
}
EXPORT_SYMBOL_GPL(iscsi_remove_conn);

-/**
- * iscsi_destroy_conn - destroy iscsi class connection
- * @conn: iscsi cls session
- *
- * This can be called from a LLD or iscsi_transport.
- */
-int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&connlock, flags);
- list_del(&conn->conn_list);
- spin_unlock_irqrestore(&connlock, flags);
-
- transport_unregister_device(&conn->dev);
- ISCSI_DBG_TRANS_CONN(conn, "Completing conn destruction\n");
- device_unregister(&conn->dev);
- return 0;
-}
-EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
-
void iscsi_put_conn(struct iscsi_cls_conn *conn)
{
put_device(&conn->dev);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 4af6768e8195..aedbc4488149 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -447,7 +447,6 @@ extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
-extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
extern void iscsi_unblock_session(struct iscsi_cls_session *session);
extern void iscsi_block_session(struct iscsi_cls_session *session);
extern int iscsi_scan_finished(struct Scsi_Host *shost, unsigned long time);
--
2.32.0

[Wenchao Hao]

iscsi_create_conn() would expose iscsi_cls_conn to sysfs, while the
initialization of iscsi_conn's dd_data is not ready now. When userspace
try to access an attribute such as connect's address, it might cause
a NULL pointer dereference.

So we should add iscsi_cls_conn to sysfs until it has been initialized.
And remove iscsi_create_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao <haowe...@huawei.com>
Signed-off-by: Wu Bo <wub...@huawei.com>
---

drivers/scsi/libiscsi.c | 13 ++++-
drivers/scsi/scsi_transport_iscsi.c | 76 -----------------------------
include/scsi/scsi_transport_iscsi.h | 2 -
3 files changed, 11 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..38e68b449c08 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3040,8 +3040,9 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
struct iscsi_conn *conn;
struct iscsi_cls_conn *cls_conn;
char *data;
+ int err;

- cls_conn = iscsi_create_conn(cls_session, sizeof(*conn) + dd_size,
+ cls_conn = iscsi_alloc_conn(cls_session, sizeof(*conn) + dd_size,
conn_idx);
if (!cls_conn)
return NULL;
@@ -3078,13 +3079,21 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, int dd_size,
goto login_task_data_alloc_fail;
conn->login_task->data = conn->data = data;

+ err = iscsi_add_conn(cls_conn);
+ if (err)
+ goto login_task_add_dev_fail;
+
return cls_conn;

+login_task_add_dev_fail:
+ free_pages((unsigned long) conn->data,
+ get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+
login_task_data_alloc_fail:
kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
sizeof(void*));
login_task_alloc_fail:
- iscsi_destroy_conn(cls_conn);
+ iscsi_put_conn(cls_conn);
return NULL;
}
EXPORT_SYMBOL_GPL(iscsi_conn_setup);
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 65117ed5626e..fc33000a6af9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2437,82 +2437,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
}
EXPORT_SYMBOL_GPL(iscsi_remove_conn);

-/**
- * iscsi_create_conn - create iscsi class connection
- * @session: iscsi cls session
- * @dd_size: private driver data size
- * @cid: connection id
- *
- * This can be called from a LLD or iscsi_transport. The connection
- * is child of the session so cid must be unique for all connections
- * on the session.
- *
- * Since we do not support MCS, cid will normally be zero. In some cases
- * for software iscsi we could be trying to preallocate a connection struct
- * in which case there could be two connection structs and cid would be
- * non-zero.
- */
-struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
-{
- struct iscsi_transport *transport = session->transport;
- struct iscsi_cls_conn *conn;
- unsigned long flags;
- int err;
-
- conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
- if (!conn)
- return NULL;
- if (dd_size)
- conn->dd_data = &conn[1];
-
- mutex_init(&conn->ep_mutex);
- INIT_LIST_HEAD(&conn->conn_list);
- INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn);
- conn->transport = transport;
- conn->cid = cid;
- conn->state = ISCSI_CONN_DOWN;
-
- /* this is released in the dev's release function */
- if (!get_device(&session->dev))
- goto free_conn;
-
- dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
- conn->dev.parent = &session->dev;
- conn->dev.release = iscsi_conn_release;
- err = device_register(&conn->dev);
- if (err) {
- iscsi_cls_session_printk(KERN_ERR, session, "could not "
- "register connection's dev\n");
- goto release_parent_ref;
- }
- err = transport_register_device(&conn->dev);
- if (err) {
- iscsi_cls_session_printk(KERN_ERR, session, "could not "
- "register transport's dev\n");
- goto release_conn_ref;
- }
-
- spin_lock_irqsave(&connlock, flags);
- list_add(&conn->conn_list, &connlist);
- spin_unlock_irqrestore(&connlock, flags);
-
- ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
- return conn;
-
-release_conn_ref:
- device_unregister(&conn->dev);
- put_device(&session->dev);
- return NULL;
-release_parent_ref:
- put_device(&session->dev);
-free_conn:
- kfree(conn);
- return NULL;
-}
-
-EXPORT_SYMBOL_GPL(iscsi_create_conn);
-
/**

* iscsi_destroy_conn - destroy iscsi class connection

* @conn: iscsi cls session

diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index ae686addde0c..4af6768e8195 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -445,8 +445,6 @@ extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
int dd_size, uint32_t cid);

extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);

-extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
- int dd_size, uint32_t cid);

extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
extern void iscsi_get_conn(struct iscsi_cls_conn *conn);

extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
--
2.32.0

[Mike Christie]

Nice. Thanks.

Reviewed-by: Mike Christie <michael....@oracle.com>

[Wenchao Hao]

On 2022/3/10 9:57, Wenchao Hao wrote:
> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
>
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
>
> This implement makes it impossible to initialize resources of device
> before add it to sysfs during setup.
>
> So this patchset splict both the setup and teradown of iscsi_cls_conn to
> 2 steps.
>
> For setup flow, we should call iscsi_alloc_conn() and initialize some
> resources, then call iscsi_add_conn().
>
> For teradown flow, we should call iscsi_remove_conn() to remove device
> and free resources which related to iscsi_cls_conn, then call
> iscsi_put_conn() to free iscsi_cls_conn.
>

Friendly ping...

[Martin K. Petersen]

Wenchao,

> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.

Applied to 5.18/scsi-staging, thanks!

--
Martin K. Petersen Oracle Linux Engineering

[Martin K. Petersen]

On Wed, 9 Mar 2022 20:57:56 -0500, Wenchao Hao wrote:

> We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
> the root reason is we did sysfs addition wrong.
>
> The origin implement do device setup in iscsi_create_conn() which
> bind the alloc/init and add in one function; do device teardown in
> iscsi_destroy_conn() which bind remove and free in one function.
>

> [...]

Applied to 5.18/scsi-queue, thanks!

[1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn
https://git.kernel.org/mkp/scsi/c/ad515cada7da
[2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
https://git.kernel.org/mkp/scsi/c/7dae459f5e56
[3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully
https://git.kernel.org/mkp/scsi/c/8709c323091b