From patchwork Sun Mar 19 15:10:08 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergey Bugaev X-Patchwork-Id: 66593 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B886E383E6B6 for ; Sun, 19 Mar 2023 15:13:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B886E383E6B6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1679238797; bh=UMwGX8IleCAQ6kxyanGTidmEZWOfs4n+goCewc703hY=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Ygya5LNIbx19pdcZnPd+1aA4bkQ3W2Abumegq7Ew2SH0Bh3sw/0r9G83UgalOsH2A gCUxmYj4vjPfkS8WagHtTpt06UTbLFjrn28+5fx1fthboewdQF4vVVBl09Lst3P4/D EV9QG5kD69p1lgMSdxxiLFhydt1WS/aLb8vKQB+4= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by sourceware.org (Postfix) with ESMTPS id EA0483858D1E for ; Sun, 19 Mar 2023 15:11:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EA0483858D1E Received: by mail-lf1-x12f.google.com with SMTP id y20so12036909lfj.2 for ; Sun, 19 Mar 2023 08:11:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679238680; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=UMwGX8IleCAQ6kxyanGTidmEZWOfs4n+goCewc703hY=; b=F1N+AxJjdEfYEjsufVxe4H2rt3Jr54LY3H1CCgAMRMdoYe3WzPzcyBrY82gHdlMGtQ TNOgN54q1Obu0aQn4beEI76y/ii3DaHHHLhbIiGBJalVMNMfuRQFI9m1z6qarNPUzUMB n97jUcf8zMGZxkqyq1jouVnYPaeW0Gl4Ww2rdDvqUUOzWhA0dj9YHmDvyeUbD8riE9XJ l01fKu9g5DLH4nyoR/f3Y0A7FBnr0v8qNRv9ZtFwm9m39USsmx1pmHSy26/a0RBFs2ig kyVgAcMxGVNeqOGeLI0O5Cx6jLUpnV87JvyOIrCUrwMPmiksmXZYj6Lk0G6R9l7ESLDz R2PA== X-Gm-Message-State: AO0yUKUabOTgD5lvdLkbcmadfIxXvebCM6UHbz17sxLpseR6SsKNWrFs GK6ZE0KVnXxzR52hTWNLVVqdUSGNmbRPcw== X-Google-Smtp-Source: AK7set8PLKHHOMxHGNuIWqmK7IEuI/IhZMOnValv6N4uqTiObVK5IHrQp2hxAYWM51Pwb9Yn6/Alwg== X-Received: by 2002:ac2:410b:0:b0:4e9:638d:8438 with SMTP id b11-20020ac2410b000000b004e9638d8438mr2666461lfi.6.1679238680234; Sun, 19 Mar 2023 08:11:20 -0700 (PDT) Received: from surface-pro-6.. ([2a00:1370:818c:4a57:577a:76f4:df43:5e66]) by smtp.gmail.com with ESMTPSA id m19-20020ac24253000000b004e90dee5469sm1274089lfl.157.2023.03.19.08.11.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 19 Mar 2023 08:11:19 -0700 (PDT) To: libc-alpha@sourceware.org, bug-hurd@gnu.org Cc: Samuel Thibault , Sergey Bugaev Subject: [RFC PATCH glibc 25/34] hurd: Improve reply port handling when exiting signal handlers Date: Sun, 19 Mar 2023 18:10:08 +0300 Message-Id: <20230319151017.531737-26-bugaevc@gmail.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230319151017.531737-1-bugaevc@gmail.com> References: <20230319151017.531737-1-bugaevc@gmail.com> MIME-Version: 1.0 X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Sergey Bugaev via Libc-alpha From: Sergey Bugaev Reply-To: Sergey Bugaev Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" If we're doing signals, that means we've already got the signal thread running, and that implies TLS having been set up. So we know that __hurd_local_reply_port will resolve to THREAD_SELF->reply_port, and can access that directly using the THREAD_GETMEM and THREAD_SETMEM macros. This avoids potential miscompilations, and should also be a tiny bit faster. Also, use mach_port_mod_refs () and not mach_port_destroy () to destroy the receive right. mach_port_destroy () should *never* be used on mach_task_self (); this can easily lead to port use-after-free vulnerabilities if the task has any other references to the same port. Signed-off-by: Sergey Bugaev --- NOTE: I don't really understand why sigunwind wants to destroy both its current reply port and user's reply port. Prior to commit fb304035c41c7ee2afede51e5e8568974549ba5e, it was *restoring* the user's reply port, in which case it actually made sense to destroy the current reply port. Post-fb304035c41c7ee2afede51e5e8568974549ba5e, wouldn't it be better to just keep using the current reply port, destroying the user's one? hurd/sigunwind.c | 24 +++++++++++------------- sysdeps/mach/hurd/i386/sigreturn.c | 21 +++++---------------- 2 files changed, 16 insertions(+), 29 deletions(-) diff --git a/hurd/sigunwind.c b/hurd/sigunwind.c index edd40b14..399e6900 100644 --- a/hurd/sigunwind.c +++ b/hurd/sigunwind.c @@ -18,7 +18,6 @@ #include #include -#include #include #include #include @@ -39,19 +38,18 @@ _hurdsig_longjmp_from_handler (void *data, jmp_buf env, int val) { /* Destroy the MiG reply port used by the signal handler, and restore the reply port in use by the thread when interrupted. */ - mach_port_t *reply_port = &__hurd_local_reply_port; - if (*reply_port) - { - mach_port_t port = *reply_port; - /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port - not to get another reply port, but avoids mig_dealloc_reply_port - trying to deallocate it after the receive fails (which it will, - because the reply port will be bogus, regardless). */ - *reply_port = MACH_PORT_DEAD; - __mach_port_destroy (__mach_task_self (), port); - } + mach_port_t reply_port = THREAD_GETMEM (THREAD_SELF, reply_port); + /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to + get another reply port, but avoids mig_dealloc_reply_port trying to + deallocate it after the receive fails (which it will, because the + reply port will be bogus, regardless). */ + THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD); + if (MACH_PORT_VALID (reply_port)) + __mach_port_mod_refs (__mach_task_self (), reply_port, + MACH_PORT_RIGHT_RECEIVE, -1); if (scp->sc_reply_port) - __mach_port_destroy (__mach_task_self (), scp->sc_reply_port); + __mach_port_mod_refs (__mach_task_self (), scp->sc_reply_port, + MACH_PORT_RIGHT_RECEIVE, -1); } __spin_lock (&ss->lock); diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c index db1a01f3..29c9629f 100644 --- a/sysdeps/mach/hurd/i386/sigreturn.c +++ b/sysdeps/mach/hurd/i386/sigreturn.c @@ -19,7 +19,6 @@ register int *sp asm ("%esp"); #include #include -#include #include #include #include @@ -59,7 +58,7 @@ __sigreturn (struct sigcontext *scp) { struct hurd_sigstate *ss; struct hurd_userlink *link = (void *) &scp[1]; - mach_port_t *reply_port; + mach_port_t reply_port; if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)) { @@ -101,20 +100,10 @@ __sigreturn (struct sigcontext *scp) /* Destroy the MiG reply port used by the signal handler, and restore the reply port in use by the thread when interrupted. */ - reply_port = &__hurd_local_reply_port; - if (*reply_port) - { - mach_port_t port = *reply_port; - - /* Assigning MACH_PORT_DEAD here tells libc's mig_get_reply_port not to - get another reply port, but avoids mig_dealloc_reply_port trying to - deallocate it after the receive fails (which it will, because the - reply port will be bogus, whether we do this or not). */ - *reply_port = MACH_PORT_DEAD; - - __mach_port_destroy (__mach_task_self (), port); - } - *reply_port = scp->sc_reply_port; + reply_port = THREAD_GETMEM (THREAD_SELF, reply_port); + THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port); + __mach_port_mod_refs (__mach_task_self (), reply_port, + MACH_PORT_RIGHT_RECEIVE, -1); if (scp->sc_fpused) /* Restore the FPU state. Mach conveniently stores the state