| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 2 Aug 2022 11:25:48 -0500
From: Mike Christie <michael.christie@...cle.com>
To: "lijinlin (A)" <lijinlin3@...wei.com>,
"lduncan@...e.com" <lduncan@...e.com>,
"cleech@...hat.com" <cleech@...hat.com>,
"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"mark.mielke@...il.com" <mark.mielke@...il.com>
Cc: "open-iscsi@...glegroups.com" <open-iscsi@...glegroups.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linfeilong <linfeilong@...wei.com>,
"liuzhiqiang (I)" <liuzhiqiang26@...wei.com>
Subject: Re: [PATCH] scsi: iscsi: iscsi_tcp: Fix null-ptr-deref while calling
getpeername()
On 8/2/22 6:23 AM, lijinlin (A) wrote:
> So sorry, this patch has problem, please ignore.
>
Was the issue the fget use?
I know I gave the suggestion to do the get, but seeing it now makes
me think I was wrong and it's getting too messy.
Let's just add a mutex for getting/setting the tcp_sw_conn->sock in
the non-io paths (io paths are flushed/locked already). Something like
this (patch is only compile tested):
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9fee70d6434a..c1696472965e 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -558,6 +558,8 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session *cls_session,
tcp_conn = conn->dd_data;
tcp_sw_conn = tcp_conn->dd_data;
+ mutex_init(&tcp_sw_conn->sock_lock);
+
tfm = crypto_alloc_ahash("crc32c", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
goto free_conn;
@@ -592,11 +594,15 @@ iscsi_sw_tcp_conn_create(struct iscsi_cls_session *cls_session,
static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
{
- struct iscsi_session *session = conn->session;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
struct socket *sock = tcp_sw_conn->sock;
+ /*
+ * The iscsi transport class will make sure we are not called in
+ * parallel with start, stop, bind and destroys. However, this can be
+ * called twice if userspace does a stop then a destroy.
+ */
if (!sock)
return;
@@ -610,9 +616,9 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn)
iscsi_sw_tcp_conn_restore_callbacks(conn);
sock_put(sock->sk);
- spin_lock_bh(&session->frwd_lock);
+ mutex_lock(&tcp_sw_conn->sock_lock);
tcp_sw_conn->sock = NULL;
- spin_unlock_bh(&session->frwd_lock);
+ mutex_unlock(&tcp_sw_conn->sock_lock);
sockfd_put(sock);
}
@@ -664,7 +670,6 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
struct iscsi_cls_conn *cls_conn, uint64_t transport_eph,
int is_leading)
{
- struct iscsi_session *session = cls_session->dd_data;
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
@@ -684,10 +689,10 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session,
if (err)
goto free_socket;
- spin_lock_bh(&session->frwd_lock);
+ mutex_lock(&tcp_sw_conn->sock_lock);
/* bind iSCSI connection and socket */
tcp_sw_conn->sock = sock;
- spin_unlock_bh(&session->frwd_lock);
+ mutex_unlock(&tcp_sw_conn->sock_lock);
/* setup Socket parameters */
sk = sock->sk;
@@ -724,8 +729,15 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
break;
case ISCSI_PARAM_DATADGST_EN:
iscsi_set_param(cls_conn, param, buf, buflen);
+
+ mutex_lock(&tcp_sw_conn->sock_lock);
+ if (!tcp_sw_conn->sock) {
+ mutex_unlock(&tcp_sw_conn->sock_lock);
+ return -ENOTCONN;
+ }
tcp_sw_conn->sendpage = conn->datadgst_en ?
sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
+ mutex_unlock(&tcp_sw_conn->sock_lock);
break;
case ISCSI_PARAM_MAX_R2T:
return iscsi_tcp_set_max_r2t(conn, buf);
@@ -750,14 +762,20 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
case ISCSI_PARAM_CONN_PORT:
case ISCSI_PARAM_CONN_ADDRESS:
case ISCSI_PARAM_LOCAL_PORT:
- spin_lock_bh(&conn->session->frwd_lock);
- if (!tcp_sw_conn || !tcp_sw_conn->sock) {
- spin_unlock_bh(&conn->session->frwd_lock);
+ /*
+ * The conn's sysfs interface is exposed to userspace after
+ * the tcp_sw_conn is setup, and the netlink interface will
+ * make sure we don't do a get_param while setup is running.
+ * We do need to make sure a user is not accessing sysfs while
+ * the netlink interface is releasing the sock via
+ * iscsi_sw_tcp_release_conn.
+ */
+ mutex_lock(&tcp_sw_conn->sock_lock);
+ sock = tcp_sw_conn->sock;
+ if (!sock) {
+ mutex_unlock(&tcp_sw_conn->sock_lock);
return -ENOTCONN;
}
- sock = tcp_sw_conn->sock;
- sock_hold(sock->sk);
- spin_unlock_bh(&conn->session->frwd_lock);
if (param == ISCSI_PARAM_LOCAL_PORT)
rc = kernel_getsockname(sock,
@@ -765,7 +783,7 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
else
rc = kernel_getpeername(sock,
(struct sockaddr *)&addr);
- sock_put(sock->sk);
+ mutex_unlock(&tcp_sw_conn->sock_lock);
if (rc < 0)
return rc;
@@ -803,17 +821,25 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost,
}
tcp_conn = conn->dd_data;
tcp_sw_conn = tcp_conn->dd_data;
+ /*
+ * If the leadconn is bound then setup has completed and destroy
+ * has not been run yet. Grab a ref to the conn incase a destroy
+ * starts to run after we drop the fwrd_lock.
+ */
+ iscsi_get_conn(conn->cls_conn);
+ spin_unlock_bh(&session->frwd_lock);
+
+ mutex_lock(&tcp_sw_conn->sock_lock);
sock = tcp_sw_conn->sock;
if (!sock) {
- spin_unlock_bh(&session->frwd_lock);
+ mutex_unlock(&tcp_sw_conn->sock_lock);
+ iscsi_put_conn(conn->cls_conn);
return -ENOTCONN;
}
- sock_hold(sock->sk);
- spin_unlock_bh(&session->frwd_lock);
-
- rc = kernel_getsockname(sock,
- (struct sockaddr *)&addr);
- sock_put(sock->sk);
+
+ rc = kernel_getsockname(sock, (struct sockaddr *)&addr);
+ mutex_unlock(&tcp_sw_conn->sock_lock);
+ iscsi_put_conn(conn->cls_conn);
if (rc < 0)
return rc;
diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h
index 791453195099..7c6f90ce6124 100644
--- a/drivers/scsi/iscsi_tcp.h
+++ b/drivers/scsi/iscsi_tcp.h
@@ -28,6 +28,8 @@ struct iscsi_sw_tcp_send {
struct iscsi_sw_tcp_conn {
struct socket *sock;
+ /* Taken when accesing the sock from the netlink/sysfs interface */
+ struct mutex sock_lock;
struct iscsi_sw_tcp_send out;
/* old values for socket callbacks */
Powered by blists - more mailing lists