[PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Peter Collingbourne]

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

Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Dave Martin]

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

Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Peter Collingbourne]

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

Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Dave Martin]

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

Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Will Deacon]

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

Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks

[Will Deacon]

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