linux-kernel - Re: [GIT PULL] parisc architecture fixes for v6.7-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0f492aac-9ba7-4bc6-95b2-6b1104a12244@gmx.de>
Date:   Sat, 18 Nov 2023 19:40:07 +0100
From:   Helge Deller <deller@....de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-parisc@...r.kernel.org,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        John David Anglin <dave.anglin@...l.net>,
        Kees Cook <keescook@...omium.org>, Sam James <sam@...too.org>
Subject: Re: [GIT PULL] parisc architecture fixes for v6.7-rc1

On 11/18/23 18:36, Linus Torvalds wrote:
> On Sat, 18 Nov 2023 at 05:58, Helge Deller <deller@....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@....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@....de>
Co-developed-by: Linus Torvalds <torvalds@...ux-foundation.org>
Reported-by: Sam James <sam@...too.org>
Closes: https://github.com/systemd/systemd/issues/29775
Tested-by: Sam James <sam@...too.org>
Link: https://lore.kernel.org/all/875y2jro9a.fsf@gentoo.org/
Cc: <stable@...r.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 */