iscsi-target: Fix wrong buffer / buffer overrun in iscsi_change_param_value()

In non-leading connection login, iscsi_login_non_zero_tsih_s1() calls
iscsi_change_param_value() with the buffer it uses to hold the login
PDU, not a temporary buffer.  This leads to the login header getting
corrupted and login failing for non-leading connections in MC/S.

Fix this by adding a wrapper iscsi_change_param_sprintf() that handles
the temporary buffer itself to avoid confusion.  Also handle sending a
reject in case of failure in the wrapper, which lets the calling code
get quite a bit smaller and easier to read.

Finally, bump the size of the temporary buffer from 32 to 64 bytes to be
safe, since "MaxRecvDataSegmentLength=" by itself is 25 bytes; with a
trailing NUL, a value >= 1M will lead to a buffer overrun.  (This isn't
the default but we don't need to run right at the ragged edge here)

Reported-by: Santosh Kulkarni <santosh.kulkarni@calsoftinc.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

1 files changed, 31 insertions, 39 deletions

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index ca31fa1b8a4b..d9b1d88e1ad3 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -249,6 +249,28 @@ static void iscsi_login_set_conn_values(
         mutex_unlock(&auth_id_lock);
 }
 
+static __printf(2, 3) int iscsi_change_param_sprintf(
+        struct iscsi_conn *conn,
+        const char *fmt, ...)
+{
+        va_list args;
+        unsigned char buf[64];
+
+        memset(buf, 0, sizeof buf);
+
+        va_start(args, fmt);
+        vsnprintf(buf, sizeof buf, fmt, args);
+        va_end(args);
+
+        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
+                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
+                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+                return -1;
+        }
+
+        return 0;
+}
+
 /*
  *      This is the leading connection of a new session,
  *      or session reinstatement.
@@ -339,7 +361,6 @@ static int iscsi_login_zero_tsih_s2(
 {
         struct iscsi_node_attrib *na;
         struct iscsi_session *sess = conn->sess;
-        unsigned char buf[32];
         bool iser = false;
 
         sess->tpg = conn->tpg;
@@ -380,26 +401,16 @@ static int iscsi_login_zero_tsih_s2(
          *
          * In our case, we have already located the struct iscsi_tiqn at this point.
          */
-        memset(buf, 0, 32);
-        sprintf(buf, "TargetPortalGroupTag=%hu", sess->tpg->tpgt);
-        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+        if (iscsi_change_param_sprintf(conn, "TargetPortalGroupTag=%hu", sess->tpg->tpgt))
                 return -1;
-        }
 
         /*
          * Workaround for Initiators that have broken connection recovery logic.
          *
          * "We would really like to get rid of this." Linux-iSCSI.org team
          */
-        memset(buf, 0, 32);
-        sprintf(buf, "ErrorRecoveryLevel=%d", na->default_erl);
-        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+        if (iscsi_change_param_sprintf(conn, "ErrorRecoveryLevel=%d", na->default_erl))
                 return -1;
-        }
 
         if (iscsi_login_disable_FIM_keys(conn->param_list, conn) < 0)
                 return -1;
@@ -411,12 +422,9 @@ static int iscsi_login_zero_tsih_s2(
                 unsigned long mrdsl, off;
                 int rc;
 
-                sprintf(buf, "RDMAExtensions=Yes");
-                if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                        iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+                if (iscsi_change_param_sprintf(conn, "RDMAExtensions=Yes"))
                         return -1;
-                }
+
                 /*
                  * Make MaxRecvDataSegmentLength PAGE_SIZE aligned for
                  * Immediate Data + Unsolicitied Data-OUT if necessary..
@@ -446,12 +454,8 @@ static int iscsi_login_zero_tsih_s2(
                 pr_warn("Aligning ISER MaxRecvDataSegmentLength: %lu down"
                         " to PAGE_SIZE\n", mrdsl);
 
-                sprintf(buf, "MaxRecvDataSegmentLength=%lu\n", mrdsl);
-                if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                        iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+                if (iscsi_change_param_sprintf(conn, "MaxRecvDataSegmentLength=%lu\n", mrdsl))
                         return -1;
-                }
                 /*
                  * ISER currently requires that ImmediateData + Unsolicited
                  * Data be disabled when protection / signature MRs are enabled.
@@ -461,19 +465,12 @@ check_prot:
                    (TARGET_PROT_DOUT_STRIP | TARGET_PROT_DOUT_PASS |
                     TARGET_PROT_DOUT_INSERT)) {
 
-                        sprintf(buf, "ImmediateData=No");
-                        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                                    ISCSI_LOGIN_STATUS_NO_RESOURCES);
+                        if (iscsi_change_param_sprintf(conn, "ImmediateData=No"))
                                 return -1;
-                        }
 
-                        sprintf(buf, "InitialR2T=Yes");
-                        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                                    ISCSI_LOGIN_STATUS_NO_RESOURCES);
+                        if (iscsi_change_param_sprintf(conn, "InitialR2T=Yes"))
                                 return -1;
-                        }
+
                         pr_debug("Forcing ImmediateData=No + InitialR2T=Yes for"
                                  " T10-PI enabled ISER session\n");
                 }
@@ -618,13 +615,8 @@ static int iscsi_login_non_zero_tsih_s2(
          *
          * In our case, we have already located the struct iscsi_tiqn at this point.
          */
-        memset(buf, 0, 32);
-        sprintf(buf, "TargetPortalGroupTag=%hu", sess->tpg->tpgt);
-        if (iscsi_change_param_value(buf, conn->param_list, 0) < 0) {
-                iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-                                ISCSI_LOGIN_STATUS_NO_RESOURCES);
+        if (iscsi_change_param_sprintf(conn, "TargetPortalGroupTag=%hu", sess->tpg->tpgt))
                 return -1;
-        }
 
         return iscsi_login_disable_FIM_keys(conn->param_list, conn);
 }