Message ID | ZVjC9P0h5mw3ZbnD@p100 |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6359:6513:b0:164:83eb:24d7 with SMTP id sk19csp1237791rwb; Sat, 18 Nov 2023 05:58:48 -0800 (PST) X-Google-Smtp-Source: AGHT+IF+tc8ldRjXGgDIdZlZpDu0a2WcTwahPYW6TXdfzUWzRs01b0L/e/CWYQkl17opse8YEQSS X-Received: by 2002:a17:90b:4a0a:b0:27d:1972:136c with SMTP id kk10-20020a17090b4a0a00b0027d1972136cmr2733653pjb.38.1700315927825; Sat, 18 Nov 2023 05:58:47 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700315927; cv=none; d=google.com; s=arc-20160816; b=AB6+uZdPuJPc02Q9QVFZsMOk8F2k2f+fU2UlgzPe/d+ESgQXmKYdT3bsoMfc8bOjHR U2GgifTTx2DN6VNPZieRerF6iuLRduxs+CvboGrUA7iOvjQcTqpmDn/WGJ6UyZ44m2O7 zmm0aXIYRCKjEP4RXSAgFkJzhcfEhPX3y+IdhkqEDDYnbFnuy3NmhoUnGq3FJl2VCSAD ZzKTcobpeXA9QUn4bgNM5tqmVoHyML79RMN/LXQIWK6wgN31mmXbyuNMGc+jQOq7tgNi p+55sc//aNcOpg67JOrB3shY/ZihYhALgTpnnNnAjps4dDQERRsjwn1PMErj3Dsu5Ups /yyA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date; bh=p5dKBKl/Vh3O8ZVJhEGTtCRhPoH74YRxlt0Hw6Q3wVI=; fh=MilSIBE2HvLQbJ0LLlTd3CoPI/3h8MnjL/gFfLf6OWI=; b=xsBeRaOgTp3TFoWXr3pFTMfwRArBcSwh2binpLvHVUKP+OQ6R2LOWXwvVguAfwuDYX X88ud2xJ+SlW7rBOsw5BFmumtEUq+AKI2vPo1jFROtYqeq3qtr246xqm7Qsnfrb+WO29 j6Bph2/fi/Lhibfw8pOt5aTjmYFZeMfy0Spgy/dlqHfFqFjX+RGiFEiNjtymyztBq04g fUtjVXPgrUdCV8eV62djdeVz/qK0oQeN+lL3C2houhu295l2/gxSzmLnX1yP5tLulU8x Za8pmSObhLmaO1JQ/gI1RruTJid0WbHQ7P12ADqJr56alHC3hvWAYaihsnJ92hhNz3gM mIZw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmx.de Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id mm1-20020a17090b358100b0028001292940si6561099pjb.68.2023.11.18.05.58.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 18 Nov 2023 05:58:47 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmx.de Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 6DE718087255; Sat, 18 Nov 2023 05:58:45 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229848AbjKRN6X (ORCPT <rfc822;jaysivo@gmail.com> + 29 others); Sat, 18 Nov 2023 08:58:23 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37634 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229536AbjKRN6W (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 18 Nov 2023 08:58:22 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9673F1AD; Sat, 18 Nov 2023 05:58:17 -0800 (PST) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 443A1C433C7; Sat, 18 Nov 2023 13:58:15 +0000 (UTC) Date: Sat, 18 Nov 2023 14:58:12 +0100 From: Helge Deller <deller@gmx.de> To: Linus Torvalds <torvalds@linux-foundation.org>, linux-kernel@vger.kernel.org, linux-parisc@vger.kernel.org, James Bottomley <James.Bottomley@hansenpartnership.com>, John David Anglin <dave.anglin@bell.net> Cc: Kees Cook <keescook@chromium.org> Subject: [GIT PULL] parisc architecture fixes for v6.7-rc1 Message-ID: <ZVjC9P0h5mw3ZbnD@p100> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-0.5 required=5.0 tests=FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sat, 18 Nov 2023 05:58:45 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782910474261209338 X-GMAIL-MSGID: 1782910474261209338 |
Series |
[GIT,PULL] parisc architecture fixes for v6.7-rc1
|
|
Pull-request
git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git tags/parisc-for-6.7-rc2Message
Helge Deller
Nov. 18, 2023, 1:58 p.m. UTC
Hi Linus, please pull three small additional parisc fixes for kernel 6.7-rc2, two of those are tagged for stable series. On parisc we still sometimes need writeable stacks, e.g. if programs aren't compiled with gcc-14. To avoid issues with the upcoming systemd-254 we therefore have to disable prctl(PR_SET_MDWE) for now (for parisc only). The other two patches are minor: a bugfix for the soft power-off on qemu with 64-bit kernel and a patch from Kees to prefer strscpy() over strlcpy(). Thanks! Helge ---------------------------------------------------------------- The following changes since commit b85ea95d086471afb4ad062012a4d73cd328fa86: Linux 6.7-rc1 (2023-11-12 16:19:07 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git tags/parisc-for-6.7-rc2 for you to fetch changes up to b8eaae484f79b37c602d112e131475013ab14519: parisc/power: Fix power soft-off when running on qemu (2023-11-17 16:54:27 +0100) ---------------------------------------------------------------- parisc architecture fixes for kernel v6.7-rc2: - Fix power soft-off on qemu - Disable prctl(PR_SET_MDWE) since parisc sometimes still needs writeable stacks - Use strscpy instead of strlcpy in show_cpuinfo() ---------------------------------------------------------------- Helge Deller (2): prctl: Temporarily disable prctl(PR_SET_MDWE) on parisc parisc/power: Fix power soft-off when running on qemu Kees Cook (1): parisc: Replace strlcpy() with strscpy() arch/parisc/kernel/processor.c | 2 +- drivers/parisc/power.c | 2 +- kernel/sys.c | 10 ++++++++-- 3 files changed, 10 insertions(+), 4 deletions(-)
Comments
On Sat, 18 Nov 2023 at 05:58, Helge Deller <deller@gmx.de> wrote: > > On parisc we still sometimes need writeable stacks, e.g. if programs aren't > compiled with gcc-14. To avoid issues with the upcoming systemd-254 we > therefore have to disable prctl(PR_SET_MDWE) for now (for parisc only). Ugh. I pulled this, but I *really* cannot live with how ugly that is. Seriously, that code is just unacceptable. Doing something like + if (IS_ENABLED(CONFIG_PARISC)) + error = -EINVAL; + else + error = prctl_set_mdwe(arg2, arg3, arg4, arg5); in generic code with no comment is just truly crazy. If you have to go and do a "git blame -C" just to understand why the code exists, the code is a problem. But it goes beyond that. The code is just *ugly*, and it's done entirely in the wrong place. Things like "mdwe is special on parisc" should *NOT* be done in the generic "prctl()" function. This issue is not specific to prctl() - it's very much specific to mdwe. So I think it would have been both much more legible, and *much* more appropriate, to do it in prctl_set_mdwe() itself, where it makes more sense, and where it matches all the *other* mdwe-specific checks the code does wrt arguments and existing state. And honestly, why wouldn't 'get_mdwe' work? So the *other* hunk in that patch (which isn't even mentioned in the commit message) that returns -EINVAL for get_mdwe makes no sense at all, and shouldn't have existed. End result: I think the code should have been something like this (whitespace-damaged) thing: --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2394,6 +2394,10 @@ static inline int prctl_set_mdwe(unsigned long bits, if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN)) return -EINVAL; + /* PARISC cannot allow mdwe as it needs writable stacks */ + if (IS_ENABLED(CONFIG_PARISC)) + return -ENOSYS; + current_bits = get_current_mdwe(); if (current_bits && current_bits != bits) return -EPERM; /* Cannot unset the flags */ where I also picked another error code, because it's not that the prctl value or the arguments are invalid, I think the error should show that there's something else going on. No, I don't think -ENOSYS is necessarily the best possible error value, but I think it at least conceptually matches the "this prctl doesn't exist on PARISC". Maybe Maybe ENOSYS should be avoided (prctl() obvious does exist), but I do think this should be a different error than the EINVAL that the generic checks do. End result: I really hated this change so much that I ended up unpulling after doing the pull. This needs to be done right, or not at all. Linus
On 11/18/23 18:36, Linus Torvalds wrote: > On Sat, 18 Nov 2023 at 05:58, Helge Deller <deller@gmx.de> wrote: >> >> On parisc we still sometimes need writeable stacks, e.g. if programs aren't >> compiled with gcc-14. To avoid issues with the upcoming systemd-254 we >> therefore have to disable prctl(PR_SET_MDWE) for now (for parisc only). > > Ugh. > > I pulled this, but I *really* cannot live with how ugly that is. > > Seriously, that code is just unacceptable. Doing something like > > + if (IS_ENABLED(CONFIG_PARISC)) > + error = -EINVAL; > + else > + error = prctl_set_mdwe(arg2, arg3, arg4, arg5); > > in generic code with no comment is just truly crazy. If you have to go > and do a "git blame -C" just to understand why the code exists, the > code is a problem. > > But it goes beyond that. The code is just *ugly*, and it's done > entirely in the wrong place. > > Things like "mdwe is special on parisc" should *NOT* be done in the > generic "prctl()" function. This issue is not specific to prctl() - > it's very much specific to mdwe. > > So I think it would have been both much more legible, and *much* more > appropriate, to do it in prctl_set_mdwe() itself, where it makes more > sense, and where it matches all the *other* mdwe-specific checks the > code does wrt arguments and existing state. > > And honestly, why wouldn't 'get_mdwe' work? So the *other* hunk in > that patch (which isn't even mentioned in the commit message) that > returns -EINVAL for get_mdwe makes no sense at all, and shouldn't have > existed. > > End result: I think the code should have been something like this > (whitespace-damaged) thing: > > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2394,6 +2394,10 @@ static inline int prctl_set_mdwe(unsigned > long bits, > if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN)) > return -EINVAL; > > + /* PARISC cannot allow mdwe as it needs writable stacks */ > + if (IS_ENABLED(CONFIG_PARISC)) > + return -ENOSYS; > + > current_bits = get_current_mdwe(); > if (current_bits && current_bits != bits) > return -EPERM; /* Cannot unset the flags */ Ok. My initial patch was actually doing exatly that, but somehow I finally decided to add it to the switch() instead. Seems this was the wrong decision :-( > where I also picked another error code, because it's not that the > prctl value or the arguments are invalid, I think the error should > show that there's something else going on. > > No, I don't think -ENOSYS is necessarily the best possible error > value, but I think it at least conceptually matches the "this prctl > doesn't exist on PARISC". Maybe > > Maybe ENOSYS should be avoided (prctl() obvious does exist), but I do > think this should be a different error than the EINVAL that the > generic checks do. I agree that returning something else than EINVAL would be better. I used ENODEV in an earlier patch (I didn't liked it either), but according to https://github.com/systemd/systemd/issues/29775#issuecomment-1809563365 EINVAL seems the best solution currently. Just as a side-note: ENOSYS gives a checkpatch warning: WARNING: ENOSYS means 'invalid syscall nr' and nothing else Would the patch below be OK? It's basically yours but with EINVAL. (might be whitespace-scrambled!) Helge --- From: Helge Deller <deller@gmx.de> Subject: [PATCH] prctl: Disable prctl(PR_SET_MDWE) on parisc systemd-254 tries to use prctl(PR_SET_MDWE) for it's MemoryDenyWriteExecute functionality, but fails on parisc which still needs executable stacks in certain combinations of gcc/glibc/kernel. Disable prctl(PR_SET_MDWE) by returning -EINVAL for now on parisc, until userspace has catched up. Signed-off-by: Helge Deller <deller@gmx.de> Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org> Reported-by: Sam James <sam@gentoo.org> Closes: https://github.com/systemd/systemd/issues/29775 Tested-by: Sam James <sam@gentoo.org> Link: https://lore.kernel.org/all/875y2jro9a.fsf@gentoo.org/ Cc: <stable@vger.kernel.org> # v6.3+ diff --git a/kernel/sys.c b/kernel/sys.c index 420d9cb9cc8e..e219fcfa112d 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2394,6 +2394,10 @@ static inline int prctl_set_mdwe(unsigned long bits, unsigned long arg3, if (bits & PR_MDWE_NO_INHERIT && !(bits & PR_MDWE_REFUSE_EXEC_GAIN)) return -EINVAL; + /* PARISC cannot allow mdwe as it needs writable stacks */ + if (IS_ENABLED(CONFIG_PARISC)) + return -EINVAL; + current_bits = get_current_mdwe(); if (current_bits && current_bits != bits) return -EPERM; /* Cannot unset the flags */
On Sat, 18 Nov 2023 at 10:40, Helge Deller <deller@gmx.de> wrote: > > Would the patch below be OK? It's basically yours but with EINVAL. > (might be whitespace-scrambled!) I don't particularly like EINVAL, but it's not the kind of show-stopper that the other issues were. Linus
On 11/18/23 19:42, Linus Torvalds wrote: > On Sat, 18 Nov 2023 at 10:40, Helge Deller <deller@gmx.de> wrote: >> >> Would the patch below be OK? It's basically yours but with EINVAL. >> (might be whitespace-scrambled!) > > I don't particularly like EINVAL, but it's not the kind of > show-stopper that the other issues were. Ok, thank you! I'll send you a new pull request. Btw, we are in the process to get rid of executable stacks, but this will take time. That said, I hope to remove this check then again. Helge