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

From: Peter Collingbourne <pcc@google.com>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Will Deacon <will@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kristina Martsenko <kristina.martsenko@arm.com>
Subject: Re: [PATCH] arm64: reject prctl(PR_PAC_RESET_KEYS) on compat tasks
Date: Wed, 14 Oct 2020 10:45:48 -0700	[thread overview]
Message-ID: <CAMn1gO73vtJzFqOENUsBGG=N2Pn3mBrTh=Y11qYxWzN_5F5_=w@mail.gmail.com> (raw)
In-Reply-To: <20201014095356.GK32292@arm.com>

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