| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190613110235.GW28398@e103592.cambridge.arm.com>
Date: Thu, 13 Jun 2019 12:02:35 +0100
From: Dave Martin <Dave.Martin@....com>
To: Andrey Konovalov <andreyknvl@...gle.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-rdma@...r.kernel.org,
linux-media@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org,
Mark Rutland <mark.rutland@....com>,
Szabolcs Nagy <Szabolcs.Nagy@....com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Kostya Serebryany <kcc@...gle.com>,
Khalid Aziz <khalid.aziz@...cle.com>,
Felix Kuehling <Felix.Kuehling@....com>,
Vincenzo Frascino <vincenzo.frascino@....com>,
Jacob Bramley <Jacob.Bramley@....com>,
Leon Romanovsky <leon@...nel.org>,
Christoph Hellwig <hch@...radead.org>,
Jason Gunthorpe <jgg@...pe.ca>,
Evgeniy Stepanov <eugenis@...gle.com>,
Kevin Brodsky <kevin.brodsky@....com>,
Kees Cook <keescook@...omium.org>,
Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
Alex Williamson <alex.williamson@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Dmitry Vyukov <dvyukov@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Yishai Hadas <yishaih@...lanox.com>,
Jens Wiklander <jens.wiklander@...aro.org>,
Lee Smith <Lee.Smith@....com>,
Alexander Deucher <Alexander.Deucher@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
enh <enh@...gle.com>, Robin Murphy <robin.murphy@....com>,
Christian Koenig <Christian.Koenig@....com>,
Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Subject: Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control
the tagged user addresses ABI
On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
> From: Catalin Marinas <catalin.marinas@....com>
>
> It is not desirable to relax the ABI to allow tagged user addresses into
> the kernel indiscriminately. This patch introduces a prctl() interface
> for enabling or disabling the tagged ABI with a global sysctl control
> for preventing applications from enabling the relaxed ABI (meant for
> testing user-space prctl() return error checking without reconfiguring
> the kernel). The ABI properties are inherited by threads of the same
> application and fork()'ed children but cleared on execve().
>
> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> MTE-specific settings like imprecise vs precise exceptions.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@....com>
> ---
> arch/arm64/include/asm/processor.h | 6 +++
> arch/arm64/include/asm/thread_info.h | 1 +
> arch/arm64/include/asm/uaccess.h | 3 +-
> arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++
> include/uapi/linux/prctl.h | 5 +++
> kernel/sys.c | 16 +++++++
> 6 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index fcd0e691b1ea..fee457456aa8 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void);
> /* PR_PAC_RESET_KEYS prctl */
> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>
> +/* PR_TAGGED_ADDR prctl */
> +long set_tagged_addr_ctrl(unsigned long arg);
> +long get_tagged_addr_ctrl(void);
> +#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg)
> +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
> +
> /*
> * For CONFIG_GCC_PLUGIN_STACKLEAK
> *
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index f1d032be628a..354a31d2b737 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -99,6 +99,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SVE 23 /* Scalable Vector Extension in use */
> #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
> #define TIF_SSBD 25 /* Wants SSB mitigation */
> +#define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */
>
> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED)
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index df729afca0ba..995b9ea11a89 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -73,7 +73,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> - addr = untagged_addr(addr);
> + if (test_thread_flag(TIF_TAGGED_ADDR))
> + addr = untagged_addr(addr);
>
> __chk_user_ptr(addr);
> asm volatile(
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb21a5b8..69d0be1fc708 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -30,6 +30,7 @@
> #include <linux/kernel.h>
> #include <linux/mm.h>
> #include <linux/stddef.h>
> +#include <linux/sysctl.h>
> #include <linux/unistd.h>
> #include <linux/user.h>
> #include <linux/delay.h>
> @@ -323,6 +324,7 @@ void flush_thread(void)
> fpsimd_flush_thread();
> tls_thread_flush();
> flush_ptrace_hw_breakpoint(current);
> + clear_thread_flag(TIF_TAGGED_ADDR);
> }
>
> void release_thread(struct task_struct *dead_task)
> @@ -552,3 +554,68 @@ void arch_setup_new_exec(void)
>
> ptrauth_thread_init_user(current);
> }
> +
> +/*
> + * Control the relaxed ABI allowing tagged user addresses into the kernel.
> + */
> +static unsigned int tagged_addr_prctl_allowed = 1;
> +
> +long set_tagged_addr_ctrl(unsigned long arg)
> +{
> + if (!tagged_addr_prctl_allowed)
> + return -EINVAL;
> + if (is_compat_task())
> + return -EINVAL;
> + if (arg & ~PR_TAGGED_ADDR_ENABLE)
> + return -EINVAL;
> +
> + if (arg & PR_TAGGED_ADDR_ENABLE)
> + set_thread_flag(TIF_TAGGED_ADDR);
> + else
> + clear_thread_flag(TIF_TAGGED_ADDR);
> +
> + return 0;
> +}
> +
> +long get_tagged_addr_ctrl(void)
> +{
> + if (!tagged_addr_prctl_allowed)
> + return -EINVAL;
> + if (is_compat_task())
> + return -EINVAL;
> +
> + if (test_thread_flag(TIF_TAGGED_ADDR))
> + return PR_TAGGED_ADDR_ENABLE;
> +
> + return 0;
> +}
> +
> +/*
> + * Global sysctl to disable the tagged user addresses support. This control
> + * only prevents the tagged address ABI enabling via prctl() and does not
> + * disable it for tasks that already opted in to the relaxed ABI.
> + */
> +static int zero;
> +static int one = 1;
!!!
And these can't even be const without a cast. Yuk.
(Not your fault though, but it would be nice to have a proc_dobool() to
avoid this.)
> +
> +static struct ctl_table tagged_addr_sysctl_table[] = {
> + {
> + .procname = "tagged_addr",
> + .mode = 0644,
> + .data = &tagged_addr_prctl_allowed,
> + .maxlen = sizeof(int),
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> + { }
> +};
> +
> +static int __init tagged_addr_init(void)
> +{
> + if (!register_sysctl("abi", tagged_addr_sysctl_table))
> + return -EINVAL;
> + return 0;
> +}
> +
> +core_initcall(tagged_addr_init);
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 094bb03b9cc2..2e927b3e9d6c 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -229,4 +229,9 @@ struct prctl_mm_map {
> # define PR_PAC_APDBKEY (1UL << 3)
> # define PR_PAC_APGAKEY (1UL << 4)
>
> +/* Tagged user address controls for arm64 */
> +#define PR_SET_TAGGED_ADDR_CTRL 55
> +#define PR_GET_TAGGED_ADDR_CTRL 56
> +# define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> +
Do we expect this prctl to be applicable to other arches, or is it
strictly arm64-specific?
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 2969304c29fe..ec48396b4943 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -124,6 +124,12 @@
> #ifndef PAC_RESET_KEYS
> # define PAC_RESET_KEYS(a, b) (-EINVAL)
> #endif
> +#ifndef SET_TAGGED_ADDR_CTRL
> +# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL)
> +#endif
> +#ifndef GET_TAGGED_ADDR_CTRL
> +# define GET_TAGGED_ADDR_CTRL() (-EINVAL)
> +#endif
>
> /*
> * this is where the system-wide overflow UID and GID are defined, for
> @@ -2492,6 +2498,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> error = PAC_RESET_KEYS(me, arg2);
> break;
> + case PR_SET_TAGGED_ADDR_CTRL:
> + if (arg3 || arg4 || arg5)
<bikeshed>
How do you anticipate these arguments being used in the future?
For the SVE prctls I took the view that "get" could only ever mean one
thing, and "put" already had a flags argument with spare bits for future
expansion anyway, so forcing the extra arguments to zero would be
unnecessary.
Opinions seem to differ on whether requiring surplus arguments to be 0
is beneficial for hygiene, but the glibc prototype for prctl() is
int prctl (int __option, ...);
so it seemed annoying to have to pass extra arguments to it just for the
sake of it. IMHO this also makes the code at the call site less
readable, since it's not immediately apparent that all those 0s are
meaningless.
</bikeshed>
(OTOH, the extra arguments are harmless and prctl is far from being a
general-purpose syscall.)
> + return -EINVAL;
> + error = SET_TAGGED_ADDR_CTRL(arg2);
> + break;
> + case PR_GET_TAGGED_ADDR_CTRL:
> + if (arg2 || arg3 || arg4 || arg5)
> + return -EINVAL;
> + error = GET_TAGGED_ADDR_CTRL();
Having a "get" prctl is probably a good idea, but is there a clear
usecase for it?
(The usecase for PR_SVE_GET_VL was always a bit dubious, since the
VL can also be read via an SVE insn or a compiler intrinsic, which is
less portable but much cheaper. As for the PR_SVE_SET_VL_INHERIT flag
that can be read via PR_SVE_GET_VL, I've never been sure how useful it
is to be able to read that...)
[...]
Cheers
---Dave
Powered by blists - more mailing lists