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

[Dave Martin <Dave.Martin@arm.com>]

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