Skip to content

Commit

Permalink
mptcp: fix UaF in listener shutdown
Browse files Browse the repository at this point in the history
As reported by Christoph, the mptcp listener shutdown path is prone
to an UaF issue.

    BUG: KASAN: use-after-free in _raw_spin_lock_bh+0x73/0xe0
    Write of size 4 at addr ffff88810cb23098 by task syz-executor731/1266

    CPU: 1 PID: 1266 Comm: syz-executor731 Not tainted 6.2.0-rc59af4eaa31c1f6c00c8f1e448ed99a45c66340dd5 #6
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
    Call Trace:
     <TASK>
     dump_stack_lvl+0x6e/0x91
     print_report+0x16a/0x46f
     kasan_report+0xad/0x130
     kasan_check_range+0x14a/0x1a0
     _raw_spin_lock_bh+0x73/0xe0
     subflow_error_report+0x6d/0x110
     sk_error_report+0x3b/0x190
     tcp_disconnect+0x138c/0x1aa0
     inet_child_forget+0x6f/0x2e0
     inet_csk_listen_stop+0x209/0x1060
     __mptcp_close_ssk+0x52d/0x610
     mptcp_destroy_common+0x165/0x640
     mptcp_destroy+0x13/0x80
     __mptcp_destroy_sock+0xe7/0x270
     __mptcp_close+0x70e/0x9b0
     mptcp_close+0x2b/0x150
     inet_release+0xe9/0x1f0
     __sock_release+0xd2/0x280
     sock_close+0x15/0x20
     __fput+0x252/0xa20
     task_work_run+0x169/0x250
     exit_to_user_mode_prepare+0x113/0x120
     syscall_exit_to_user_mode+0x1d/0x40
     do_syscall_64+0x48/0x90
     entry_SYSCALL_64_after_hwframe+0x72/0xdc

The msk grace period can legitly expire in between the last
reference count dropped in mptcp_subflow_queue_clean() and
the later eventual access in inet_csk_listen_stop()

After the previous patch we don't need anymore special-casing
msk listener socket cleanup: the mptcp worker will process each
of the unaccepted msk sockets.

Just drop the now unnecessary code.

Fixes: 6aeed90 ("mptcp: fix race on unaccepted mptcp sockets")
Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com>
Closes: #346
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Paolo Abeni authored and matttbe committed Feb 17, 2023
1 parent 2f407c3 commit 2a20739
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 74 deletions.
1 change: 0 additions & 1 deletion net/mptcp/protocol.c
Original file line number Diff line number Diff line change
Expand Up @@ -2357,7 +2357,6 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
/* otherwise tcp will dispose of the ssk and subflow ctx */
if (ssk->sk_state == TCP_LISTEN) {
tcp_set_state(ssk, TCP_CLOSE);
mptcp_subflow_queue_clean(sk, ssk);
inet_csk_listen_stop(ssk);
mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CLOSED);
}
Expand Down
1 change: 0 additions & 1 deletion net/mptcp/protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,6 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
struct mptcp_subflow_context *subflow);
void __mptcp_subflow_send_ack(struct sock *ssk);
void mptcp_subflow_reset(struct sock *ssk);
void mptcp_subflow_queue_clean(struct sock *sk, struct sock *ssk);
void mptcp_sock_graft(struct sock *sk, struct socket *parent);
struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
bool __mptcp_close(struct sock *sk, long timeout);
Expand Down
72 changes: 0 additions & 72 deletions net/mptcp/subflow.c
Original file line number Diff line number Diff line change
Expand Up @@ -1801,78 +1801,6 @@ static void subflow_state_change(struct sock *sk)
}
}

void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_ssk)
{
struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
struct mptcp_sock *msk, *next, *head = NULL;
struct request_sock *req;

/* build a list of all unaccepted mptcp sockets */
spin_lock_bh(&queue->rskq_lock);
for (req = queue->rskq_accept_head; req; req = req->dl_next) {
struct mptcp_subflow_context *subflow;
struct sock *ssk = req->sk;
struct mptcp_sock *msk;

if (!sk_is_mptcp(ssk))
continue;

subflow = mptcp_subflow_ctx(ssk);
if (!subflow || !subflow->conn)
continue;

/* skip if already in list */
msk = mptcp_sk(subflow->conn);
if (msk->dl_next || msk == head)
continue;

msk->dl_next = head;
head = msk;
}
spin_unlock_bh(&queue->rskq_lock);
if (!head)
return;

/* can't acquire the msk socket lock under the subflow one,
* or will cause ABBA deadlock
*/
release_sock(listener_ssk);

for (msk = head; msk; msk = next) {
struct sock *sk = (struct sock *)msk;
bool do_cancel_work;

lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
next = msk->dl_next;
msk->first = NULL;
msk->dl_next = NULL;

do_cancel_work = __mptcp_close(sk, 0);
release_sock(sk);
if (do_cancel_work) {
/* lockdep will report a false positive ABBA deadlock
* between cancel_work_sync and the listener socket.
* The involved locks belong to different sockets WRT
* the existing AB chain.
* Using a per socket key is problematic as key
* deregistration requires process context and must be
* performed at socket disposal time, in atomic
* context.
* Just tell lockdep to consider the listener socket
* released here.
*/
mutex_release(&listener_sk->sk_lock.dep_map, _RET_IP_);
mptcp_cancel_work(sk);
mutex_acquire(&listener_sk->sk_lock.dep_map,
SINGLE_DEPTH_NESTING, 0, _RET_IP_);
}
sock_put(sk);
}

/* we are still under the listener msk socket lock */
lock_sock_nested(listener_ssk, SINGLE_DEPTH_NESTING);
}

static int subflow_ulp_init(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
Expand Down

0 comments on commit 2a20739

Please sign in to comment.