| 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
| ||
|
Message-ID: <10a0428b-a6da-7f66-c3a6-1e74fac34da5@oracle.com>
Date: Wed, 6 Mar 2019 09:55:01 -0800
From: Santosh Shilimkar <santosh.shilimkar@...cle.com>
To: Gerd Rausch <gerd.rausch@...cle.com>, netdev@...r.kernel.org,
Yanjun Zhu <yanjun.zhu@...cle.com>
Cc: David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net/rds: Accept peer connection reject messages due to
incompatible version
On 3/5/2019 11:04 PM, Gerd Rausch wrote:
> Prior to
> commit d021fabf525ff ("rds: rdma: add consumer reject")
>
> function "rds_rdma_cm_event_handler_cmn" would always honor a rejected
> connection attempt by issuing a "rds_conn_drop".
>
> The commit mentioned above added a "break", eliminating
> the "fallthrough" case and made the "rds_conn_drop" rather conditional:
>
> Now it only happens if a "consumer defined" reject (i.e. "rdma_reject")
> carries an integer-value of "1" inside "private_data":
>
>> if (!conn)
>> + break;
>> + err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
>> + if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
>> + pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n",
>> + &conn->c_laddr, &conn->c_faddr);
>> + conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
>> + rds_conn_drop(conn);
>> + }
>> rdsdebug("Connection rejected: %s\n",
>> rdma_reject_msg(cm_id, event->status));
>> + break;
>> /* FALLTHROUGH */
>
> A number of issues are worth mentioning here:
> #1) Previous versions of the RDS code simply rejected a connection
> by calling "rdma_reject(cm_id, NULL, 0);"
> So the value of the payload in "private_data" will not be "1",
> but "0".
>
> #2) Now the code has become dependent on host byte order and sizing.
> If one peer is big-endian, the other is little-endian,
> or there's a difference in sizeof(int) (e.g. ILP64 vs LP64),
> the *err check does not work as intended.
>
> #3) There is no check for "len" to see if the data behind *err is even valid.
> Luckily, it appears that the "rdma_reject(cm_id, NULL, 0)" will always
> carry 148 bytes of zeroized payload.
> But that should probably not be relied upon here.
>
> #4) With the added "break;",
> we might as well drop the misleading "/* FALLTHROUGH */" comment.
>
> This commit does _not_ address issue #2, as the sender would have to
> agree on a byte order as well.
>
> Here is the sequence of messages in this observed error-scenario:
> Host-A is pre-QoS changes (excluding the commit mentioned above)
> Host-B is post-QoS changes (including the commit mentioned above)
>
> #1 Host-B
> issues a connection request via function "rds_conn_path_transition"
> connection state transitions to "RDS_CONN_CONNECTING"
>
> #2 Host-A
> rejects the incompatible connection request (from #1)
> It does so by calling "rdma_reject(cm_id, NULL, 0);"
>
> #3 Host-B
> receives an "RDMA_CM_EVENT_REJECTED" event (from #2)
> But since the code is changed in the way described above,
> it won't drop the connection here, simply because "*err == 0".
>
> #4 Host-A
> issues a connection request
>
> #5 Host-B
> receives an "RDMA_CM_EVENT_CONNECT_REQUEST" event
> and ends up calling "rds_ib_cm_handle_connect".
> But since the state is already in "RDS_CONN_CONNECTING"
> (as of #1) it will end up issuing a "rdma_reject" without
> dropping the connection:
> if (rds_conn_state(conn) == RDS_CONN_CONNECTING) {
> /* Wait and see - our connect may still be succeeding */
> rds_ib_stats_inc(s_ib_connect_raced);
> }
> goto out;
>
> #6 Host-A
> receives an "RDMA_CM_EVENT_REJECTED" event (from #5),
> drops the connection and tries again (goto #4) until it gives up.
>
> Orabug: 29444532
>
> Signed-off-by: Gerd Rausch <gerd.rausch@...cle.com>
> ---
> net/rds/rdma_transport.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index 46bce8389066..f628e7fda66d 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -112,7 +112,7 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
> if (!conn)
> break;
> err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
> - if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
> + if (!err || (err && len >= sizeof(*err) && ((*err) <= RDS_RDMA_REJ_INCOMPAT))) {
> pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n",
> &conn->c_laddr, &conn->c_faddr);
> conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
> @@ -122,7 +122,6 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
> rdsdebug("Connection rejected: %s\n",
> rdma_reject_msg(cm_id, event->status));
> break;
> - /* FALLTHROUGH */
> case RDMA_CM_EVENT_ADDR_ERROR:
> case RDMA_CM_EVENT_ROUTE_ERROR:
> case RDMA_CM_EVENT_CONNECT_ERROR:
>
Very similar test diff [1] I sent Yanjun to test yesterday... Thanks for
checking. Will submit a cleaned up fix Gerd. Thansk for checking
From 82c638f11137e29220051c704b63e3d7ed7d44d0 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@...cle.com>
Date: Tue, 5 Mar 2019 18:19:52 -0800
Subject: [PATCH] TEST debug patch
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@...cle.com>
---
net/rds/rdma_transport.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 46bce83..c7b1fff 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -109,8 +109,10 @@ static int rds_rdma_cm_event_handler_cmn(struct
rdma_cm_id *cm_id,
break;
case RDMA_CM_EVENT_REJECTED:
+ err = (int *)event->param.conn.private_data;
if (!conn)
break;
+#if 0
err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
@@ -119,6 +121,20 @@ static int rds_rdma_cm_event_handler_cmn(struct
rdma_cm_id *cm_id,
conn->c_tos = 0;
rds_conn_drop(conn);
}
+#endif
+ if (event->status == RDS_REJ_CONSUMER_DEFINED &&
+ (*err) == RDS_RDMA_REJ_INCOMPAT) {
+
+ pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n",
+ &conn->c_laddr, &conn->c_faddr);
+ conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
+ conn->c_tos = 0;
+ rds_conn_drop(conn);
+ } else {
+ pr_debug("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
+ &conn->c_laddr, &conn->c_faddr);
+ rds_conn_drop(conn);
+ }
rdsdebug("Connection rejected: %s\n",
rdma_reject_msg(cm_id, event->status));
break;
--
1.7.1
Powered by blists - more mailing lists