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]

Date:   Sat, 18 Nov 2023 09:36:14 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Helge Deller <deller@....de>
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>
Subject: Re: [GIT PULL] parisc architecture fixes for v6.7-rc1

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 */

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