* [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks
@ 2020-10-14 5:24 Peter Collingbourne
2020-10-14 9:53 ` Dave Martin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Peter Collingbourne @ 2020-10-14 5:24 UTC (permalink / raw)
To: Kristina Martsenko, Vincenzo Frascino, Catalin Marinas,
Will Deacon, Dave Martin
Cc: Peter Collingbourne, Linux ARM
It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a
compat task because the 32-bit instruction set does not offer PAuth
instructions. For consistency with other 64-bit only prctls such as
{SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks.
Although this is a userspace-visible change, maybe it isn't too late
to make this change given that the hardware isn't available yet and
it's very unlikely that anyone has 32-bit software that actually
depends on this succeeding.
Link: https://linux-review.googlesource.com/id/Ie885a1ff84ab498cc9f62d6451e9f2cfd4b1d06a
Signed-off-by: Peter Collingbourne <pcc@google.com>
---
arch/arm64/kernel/pointer_auth.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c
index 1e77736a4f66..245c3ee97ed8 100644
--- a/arch/arm64/kernel/pointer_auth.c
+++ b/arch/arm64/kernel/pointer_auth.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/compat.h>
#include <linux/errno.h>
#include <linux/prctl.h>
#include <linux/random.h>
@@ -17,6 +18,9 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg)
if (!system_supports_address_auth() && !system_supports_generic_auth())
return -EINVAL;
+ if (is_compat_task())
+ return -EINVAL;
+
if (!arg) {
ptrauth_keys_init_user(keys);
return 0;
--
2.28.0.1011.ga647a8990f-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks 2020-10-14 5:24 [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks Peter Collingbourne @ 2020-10-14 9:53 ` Dave Martin 2020-10-14 17:45 ` Peter Collingbourne 2020-10-15 10:46 ` Will Deacon 2020-10-15 20:40 ` Will Deacon 2 siblings, 1 reply; 6+ messages in thread From: Dave Martin @ 2020-10-14 9:53 UTC (permalink / raw) To: Peter Collingbourne Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Linux ARM, Kristina Martsenko On Wed, Oct 14, 2020 at 06:24:30AM +0100, Peter Collingbourne wrote: > It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a > compat task because the 32-bit instruction set does not offer PAuth > instructions. For consistency with other 64-bit only prctls such as > {SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks. > > Although this is a userspace-visible change, maybe it isn't too late > to make this change given that the hardware isn't available yet and > it's very unlikely that anyone has 32-bit software that actually > depends on this succeeding. > > Link: https://linux-review.googlesource.com/id/Ie885a1ff84ab498cc9f62d6451e9f2cfd4b1d06a > Signed-off-by: Peter Collingbourne <pcc@google.com> This does seem an anomaly, but it's not an isolated case. I suspect that some other prctls are also missing a compat check -- PR_SVE_SET_VL doesn't have it, for example. So, I'm not sure it's worth fixing this one case in isolation. Fixing all affected cases may have greater risk, and it won't stay fixed, since the compat check will likely often get forgotten when a new prctl is added. So, is this anomaly in any way harmful? Can the code be refactored in such a way as to make it hard to forget the check in future? Cheers ---Dave > --- > arch/arm64/kernel/pointer_auth.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c > index 1e77736a4f66..245c3ee97ed8 100644 > --- a/arch/arm64/kernel/pointer_auth.c > +++ b/arch/arm64/kernel/pointer_auth.c > @@ -1,5 +1,6 @@ > // SPDX-License-Identifier: GPL-2.0 > > +#include <linux/compat.h> > #include <linux/errno.h> > #include <linux/prctl.h> > #include <linux/random.h> > @@ -17,6 +18,9 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) > if (!system_supports_address_auth() && !system_supports_generic_auth()) > return -EINVAL; > > + if (is_compat_task()) > + return -EINVAL; > + > if (!arg) { > ptrauth_keys_init_user(keys); > return 0; > -- > 2.28.0.1011.ga647a8990f-goog > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks 2020-10-14 9:53 ` Dave Martin @ 2020-10-14 17:45 ` Peter Collingbourne 2020-10-14 18:00 ` Dave Martin 0 siblings, 1 reply; 6+ messages in thread From: Peter Collingbourne @ 2020-10-14 17:45 UTC (permalink / raw) To: Dave Martin Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Linux ARM, Kristina Martsenko On Wed, Oct 14, 2020 at 2:54 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Wed, Oct 14, 2020 at 06:24:30AM +0100, Peter Collingbourne wrote: > > It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a > > compat task because the 32-bit instruction set does not offer PAuth > > instructions. For consistency with other 64-bit only prctls such as > > {SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks. > > > > Although this is a userspace-visible change, maybe it isn't too late > > to make this change given that the hardware isn't available yet and > > it's very unlikely that anyone has 32-bit software that actually > > depends on this succeeding. > > > > Link: https://linux-review.googlesource.com/id/Ie885a1ff84ab498cc9f62d6451e9f2cfd4b1d06a > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > This does seem an anomaly, but it's not an isolated case. I suspect > that some other prctls are also missing a compat check -- PR_SVE_SET_VL > doesn't have it, for example. > > So, I'm not sure it's worth fixing this one case in isolation. Fixing > all affected cases may have greater risk, and it won't stay fixed, since > the compat check will likely often get forgotten when a new prctl is > added. The only other affected cases involve SVE and that doesn't have hardware available yet either, right? I'm going by the binutils CPU list, which is the closest thing that I'm aware of to an official list of all microarchitectures and their supported ISA features: https://github.com/bminor/binutils-gdb/blob/6248f5e4fc4ad1e433156520e44ac3217c39a621/gas/config/tc-aarch64.c#L8888 (and I know that Neoverse V1/N2 isn't available yet) > So, is this anomaly in any way harmful? Not as far as I can tell, at least for this specific prctl. > Can the code be refactored in such a way as to make it hard to forget > the check in future? I've never been a fan of the arch-specific prctls being listed in kernel/sys.c. It seems better to me to have that handling be moved into a new arch hook and that should let us remove some boilerplate as well. We can make the default case in the prctl syscall handler look like this: default: return arch_handle_prctrl(option, arg2, arg3, arg4, arg5); And move the arch-specific prctls into a switch in arch_handle_prctl(). Now, since (as far as I can tell) all of the arm64-specific prctls do not make sense on compat tasks, we can add an: if (is_compat_task()) return -EINVAL; to the top of arch_handle_prctl() and any new arm64-specific prctls will get the compat check by default. Of course, if we add a new compat-compatible prctl in the future, we may add it to a new switch before the if statement. Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks 2020-10-14 17:45 ` Peter Collingbourne @ 2020-10-14 18:00 ` Dave Martin 0 siblings, 0 replies; 6+ messages in thread From: Dave Martin @ 2020-10-14 18:00 UTC (permalink / raw) To: Peter Collingbourne Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Kristina Martsenko, Linux ARM On Wed, Oct 14, 2020 at 10:45:48AM -0700, Peter Collingbourne wrote: > On Wed, Oct 14, 2020 at 2:54 AM Dave Martin <Dave.Martin@arm.com> wrote: > > > > On Wed, Oct 14, 2020 at 06:24:30AM +0100, Peter Collingbourne wrote: > > > It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a > > > compat task because the 32-bit instruction set does not offer PAuth > > > instructions. For consistency with other 64-bit only prctls such as > > > {SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks. > > > > > > Although this is a userspace-visible change, maybe it isn't too late > > > to make this change given that the hardware isn't available yet and > > > it's very unlikely that anyone has 32-bit software that actually > > > depends on this succeeding. > > > > > > Link: https://linux-review.googlesource.com/id/Ie885a1ff84ab498cc9f62d6451e9f2cfd4b1d06a > > > Signed-off-by: Peter Collingbourne <pcc@google.com> > > > > This does seem an anomaly, but it's not an isolated case. I suspect > > that some other prctls are also missing a compat check -- PR_SVE_SET_VL > > doesn't have it, for example. > > > > So, I'm not sure it's worth fixing this one case in isolation. Fixing > > all affected cases may have greater risk, and it won't stay fixed, since > > the compat check will likely often get forgotten when a new prctl is > > added. > > The only other affected cases involve SVE and that doesn't have > hardware available yet either, right? I'm going by the binutils CPU > list, which is the closest thing that I'm aware of to an official list > of all microarchitectures and their supported ISA features: > > https://github.com/bminor/binutils-gdb/blob/6248f5e4fc4ad1e433156520e44ac3217c39a621/gas/config/tc-aarch64.c#L8888 > > (and I know that Neoverse V1/N2 isn't available yet) Yes, that's probably a reasonable list. SVE is supported by Fujitsu's A64FX part, but that's in few pockets. In any case, it doesn't feel legitimate for a compat process to be using this. The chance of extra compat checks there breaking something seems pretty low. It's not like we ever promised it would work on compat. > > So, is this anomaly in any way harmful? > > Not as far as I can tell, at least for this specific prctl. Ack > > Can the code be refactored in such a way as to make it hard to forget > > the check in future? > > I've never been a fan of the arch-specific prctls being listed in > kernel/sys.c. It seems better to me to have that handling be moved > into a new arch hook and that should let us remove some boilerplate as > well. We can make the default case in the prctl syscall handler look > like this: > > default: > return arch_handle_prctrl(option, arg2, arg3, arg4, arg5); > > And move the arch-specific prctls into a switch in > arch_handle_prctl(). Now, since (as far as I can tell) all of the > arm64-specific prctls do not make sense on compat tasks, we can add > an: > > if (is_compat_task()) > return -EINVAL; > > to the top of arch_handle_prctl() and any new arm64-specific prctls > will get the compat check by default. Of course, if we add a new > compat-compatible prctl in the future, we may add it to a new switch > before the if statement. > > Peter Something like that could work. I did have a try at cleaning some of this up some time ago [1], but other stuff happened and I never finished it -- not sure how relevant it still is, but might be worth a look. Cheers ---Dave [1] [RFC PATCH 00/11] prctl: Modernise wiring for optional prctl() calls https://lore.kernel.org/lkml/1526318067-4964-1-git-send-email-Dave.Martin@arm.com/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks 2020-10-14 5:24 [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks Peter Collingbourne 2020-10-14 9:53 ` Dave Martin @ 2020-10-15 10:46 ` Will Deacon 2020-10-15 20:40 ` Will Deacon 2 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2020-10-15 10:46 UTC (permalink / raw) To: Peter Collingbourne Cc: Linux ARM, Catalin Marinas, Vincenzo Frascino, Dave Martin, Kristina Martsenko On Tue, Oct 13, 2020 at 10:24:30PM -0700, Peter Collingbourne wrote: > It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a > compat task because the 32-bit instruction set does not offer PAuth > instructions. For consistency with other 64-bit only prctls such as > {SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks. > > Although this is a userspace-visible change, maybe it isn't too late > to make this change given that the hardware isn't available yet and > it's very unlikely that anyone has 32-bit software that actually > depends on this succeeding. > > Link: https://linux-review.googlesource.com/id/Ie885a1ff84ab498cc9f62d6451e9f2cfd4b1d06a > Signed-off-by: Peter Collingbourne <pcc@google.com> > --- > arch/arm64/kernel/pointer_auth.c | 4 ++++ > 1 file changed, 4 insertions(+) Cheers. Although I agree with the discussion here that it would be better to catch this in one place, for now I'll merge this and add similar checks to the SVE prctl()s too. Patch below (I also tweaked it to use the 'tsk' parameter for ptrauth, for consistency). Will --->8 diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index a6d688c10745..062b21f30f94 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -678,7 +678,7 @@ int sve_set_current_vl(unsigned long arg) vl = arg & PR_SVE_VL_LEN_MASK; flags = arg & ~vl; - if (!system_supports_sve()) + if (!system_supports_sve() || is_compat_task()) return -EINVAL; ret = sve_set_vector_length(current, vl, flags); @@ -691,7 +691,7 @@ int sve_set_current_vl(unsigned long arg) /* PR_SVE_GET_VL */ int sve_get_current_vl(void) { - if (!system_supports_sve()) + if (!system_supports_sve() || is_compat_task()) return -EINVAL; return sve_prctl_status(0); diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c index 1e77736a4f66..adb955fd9bdd 100644 --- a/arch/arm64/kernel/pointer_auth.c +++ b/arch/arm64/kernel/pointer_auth.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 +#include <linux/compat.h> #include <linux/errno.h> #include <linux/prctl.h> #include <linux/random.h> @@ -17,6 +18,9 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) if (!system_supports_address_auth() && !system_supports_generic_auth()) return -EINVAL; + if (is_compat_thread(task_thread_info(tsk))) + return -EINVAL; + if (!arg) { ptrauth_keys_init_user(keys); return 0; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks 2020-10-14 5:24 [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks Peter Collingbourne 2020-10-14 9:53 ` Dave Martin 2020-10-15 10:46 ` Will Deacon @ 2020-10-15 20:40 ` Will Deacon 2 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2020-10-15 20:40 UTC (permalink / raw) To: Kristina Martsenko, Catalin Marinas, Vincenzo Frascino, Dave Martin, Peter Collingbourne Cc: Will Deacon, kernel-team, Linux ARM On Tue, 13 Oct 2020 22:24:30 -0700, Peter Collingbourne wrote: > It doesn't make sense to issue prctl(PR_PAC_RESET_KEYS) on a > compat task because the 32-bit instruction set does not offer PAuth > instructions. For consistency with other 64-bit only prctls such as > {SET,GET}_TAGGED_ADDR_CTRL, reject the prctl on compat tasks. > > Although this is a userspace-visible change, maybe it isn't too late > to make this change given that the hardware isn't available yet and > it's very unlikely that anyone has 32-bit software that actually > depends on this succeeding. Applied to arm64 (for-next/core), thanks! [1/1] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks https://git.kernel.org/arm64/c/de7ae1c57273 Please check my changes if you get a chance. Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-15 20:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-14 5:24 [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks Peter Collingbourne 2020-10-14 9:53 ` Dave Martin 2020-10-14 17:45 ` Peter Collingbourne 2020-10-14 18:00 ` Dave Martin 2020-10-15 10:46 ` Will Deacon 2020-10-15 20:40 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).