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(-)
On Sat, 18 Nov 2023 at 05:58, Helge Deller <[email protected]> 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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
Co-developed-by: Linus Torvalds <[email protected]>
Reported-by: Sam James <[email protected]>
Closes: https://github.com/systemd/systemd/issues/29775
Tested-by: Sam James <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Cc: <[email protected]> # 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 <[email protected]> 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 <[email protected]> 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