From 8184bde53773e5419a676cb57a46aa8025ee5876 Mon Sep 17 00:00:00 2001 From: Andreas Eversberg Date: Thu, 15 Feb 2024 10:16:33 +0100 Subject: [PATCH] osmo_io_uring: Cancel pending request, free msghdr on completion There is always a completion after cancelling a uring request. Because uring requests use msghdr pointer as user data, we cannot just free the msghdr after cancelling. Upon completion (received after cancelling), the user data still points to the msghdr. To prevent a use-after-free bug, msghdr is not freed, but detached from iofd instance. Then upon completion, the msghdr (if it was detached from iofd) is freed. Additionally it is not required to keep IOFD_FLAG_IN_CALLBACK set anymore, if there is a msghdr attached to iofd. As described above, all msghdr get detached, if iofd is freed (uring request get cancelled) during callback. Related: OS#5751 Change-Id: Ic253f085dd6362db85f029f46350951472210a02 --- src/core/osmo_io_uring.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/core/osmo_io_uring.c b/src/core/osmo_io_uring.c index c315c18bc..26586808a 100644 --- a/src/core/osmo_io_uring.c +++ b/src/core/osmo_io_uring.c @@ -230,8 +230,7 @@ static void iofd_uring_handle_completion(struct iofd_msghdr *msghdr, int res) OSMO_ASSERT(0) } - if (!iofd->u.uring.read_msghdr && !iofd->u.uring.write_msghdr) - IOFD_FLAG_UNSET(iofd, IOFD_FLAG_IN_CALLBACK); + IOFD_FLAG_UNSET(iofd, IOFD_FLAG_IN_CALLBACK); if (IOFD_FLAG_ISSET(iofd, IOFD_FLAG_TO_FREE) && !iofd->u.uring.read_msghdr && !iofd->u.uring.write_msghdr) talloc_free(iofd); @@ -252,6 +251,11 @@ static void iofd_uring_cqe(struct io_uring *ring) io_uring_cqe_seen(ring, cqe); continue; } + if (!msghdr->iofd) { + io_uring_cqe_seen(ring, cqe); + iofd_msghdr_free(msghdr); + continue; + } rc = cqe->res; /* Hand the entry back to the kernel before */ @@ -307,20 +311,31 @@ static int iofd_uring_register(struct osmo_io_fd *iofd) static int iofd_uring_unregister(struct osmo_io_fd *iofd) { struct io_uring_sqe *sqe; + struct iofd_msghdr *msghdr; + if (iofd->u.uring.read_msghdr) { + msghdr = iofd->u.uring.read_msghdr; sqe = io_uring_get_sqe(&g_ring.ring); OSMO_ASSERT(sqe != NULL); io_uring_sqe_set_data(sqe, NULL); LOGPIO(iofd, LOGL_DEBUG, "Cancelling read\n"); - io_uring_prep_cancel(sqe, iofd->u.uring.read_msghdr, 0); + iofd->u.uring.read_msghdr = NULL; + talloc_steal(OTC_GLOBAL, msghdr); + msghdr->iofd = NULL; + io_uring_prep_cancel(sqe, msghdr, 0); } if (iofd->u.uring.write_msghdr) { + msghdr = iofd->u.uring.write_msghdr; sqe = io_uring_get_sqe(&g_ring.ring); OSMO_ASSERT(sqe != NULL); io_uring_sqe_set_data(sqe, NULL); LOGPIO(iofd, LOGL_DEBUG, "Cancelling write\n"); - io_uring_prep_cancel(sqe, iofd->u.uring.write_msghdr, 0); + iofd->u.uring.write_msghdr = NULL; + talloc_steal(OTC_GLOBAL, msghdr); + msgb_free(msghdr->msg); + msghdr->iofd = NULL; + io_uring_prep_cancel(sqe, msghdr, 0); } io_uring_submit(&g_ring.ring);