From: Mike Christie <michaelc@cs.wisc.edu>
To: Xi Wang <xi.wang@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>,
open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
stable@vger.kernel.org
Subject: Re: [PATCH] iscsi_tcp: bound max_r2t in iscsi_sw_tcp_conn_set_param()
Date: Mon, 02 Jan 2012 01:05:55 -0600 [thread overview]
Message-ID: <4F015753.1010106@cs.wisc.edu> (raw)
In-Reply-To: <1325368867-13696-1-git-send-email-xi.wang@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]
Thanks for the patch.
On 12/31/2011 04:01 PM, Xi Wang wrote:
> A large max_r2t could lead to integer overflow in subsequent call to
> iscsi_tcp_r2tpool_alloc(), allocating a smaller buffer than expected
> and leading to out-of-bounds write.
Are you actually hitting this and if so with what tools and target? And
when using greater than 1 do you see any throughput improvements?
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/scsi/iscsi_tcp.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 7c34d8e..9a1bf21 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -687,7 +687,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_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;
> - int value;
> + int value = 0;
>
> switch(param) {
> case ISCSI_PARAM_HDRDGST_EN:
> @@ -700,7 +700,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
> break;
> case ISCSI_PARAM_MAX_R2T:
> sscanf(buf, "%d", &value);
> - if (value <= 0 || !is_power_of_2(value))
> + if (value <= 0 || value > 65536 || !is_power_of_2(value))
> return -EINVAL;
> if (session->max_r2t == value)
> break;
What about the attached patch? It also fixes cxgb*i.
[-- Attachment #2: 0001-libiscsi_tcp-fix-max_r2t-manipulation.patch --]
[-- Type: text/x-patch, Size: 5571 bytes --]
>From 1ff28b040e4e8ff5b490adde2028f05aaa0374f5 Mon Sep 17 00:00:00 2001
From: Mike Christie <michaelc@cs.wisc.edu>
Date: Mon, 2 Jan 2012 01:09:38 -0600
Subject: [PATCH] libiscsi_tcp: fix max_r2t manipulation
Problem description from Xi Wang:
A large max_r2t could lead to integer overflow in subsequent call to
iscsi_tcp_r2tpool_alloc(), allocating a smaller buffer than expected
and leading to out-of-bounds write.
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
drivers/scsi/cxgbi/libcxgbi.c | 13 ++-----------
drivers/scsi/iscsi_tcp.c | 13 +------------
drivers/scsi/libiscsi.c | 2 +-
drivers/scsi/libiscsi_tcp.c | 18 ++++++++++++++++++
include/scsi/libiscsi.h | 2 +-
include/scsi/libiscsi_tcp.h | 2 +-
6 files changed, 24 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index c10f74a..075705a 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -2141,11 +2141,10 @@ int cxgbi_set_conn_param(struct iscsi_cls_conn *cls_conn,
enum iscsi_param param, char *buf, int buflen)
{
struct iscsi_conn *conn = cls_conn->dd_data;
- struct iscsi_session *session = conn->session;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
struct cxgbi_conn *cconn = tcp_conn->dd_data;
struct cxgbi_sock *csk = cconn->cep->csk;
- int value, err = 0;
+ int err;
log_debug(1 << CXGBI_DBG_ISCSI,
"cls_conn 0x%p, param %d, buf(%d) %s.\n",
@@ -2167,15 +2166,7 @@ int cxgbi_set_conn_param(struct iscsi_cls_conn *cls_conn,
conn->datadgst_en, 0);
break;
case ISCSI_PARAM_MAX_R2T:
- sscanf(buf, "%d", &value);
- if (value <= 0 || !is_power_of_2(value))
- return -EINVAL;
- if (session->max_r2t == value)
- break;
- iscsi_tcp_r2tpool_free(session);
- err = iscsi_set_param(cls_conn, param, buf, buflen);
- if (!err && iscsi_tcp_r2tpool_alloc(session))
- return -ENOMEM;
+ return iscsi_tcp_set_max_r2t(conn, buf);
case ISCSI_PARAM_MAX_RECV_DLENGTH:
err = iscsi_set_param(cls_conn, param, buf, buflen);
if (!err)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 7c34d8e..0a544bf 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -684,10 +684,8 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
int buflen)
{
struct iscsi_conn *conn = cls_conn->dd_data;
- 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;
- int value;
switch(param) {
case ISCSI_PARAM_HDRDGST_EN:
@@ -699,16 +697,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
break;
case ISCSI_PARAM_MAX_R2T:
- sscanf(buf, "%d", &value);
- if (value <= 0 || !is_power_of_2(value))
- return -EINVAL;
- if (session->max_r2t == value)
- break;
- iscsi_tcp_r2tpool_free(session);
- iscsi_set_param(cls_conn, param, buf, buflen);
- if (iscsi_tcp_r2tpool_alloc(session))
- return -ENOMEM;
- break;
+ return iscsi_tcp_set_max_r2t(conn, buf);
default:
return iscsi_set_param(cls_conn, param, buf, buflen);
}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 143bbe4..911a4a9 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3200,7 +3200,7 @@ int iscsi_set_param(struct iscsi_cls_conn *cls_conn,
sscanf(buf, "%d", &session->initial_r2t_en);
break;
case ISCSI_PARAM_MAX_R2T:
- sscanf(buf, "%d", &session->max_r2t);
+ sscanf(buf, "%hu", &session->max_r2t);
break;
case ISCSI_PARAM_IMM_DATA_EN:
sscanf(buf, "%d", &session->imm_data_en);
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 5715a3d..24f812d 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -1170,6 +1170,24 @@ void iscsi_tcp_r2tpool_free(struct iscsi_session *session)
}
EXPORT_SYMBOL_GPL(iscsi_tcp_r2tpool_free);
+int iscsi_tcp_set_max_r2t(struct iscsi_conn *conn, char*buf)
+{
+ struct iscsi_session *session = conn->session;
+ unsigned short r2ts = 0;
+
+ sscanf(buf, "%hu", &r2ts);
+ if (session->max_r2t == r2ts)
+ return 0;
+
+ if (!r2ts || !is_power_of_2(r2ts))
+ return -EINVAL;
+
+ session->max_r2t = r2ts;
+ iscsi_tcp_r2tpool_free(session);
+ return iscsi_tcp_r2tpool_alloc(session);
+}
+EXPORT_SYMBOL_GPL(iscsi_tcp_set_max_r2t);
+
void iscsi_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn,
struct iscsi_stats *stats)
{
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index cedcff3..a84c1ad 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -268,7 +268,7 @@ struct iscsi_session {
int lu_reset_timeout;
int tgt_reset_timeout;
int initial_r2t_en;
- unsigned max_r2t;
+ unsigned short max_r2t;
int imm_data_en;
unsigned first_burst;
unsigned max_burst;
diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
index ac0cc1d..215469a 100644
--- a/include/scsi/libiscsi_tcp.h
+++ b/include/scsi/libiscsi_tcp.h
@@ -128,7 +128,7 @@ extern void iscsi_tcp_conn_teardown(struct iscsi_cls_conn *cls_conn);
/* misc helpers */
extern int iscsi_tcp_r2tpool_alloc(struct iscsi_session *session);
extern void iscsi_tcp_r2tpool_free(struct iscsi_session *session);
-
+extern int iscsi_tcp_set_max_r2t(struct iscsi_conn *conn, char *buf);
extern void iscsi_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn,
struct iscsi_stats *stats);
#endif /* LIBISCSI_TCP_H */
--
1.7.6
next prev parent reply other threads:[~2012-01-02 7:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-31 22:01 [PATCH] iscsi_tcp: bound max_r2t in iscsi_sw_tcp_conn_set_param() Xi Wang
2012-01-02 7:05 ` Mike Christie [this message]
2012-01-02 14:18 ` Xi Wang
2012-01-04 6:36 ` Xi Wang
2012-01-04 7:16 ` Mike Christie
2012-01-04 7:12 ` Xi Wang
2012-02-09 16:04 ` Greg KH
2012-02-09 18:04 ` Xi Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F015753.1010106@cs.wisc.edu \
--to=michaelc@cs.wisc.edu \
--cc=JBottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=open-iscsi@googlegroups.com \
--cc=stable@vger.kernel.org \
--cc=xi.wang@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.