From 141fe2dceeaeefaaffc2242c8652345a081e825a Mon Sep 17 00:00:00 2001 From: Mark Johnston Date: Fri, 10 Sep 2021 17:21:11 -0400 Subject: [PATCH] aio: Interlock with listen(2) soo_aio_queue() did not handle the possibility that the provided socket is a listening socket. Up until recently, to fix this one would have to acquire the socket lock first and check, since the socket buffer locks were destroyed by listen(2). Now that the socket buffer locks belong to the socket, simply check SOLISTENING(so) after acquiring them, and make listen(2) return an error if any AIO jobs are enqueued on the socket. Add a couple of simple regression test cases. Note that this fixes things only for the default AIO implementation; cxgbe(4)'s TCP offload has a separate pru_aio_queue implementation which requires its own solution. Reported by: syzbot+c8aa122fa2c6a4e2a28b@syzkaller.appspotmail.com Reported by: syzbot+39af117d43d4f0faf512@syzkaller.appspotmail.com Reported by: syzbot+60cceb9569145a0b993b@syzkaller.appspotmail.com Reported by: syzbot+2d522c5db87710277ca5@syzkaller.appspotmail.com Reviewed by: tuexen, gallatin, jhb Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D31901 --- sys/kern/sys_socket.c | 12 ++++++- sys/kern/uipc_socket.c | 7 ++++ tests/sys/aio/aio_test.c | 78 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/sys/kern/sys_socket.c b/sys/kern/sys_socket.c index e53b0367960..83dc1cb2622 100644 --- a/sys/kern/sys_socket.c +++ b/sys/kern/sys_socket.c @@ -808,18 +808,28 @@ soo_aio_queue(struct file *fp, struct kaiocb *job) if (error == 0) return (0); + /* Lock through the socket, since this may be a listening socket. */ switch (job->uaiocb.aio_lio_opcode & (LIO_WRITE | LIO_READ)) { case LIO_READ: sb = &so->so_rcv; + SOCK_RECVBUF_LOCK(so); break; case LIO_WRITE: sb = &so->so_snd; + SOCK_SENDBUF_LOCK(so); break; default: return (EINVAL); } - SOCKBUF_LOCK(sb); + if (SOLISTENING(so)) { + if (sb == &so->so_rcv) + SOCK_RECVBUF_UNLOCK(so); + else + SOCK_SENDBUF_UNLOCK(so); + return (EINVAL); + } + if (!aio_set_cancel_function(job, soo_aio_cancel)) panic("new job was cancelled"); TAILQ_INSERT_TAIL(&sb->sb_aiojobq, job, list); diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index a502b06ce00..cbddd80e054 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -928,6 +928,13 @@ solisten_proto_check(struct socket *so) } mtx_lock(&so->so_snd_mtx); mtx_lock(&so->so_rcv_mtx); + + /* Interlock with soo_aio_queue(). */ + if ((so->so_snd.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0 || + (so->so_rcv.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0) { + solisten_proto_abort(so); + return (EINVAL); + } return (0); } diff --git a/tests/sys/aio/aio_test.c b/tests/sys/aio/aio_test.c index 1c694ad0c18..a81cb906e38 100644 --- a/tests/sys/aio/aio_test.c +++ b/tests/sys/aio/aio_test.c @@ -40,11 +40,12 @@ */ #include +#include #include #include #include #include -#include +#include #include #include @@ -1177,6 +1178,79 @@ ATF_TC_BODY(aio_socket_blocking_short_write_vectored, tc) aio_socket_blocking_short_write_test(true); } +/* + * Verify that AIO requests fail when applied to a listening socket. + */ +ATF_TC_WITHOUT_HEAD(aio_socket_listen_fail); +ATF_TC_BODY(aio_socket_listen_fail, tc) +{ + struct aiocb iocb; + struct sockaddr_un sun; + char buf[16]; + int s; + + s = socket(AF_LOCAL, SOCK_STREAM, 0); + ATF_REQUIRE(s != -1); + + memset(&sun, 0, sizeof(sun)); + snprintf(sun.sun_path, sizeof(sun.sun_path), "%s", "listen.XXXXXX"); + mktemp(sun.sun_path); + sun.sun_family = AF_LOCAL; + sun.sun_len = SUN_LEN(&sun); + + ATF_REQUIRE(bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) == 0); + ATF_REQUIRE(listen(s, 5) == 0); + + memset(buf, 0, sizeof(buf)); + memset(&iocb, 0, sizeof(iocb)); + iocb.aio_fildes = s; + iocb.aio_buf = buf; + iocb.aio_nbytes = sizeof(buf); + + ATF_REQUIRE_ERRNO(EINVAL, aio_read(&iocb) == -1); + ATF_REQUIRE_ERRNO(EINVAL, aio_write(&iocb) == -1); + + ATF_REQUIRE(unlink(sun.sun_path) == 0); + close(s); +} + +/* + * Verify that listen(2) fails if a socket has pending AIO requests. + */ +ATF_TC_WITHOUT_HEAD(aio_socket_listen_pending); +ATF_TC_BODY(aio_socket_listen_pending, tc) +{ + struct aiocb iocb; + struct sockaddr_un sun; + char buf[16]; + int s; + + s = socket(AF_LOCAL, SOCK_STREAM, 0); + ATF_REQUIRE(s != -1); + + memset(&sun, 0, sizeof(sun)); + snprintf(sun.sun_path, sizeof(sun.sun_path), "%s", "listen.XXXXXX"); + mktemp(sun.sun_path); + sun.sun_family = AF_LOCAL; + sun.sun_len = SUN_LEN(&sun); + + ATF_REQUIRE(bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) == 0); + + memset(buf, 0, sizeof(buf)); + memset(&iocb, 0, sizeof(iocb)); + iocb.aio_fildes = s; + iocb.aio_buf = buf; + iocb.aio_nbytes = sizeof(buf); + ATF_REQUIRE(aio_read(&iocb) == 0); + + ATF_REQUIRE_ERRNO(EINVAL, listen(s, 5) == -1); + + ATF_REQUIRE(aio_cancel(s, &iocb) != -1); + + ATF_REQUIRE(unlink(sun.sun_path) == 0); + close(s); +} + /* * This test verifies that cancelling a partially completed socket write * returns a short write rather than ECANCELED. @@ -1808,6 +1882,8 @@ ATF_TP_ADD_TCS(tp) ATF_TP_ADD_TC(tp, aio_socket_two_reads); ATF_TP_ADD_TC(tp, aio_socket_blocking_short_write); ATF_TP_ADD_TC(tp, aio_socket_blocking_short_write_vectored); + ATF_TP_ADD_TC(tp, aio_socket_listen_fail); + ATF_TP_ADD_TC(tp, aio_socket_listen_pending); ATF_TP_ADD_TC(tp, aio_socket_short_write_cancel); ATF_TP_ADD_TC(tp, aio_writev_dos_iov_len); ATF_TP_ADD_TC(tp, aio_writev_dos_iovcnt);