Re: [RFC PATCH 00/11] prctl: Modernise wiring for optional prctl() calls

[Kees Cook]

On Mon, May 14, 2018 at 10:14 AM, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> [Reviewer note: this is a cross-arch series.  To reduce spam, I have
> tried not to Cc people on patches they aren't likely to care about.
> The complete series can be found in the LKML or linux-arch archives.]
>
> The core framework for the prctl() syscall is unloved and looking
> rather crusty these days.  It also relies on defining ancillary
> boilerplate macros for each prctl() in order to control conditional
> compilation of the different prctl calls.  We have better ways to
> do this now.

This is a nice clean-up series, thanks! Some thoughts/comments below...

> This series attempts to modernise the code by defining a couple of new
> Kconfig variables HAVE_PRCTL_ARCH and HAVE_ARCH_PR_SET_GET_UNALIGN to
> allow architectures to provide hooks for arch-dependent prctls.
>
>
> For now this series has had minimal testing: some basic testing of
> arch-specific prctls using PR_SVE_SET_VL on arm64; build-tested on
> x86; otherwise untested.
>
> This is not polished yet... but I'm interested to know what people
> think about the approach.

I'm not entirely convinced the removal of the "task not current"
interface is very useful as I've found myself needing to adjust other
interfaces like this to _add_ the "task not current" logic, but ... I
can't really argue very hard for keeping the task args since nothing
is using them... meh.

I dislike this being named "prctl_arch" when everything else like this
is "arch_foo...", but I do see the collision with the x86-specific
"arch_prctl" syscall. Perhaps it might be better to wire things up
differently, since x86's arch_prctl is actually "sys_arch_prctl" so I
don't think there would be a symbol collision, and maybe the 9 options
could be added to the global prctl list, with the x86 syscall getting
called at the end of the new "arch_prctl"? Then the syscall could get
deprecated in favor of just using prctl directly?

Your new x86 arch_prctl (neà prctl_arch) could be something like:

int arch_prctl(int option, unsigned long arg2, unsigned long arg3,
                     unsigned long arg4, unsigned long arg5)
{
... existing stuff in your series ...
    case ARCH_SET_GS:
    case ARCH_SET_FS:
    case ARCH_GET_GS:
    case ARCH_GET_FS:
    case ARCH_MAP_VDSO_X32:
    case ARCH_MAP_VDSO_32:
    case ARCH_MAP_VDSO_64:
        if (arg3 || arg4 || arg5)
            return -EINVAL;
        return do_arch_prctl_64(current, option, arg2);
    case ARCH_GET_CPUID:
    case ARCH_SET_CPUID:
        if (arg3 || arg4 || arg5)
            return -EINVAL;
        return do_arch_prctl_common(current, option, arg2);
    default:
        return -EINVAL;
    }
}

Or maybe all the logic could get ripped out of
arch/x86/kernel/process*.c and put in arch/x86/kernel/sys.c directly?

Perhaps this is all overkill, but I find it rather annoying that one
arch's weird syscall would block "expected" naming of a cross-arch
interface...

-Kees

-- 
Kees Cook
Pixel Security