netdev - Re: [net-next PATCH v2] net/rds: Accept peer connection reject messages due to incompatible version

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <2115f386-46fd-a9af-d126-4eeb6cc01ec0@oracle.com>
Date:   Wed, 6 Mar 2019 17:55:53 -0800
From:   Santosh Shilimkar <santosh.shilimkar@...cle.com>
To:     Gerd Rausch <gerd.rausch@...cle.com>,
        Yanjun Zhu <yanjun.zhu@...cle.com>, netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH v2] net/rds: Accept peer connection reject
 messages due to incompatible version



On 3/6/2019 5:49 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.
> 
> Fixes: d021fabf525ff ("rds: rdma: add consumer reject")
> Signed-off-by: Gerd Rausch <gerd.rausch@...cle.com>
> ---
>   net/rds/rdma_transport.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Changes in submitted patch v2:
> * Dropped the "Orabug:" line from the commit-log message (as requested)
> * Added a "Fixes:" line to the commit-log-message
> 
Thanks Gerd for posting an update. The fix looks correct as already
mentioned in earlier post.
FWIW,
Acked-by: Santosh Shilimkar<santosh.shilimkar@...cle.com>

Hi Yanjun,
Please provide your tested-by since you mentioned offlist that
so far you are unable to reproduce the issue on net-next. Thanks.

Regards,
Santosh