diff mbox

libceph: validate con->state at the top of try_write()

Message ID CAOi1vP_Xp4ehJKFCfbMB3UiUi2U2vMxqSwTbZ_hF-n5hC6hQ+A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ilya Dryomov April 26, 2018, 4:11 p.m. UTC 
On Wed, Apr 25, 2018 at 5:44 PM, Jason Dillaman <jdillama@redhat.com> wrote:
> On Wed, Apr 25, 2018 at 6:28 AM, Ilya Dryomov <idryomov@gmail.com> wrote:
>> ceph_con_workfn() validates con->state before calling try_read() and
>> then try_write().  However, try_read() temporarily releases con->mutex,
>> notably in process_message() and ceph_con_in_msg_alloc(), opening the
>> window for ceph_con_close() to sneak in, close the connection and
>> release con->sock.  When try_write() is called on the assumption that
>> con->state is still valid (i.e. not STANDBY or CLOSED), a NULL sock
>> gets passed to the networking stack:
>>
>>   BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
>>   IP: selinux_socket_sendmsg+0x5/0x20
>>
>> Make sure con->state is valid at the top of try_write() and add an
>> explicit BUG_ON for this, similar to try_read().
>>
>> Cc: stable@vger.kernel.org
>> Link: https://tracker.ceph.com/issues/23706
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>>  net/ceph/messenger.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>> index fcb40c12b1f8..a7bfc07d2876 100644
>> --- a/net/ceph/messenger.c
>> +++ b/net/ceph/messenger.c
>> @@ -2569,6 +2569,10 @@ static int try_write(struct ceph_connection *con)
>>         int ret = 1;
>>
>>         dout("try_write start %p state %lu\n", con, con->state);
>> +       if (con->state != CON_STATE_PREOPEN &&
>> +           con->state != CON_STATE_NEGOTIATING &&
>> +           con->state != CON_STATE_OPEN)
>> +               return 0;
>>
>>  more:
>>         dout("try_write out_kvec_bytes %d\n", con->out_kvec_bytes);
>> @@ -2594,6 +2598,8 @@ static int try_write(struct ceph_connection *con)
>>         }
>>
>>  more_kvec:
>> +       BUG_ON(!con->sock);
>> +
>>         /* kvec data queued? */
>>         if (con->out_kvec_left) {
>>                 ret = write_partial_kvec(con);
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Reviewed-by: Jason Dillaman <dillaman@redhat.com>

Actually, even though try_write() is where PREOPEN -> CONNECTING
transition happens, it is technically possible for try_write() to be
called in CONNECTING.  Denying it shouldn't break anything, but no
point in being overly restrictive here.  I have updated the patch:


Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index a7bfc07d2876..3b3d33ea9ed8 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2570,6 +2570,7 @@  static int try_write(struct ceph_connection *con)

        dout("try_write start %p state %lu\n", con, con->state);
        if (con->state != CON_STATE_PREOPEN &&
+           con->state != CON_STATE_CONNECTING &&
            con->state != CON_STATE_NEGOTIATING &&
            con->state != CON_STATE_OPEN)
                return 0;