* [PATCH v2] arm64: add ptrace regsets for ptrauth key management @ 2019-01-10 19:35 Kristina Martsenko 2019-01-10 19:41 ` Kristina Martsenko 2019-01-11 13:31 ` Dave Martin 0 siblings, 2 replies; 10+ messages in thread From: Kristina Martsenko @ 2019-01-10 19:35 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Dave P Martin Cc: Mark Rutland, Amit Kachhap, linux-arm-kernel Add two new ptrace regsets, which can be used to request and change the pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access to the instruction/data address keys, and NT_ARM_PACG_KEYS to the generic authentication key. The keys are also part of the core dump file of the process. The regsets are only exposed if the kernel is compiled with CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing and restoring processes that are using pointer authentication. Normally applications or debuggers should not need to know the keys (and exposing the keys is a security risk), so the regsets are not exposed by default. Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> --- The previous version of this patch was: https://lore.kernel.org/lkml/20181207183931.4285-12-kristina.martsenko@arm.com/ Changes in v2: - Convert each field individually between ptrauth_keys and user_pac_address_keys/user_pac_generic_keys Documentation/arm64/pointer-authentication.txt | 5 + arch/arm64/include/uapi/asm/ptrace.h | 18 ++++ arch/arm64/kernel/ptrace.c | 144 +++++++++++++++++++++++++ include/uapi/linux/elf.h | 2 + 4 files changed, 169 insertions(+) diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt index a25cd21290e9..5baca42ba146 100644 --- a/Documentation/arm64/pointer-authentication.txt +++ b/Documentation/arm64/pointer-authentication.txt @@ -78,6 +78,11 @@ bits can vary between the two. Note that the masks apply to TTBR0 addresses, and are not valid to apply to TTBR1 addresses (e.g. kernel pointers). +Additionally, when CONFIG_CHECKPOINT_RESTORE is also set, the kernel +will expose the NT_ARM_PACA_KEYS and NT_ARM_PACG_KEYS regsets (struct +user_pac_address_keys and struct user_pac_generic_keys). These can be +used to get and set the keys for a thread. + Virtualization -------------- diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h index 28d77c9ed531..0affa43602a5 100644 --- a/arch/arm64/include/uapi/asm/ptrace.h +++ b/arch/arm64/include/uapi/asm/ptrace.h @@ -233,6 +233,24 @@ struct user_pac_mask { __u64 insn_mask; }; +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ + +struct user_pac_address_keys { + __u64 apiakey_lo; + __u64 apiakey_hi; + __u64 apibkey_lo; + __u64 apibkey_hi; + __u64 apdakey_lo; + __u64 apdakey_hi; + __u64 apdbkey_lo; + __u64 apdbkey_hi; +}; + +struct user_pac_generic_keys { + __u64 apgakey_lo; + __u64 apgakey_hi; +}; + #endif /* __ASSEMBLY__ */ #endif /* _UAPI__ASM_PTRACE_H */ diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 9dce33b0e260..a5f9f07e9eac 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -979,6 +979,128 @@ static int pac_mask_get(struct task_struct *target, return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &uregs, 0, -1); } + +#ifdef CONFIG_CHECKPOINT_RESTORE +static void pac_address_keys_to_user(struct user_pac_address_keys *ukeys, + struct ptrauth_keys *keys) +{ + ukeys->apiakey_lo = keys->apia.lo; + ukeys->apiakey_hi = keys->apia.hi; + ukeys->apibkey_lo = keys->apib.lo; + ukeys->apibkey_hi = keys->apib.hi; + ukeys->apdakey_lo = keys->apda.lo; + ukeys->apdakey_hi = keys->apda.hi; + ukeys->apdbkey_lo = keys->apdb.lo; + ukeys->apdbkey_hi = keys->apdb.hi; +} + +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, + struct ptrauth_keys *keys) +{ + keys->apia.lo = ukeys->apiakey_lo; + keys->apia.hi = ukeys->apiakey_hi; + keys->apib.lo = ukeys->apibkey_lo; + keys->apib.hi = ukeys->apibkey_hi; + keys->apda.lo = ukeys->apdakey_lo; + keys->apda.hi = ukeys->apdakey_hi; + keys->apdb.lo = ukeys->apdbkey_lo; + keys->apdb.hi = ukeys->apdbkey_hi; +} + +static int pac_address_keys_get(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + struct ptrauth_keys *keys = &target->thread.keys_user; + struct user_pac_address_keys user_keys; + + if (!system_supports_address_auth()) + return -EINVAL; + + memset(&user_keys, 0, sizeof(user_keys)); + pac_address_keys_to_user(&user_keys, keys); + + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, + &user_keys, 0, -1); +} + +static int pac_address_keys_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct ptrauth_keys *keys = &target->thread.keys_user; + struct user_pac_address_keys user_keys; + int ret; + + if (!system_supports_address_auth()) + return -EINVAL; + + pac_address_keys_to_user(&user_keys, keys); + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, + &user_keys, 0, -1); + if (ret) + return ret; + pac_address_keys_from_user(&user_keys, keys); + + return 0; +} + +static void pac_generic_keys_to_user(struct user_pac_generic_keys *ukeys, + struct ptrauth_keys *keys) +{ + ukeys->apgakey_lo = keys->apga.lo; + ukeys->apgakey_hi = keys->apga.hi; +} + +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, + struct ptrauth_keys *keys) +{ + keys->apga.lo = ukeys->apgakey_lo; + keys->apga.hi = ukeys->apgakey_hi; +} + +static int pac_generic_keys_get(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + struct ptrauth_keys *keys = &target->thread.keys_user; + struct user_pac_generic_keys user_keys; + + if (!system_supports_generic_auth()) + return -EINVAL; + + memset(&user_keys, 0, sizeof(user_keys)); + pac_generic_keys_to_user(&user_keys, keys); + + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, + &user_keys, 0, -1); +} + +static int pac_generic_keys_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + struct ptrauth_keys *keys = &target->thread.keys_user; + struct user_pac_generic_keys user_keys; + int ret; + + if (!system_supports_generic_auth()) + return -EINVAL; + + pac_generic_keys_to_user(&user_keys, keys); + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, + &user_keys, 0, -1); + if (ret) + return ret; + pac_generic_keys_from_user(&user_keys, keys); + + return 0; +} +#endif /* CONFIG_CHECKPOINT_RESTORE */ #endif /* CONFIG_ARM64_PTR_AUTH */ enum aarch64_regset { @@ -995,6 +1117,10 @@ enum aarch64_regset { #endif #ifdef CONFIG_ARM64_PTR_AUTH REGSET_PAC_MASK, +#ifdef CONFIG_CHECKPOINT_RESTORE + REGSET_PACA_KEYS, + REGSET_PACG_KEYS, +#endif #endif }; @@ -1074,6 +1200,24 @@ static const struct user_regset aarch64_regsets[] = { .get = pac_mask_get, /* this cannot be set dynamically */ }, +#ifdef CONFIG_CHECKPOINT_RESTORE + [REGSET_PACA_KEYS] = { + .core_note_type = NT_ARM_PACA_KEYS, + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), + .size = sizeof(u64), + .align = sizeof(u64), + .get = pac_address_keys_get, + .set = pac_address_keys_set, + }, + [REGSET_PACG_KEYS] = { + .core_note_type = NT_ARM_PACG_KEYS, + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), + .size = sizeof(u64), + .align = sizeof(u64), + .get = pac_generic_keys_get, + .set = pac_generic_keys_set, + }, +#endif #endif }; diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index e4d6ddd93567..34c02e4290fe 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -421,6 +421,8 @@ typedef struct elf64_shdr { #define NT_ARM_SYSTEM_CALL 0x404 /* ARM system call number */ #define NT_ARM_SVE 0x405 /* ARM Scalable Vector Extension registers */ #define NT_ARM_PAC_MASK 0x406 /* ARM pointer authentication code masks */ +#define NT_ARM_PACA_KEYS 0x407 /* ARM pointer authentication address keys */ +#define NT_ARM_PACG_KEYS 0x408 /* ARM pointer authentication generic key */ #define NT_ARC_V2 0x600 /* ARCv2 accumulator/extra registers */ #define NT_VMCOREDD 0x700 /* Vmcore Device Dump Note */ #define NT_MIPS_DSP 0x800 /* MIPS DSP ASE registers */ -- 2.11.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-10 19:35 [PATCH v2] arm64: add ptrace regsets for ptrauth key management Kristina Martsenko @ 2019-01-10 19:41 ` Kristina Martsenko 2019-01-11 13:58 ` Dave Martin 2019-01-11 13:31 ` Dave Martin 1 sibling, 1 reply; 10+ messages in thread From: Kristina Martsenko @ 2019-01-10 19:41 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Dave P Martin Cc: Mark Rutland, Amit Kachhap, linux-arm-kernel On 10/01/2019 19:35, Kristina Martsenko wrote: > Add two new ptrace regsets, which can be used to request and change the > pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access > to the instruction/data address keys, and NT_ARM_PACG_KEYS to the > generic authentication key. The keys are also part of the core dump file > of the process. > > The regsets are only exposed if the kernel is compiled with > CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing > and restoring processes that are using pointer authentication. Normally > applications or debuggers should not need to know the keys (and exposing > the keys is a security risk), so the regsets are not exposed by default. > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > --- > > The previous version of this patch was: > https://lore.kernel.org/lkml/20181207183931.4285-12-kristina.martsenko@arm.com/ > > Changes in v2: > - Convert each field individually between ptrauth_keys and > user_pac_address_keys/user_pac_generic_keys For comparison, this is what the patch might look like if we instead used struct user_pac_address_keys/user_pac_generic_keys in both ptrace and the kernel: -- >8 -- Subject: [PATCH] arm64: add ptrace regsets for ptrauth key management Add two new ptrace regsets, which can be used to request and change the pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access to the instruction/data address keys, and NT_ARM_PACG_KEYS to the generic authentication key. The keys are also part of the core dump file of the process. The regsets are only exposed if the kernel is compiled with CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing and restoring processes that are using pointer authentication. Normally applications or debuggers should not need to know the keys (and exposing the keys is a security risk), so the regsets are not exposed by default. Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> --- Documentation/arm64/pointer-authentication.txt | 5 ++ arch/arm64/include/asm/pointer_auth.h | 47 ++++++---------- arch/arm64/include/asm/processor.h | 9 ++- arch/arm64/include/uapi/asm/ptrace.h | 18 ++++++ arch/arm64/kernel/pointer_auth.c | 19 +++++-- arch/arm64/kernel/ptrace.c | 76 ++++++++++++++++++++++++++ include/uapi/linux/elf.h | 2 + 7 files changed, 136 insertions(+), 40 deletions(-) diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt index a25cd21290e9..5baca42ba146 100644 --- a/Documentation/arm64/pointer-authentication.txt +++ b/Documentation/arm64/pointer-authentication.txt @@ -78,6 +78,11 @@ bits can vary between the two. Note that the masks apply to TTBR0 addresses, and are not valid to apply to TTBR1 addresses (e.g. kernel pointers). +Additionally, when CONFIG_CHECKPOINT_RESTORE is also set, the kernel +will expose the NT_ARM_PACA_KEYS and NT_ARM_PACG_KEYS regsets (struct +user_pac_address_keys and struct user_pac_generic_keys). These can be +used to get and set the keys for a thread. + Virtualization -------------- diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h index 15d49515efdd..2fde45d0e24f 100644 --- a/arch/arm64/include/asm/pointer_auth.h +++ b/arch/arm64/include/asm/pointer_auth.h @@ -7,60 +7,45 @@ #include <asm/cpufeature.h> #include <asm/memory.h> +#include <asm/ptrace.h> #include <asm/sysreg.h> #ifdef CONFIG_ARM64_PTR_AUTH /* - * Each key is a 128-bit quantity which is split across a pair of 64-bit - * registers (Lo and Hi). - */ -struct ptrauth_key { - unsigned long lo, hi; -}; - -/* * We give each process its own keys, which are shared by all threads. The keys * are inherited upon fork(), and reinitialised upon exec*(). */ struct ptrauth_keys { - struct ptrauth_key apia; - struct ptrauth_key apib; - struct ptrauth_key apda; - struct ptrauth_key apdb; - struct ptrauth_key apga; + struct user_pac_address_keys addr_keys; + struct user_pac_generic_keys gen_keys; }; static inline void ptrauth_keys_init(struct ptrauth_keys *keys) { - if (system_supports_address_auth()) { - get_random_bytes(&keys->apia, sizeof(keys->apia)); - get_random_bytes(&keys->apib, sizeof(keys->apib)); - get_random_bytes(&keys->apda, sizeof(keys->apda)); - get_random_bytes(&keys->apdb, sizeof(keys->apdb)); - } + if (system_supports_address_auth()) + get_random_bytes(&keys->addr_keys, sizeof(keys->addr_keys)); if (system_supports_generic_auth()) - get_random_bytes(&keys->apga, sizeof(keys->apga)); + get_random_bytes(&keys->gen_keys, sizeof(keys->gen_keys)); } #define __ptrauth_key_install(k, v) \ do { \ - struct ptrauth_key __pki_v = (v); \ - write_sysreg_s(__pki_v.lo, SYS_ ## k ## KEYLO_EL1); \ - write_sysreg_s(__pki_v.hi, SYS_ ## k ## KEYHI_EL1); \ + write_sysreg_s(v ## _lo, SYS_ ## k ## KEYLO_EL1); \ + write_sysreg_s(v ## _hi, SYS_ ## k ## KEYHI_EL1); \ } while (0) static inline void ptrauth_keys_switch(struct ptrauth_keys *keys) { if (system_supports_address_auth()) { - __ptrauth_key_install(APIA, keys->apia); - __ptrauth_key_install(APIB, keys->apib); - __ptrauth_key_install(APDA, keys->apda); - __ptrauth_key_install(APDB, keys->apdb); + __ptrauth_key_install(APIA, keys->addr_keys.apiakey); + __ptrauth_key_install(APIB, keys->addr_keys.apibkey); + __ptrauth_key_install(APDA, keys->addr_keys.apdakey); + __ptrauth_key_install(APDB, keys->addr_keys.apdbkey); } if (system_supports_generic_auth()) - __ptrauth_key_install(APGA, keys->apga); + __ptrauth_key_install(APGA, keys->gen_keys.apgakey); } extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg); @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) #define ptrauth_thread_init_user(tsk) \ do { \ struct task_struct *__ptiu_tsk = (tsk); \ - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ } while (0) #define ptrauth_thread_switch(tsk) \ - ptrauth_keys_switch(&(tsk)->thread.keys_user) + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) #else /* CONFIG_ARM64_PTR_AUTH */ #define ptrauth_prctl_reset_keys(tsk, arg) (-EINVAL) diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index f1a7ab18faf3..f1d41bac9a23 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -137,6 +137,9 @@ struct thread_struct { struct { unsigned long tp_value; /* TLS register */ unsigned long tp2_value; +#ifdef CONFIG_ARM64_PTR_AUTH + struct ptrauth_keys keys_user; +#endif struct user_fpsimd_state fpsimd_state; } uw; @@ -147,9 +150,6 @@ struct thread_struct { unsigned long fault_address; /* fault info */ unsigned long fault_code; /* ESR_EL1 value */ struct debug_info debug; /* debugging */ -#ifdef CONFIG_ARM64_PTR_AUTH - struct ptrauth_keys keys_user; -#endif }; static inline void arch_thread_struct_whitelist(unsigned long *offset, @@ -159,6 +159,9 @@ static inline void arch_thread_struct_whitelist(unsigned long *offset, BUILD_BUG_ON(sizeof_field(struct thread_struct, uw) != sizeof_field(struct thread_struct, uw.tp_value) + sizeof_field(struct thread_struct, uw.tp2_value) + +#ifdef CONFIG_ARM64_PTR_AUTH + sizeof_field(struct thread_struct, uw.keys_user) + +#endif sizeof_field(struct thread_struct, uw.fpsimd_state)); *offset = offsetof(struct thread_struct, uw); diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h index 28d77c9ed531..0affa43602a5 100644 --- a/arch/arm64/include/uapi/asm/ptrace.h +++ b/arch/arm64/include/uapi/asm/ptrace.h @@ -233,6 +233,24 @@ struct user_pac_mask { __u64 insn_mask; }; +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ + +struct user_pac_address_keys { + __u64 apiakey_lo; + __u64 apiakey_hi; + __u64 apibkey_lo; + __u64 apibkey_hi; + __u64 apdakey_lo; + __u64 apdakey_hi; + __u64 apdbkey_lo; + __u64 apdbkey_hi; +}; + +struct user_pac_generic_keys { + __u64 apgakey_lo; + __u64 apgakey_hi; +}; + #endif /* __ASSEMBLY__ */ #endif /* _UAPI__ASM_PTRACE_H */ diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c index c507b584259d..3fd90ec5efb2 100644 --- a/arch/arm64/kernel/pointer_auth.c +++ b/arch/arm64/kernel/pointer_auth.c @@ -7,9 +7,16 @@ #include <asm/cpufeature.h> #include <asm/pointer_auth.h> + +#define ptrauth_reset_key(key) \ +do { \ + get_random_bytes(&key ## _lo, sizeof(key ## _lo)); \ + get_random_bytes(&key ## _hi, sizeof(key ## _hi)); \ +} while (0) + int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) { - struct ptrauth_keys *keys = &tsk->thread.keys_user; + struct ptrauth_keys *keys = &tsk->thread.uw.keys_user; unsigned long addr_key_mask = PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY | PR_PAC_APDBKEY; unsigned long key_mask = addr_key_mask | PR_PAC_APGAKEY; @@ -31,15 +38,15 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) return -EINVAL; if (arg & PR_PAC_APIAKEY) - get_random_bytes(&keys->apia, sizeof(keys->apia)); + ptrauth_reset_key(keys->addr_keys.apiakey); if (arg & PR_PAC_APIBKEY) - get_random_bytes(&keys->apib, sizeof(keys->apib)); + ptrauth_reset_key(keys->addr_keys.apibkey); if (arg & PR_PAC_APDAKEY) - get_random_bytes(&keys->apda, sizeof(keys->apda)); + ptrauth_reset_key(keys->addr_keys.apdakey); if (arg & PR_PAC_APDBKEY) - get_random_bytes(&keys->apdb, sizeof(keys->apdb)); + ptrauth_reset_key(keys->addr_keys.apdbkey); if (arg & PR_PAC_APGAKEY) - get_random_bytes(&keys->apga, sizeof(keys->apga)); + ptrauth_reset_key(keys->gen_keys.apgakey); ptrauth_keys_switch(keys); diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 9dce33b0e260..cd537bd669e7 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -979,6 +979,60 @@ static int pac_mask_get(struct task_struct *target, return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &uregs, 0, -1); } + +#ifdef CONFIG_CHECKPOINT_RESTORE +static int pac_address_keys_get(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + if (!system_supports_address_auth()) + return -EINVAL; + + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, + &target->thread.uw.keys_user.addr_keys, + 0, -1); +} + +static int pac_address_keys_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + if (!system_supports_address_auth()) + return -EINVAL; + + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, + &target->thread.uw.keys_user.addr_keys, + 0, -1); +} + +static int pac_generic_keys_get(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + void *kbuf, void __user *ubuf) +{ + if (!system_supports_generic_auth()) + return -EINVAL; + + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, + &target->thread.uw.keys_user.gen_keys, + 0, -1); +} + +static int pac_generic_keys_set(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf) +{ + if (!system_supports_generic_auth()) + return -EINVAL; + + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, + &target->thread.uw.keys_user.gen_keys, + 0, -1); +} +#endif /* CONFIG_CHECKPOINT_RESTORE */ #endif /* CONFIG_ARM64_PTR_AUTH */ enum aarch64_regset { @@ -995,6 +1049,10 @@ enum aarch64_regset { #endif #ifdef CONFIG_ARM64_PTR_AUTH REGSET_PAC_MASK, +#ifdef CONFIG_CHECKPOINT_RESTORE + REGSET_PACA_KEYS, + REGSET_PACG_KEYS, +#endif #endif }; @@ -1074,6 +1132,24 @@ static const struct user_regset aarch64_regsets[] = { .get = pac_mask_get, /* this cannot be set dynamically */ }, +#ifdef CONFIG_CHECKPOINT_RESTORE + [REGSET_PACA_KEYS] = { + .core_note_type = NT_ARM_PACA_KEYS, + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), + .size = sizeof(u64), + .align = sizeof(u64), + .get = pac_address_keys_get, + .set = pac_address_keys_set, + }, + [REGSET_PACG_KEYS] = { + .core_note_type = NT_ARM_PACG_KEYS, + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), + .size = sizeof(u64), + .align = sizeof(u64), + .get = pac_generic_keys_get, + .set = pac_generic_keys_set, + }, +#endif #endif }; diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h index e4d6ddd93567..34c02e4290fe 100644 --- a/include/uapi/linux/elf.h +++ b/include/uapi/linux/elf.h @@ -421,6 +421,8 @@ typedef struct elf64_shdr { #define NT_ARM_SYSTEM_CALL 0x404 /* ARM system call number */ #define NT_ARM_SVE 0x405 /* ARM Scalable Vector Extension registers */ #define NT_ARM_PAC_MASK 0x406 /* ARM pointer authentication code masks */ +#define NT_ARM_PACA_KEYS 0x407 /* ARM pointer authentication address keys */ +#define NT_ARM_PACG_KEYS 0x408 /* ARM pointer authentication generic key */ #define NT_ARC_V2 0x600 /* ARCv2 accumulator/extra registers */ #define NT_VMCOREDD 0x700 /* Vmcore Device Dump Note */ #define NT_MIPS_DSP 0x800 /* MIPS DSP ASE registers */ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-10 19:41 ` Kristina Martsenko @ 2019-01-11 13:58 ` Dave Martin 2019-01-15 19:32 ` Kristina Martsenko 0 siblings, 1 reply; 10+ messages in thread From: Dave Martin @ 2019-01-11 13:58 UTC (permalink / raw) To: Kristina Martsenko Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On Thu, Jan 10, 2019 at 07:41:15PM +0000, Kristina Martsenko wrote: > On 10/01/2019 19:35, Kristina Martsenko wrote: > > Add two new ptrace regsets, which can be used to request and change the > > pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access > > to the instruction/data address keys, and NT_ARM_PACG_KEYS to the > > generic authentication key. The keys are also part of the core dump file > > of the process. > > > > The regsets are only exposed if the kernel is compiled with > > CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing > > and restoring processes that are using pointer authentication. Normally > > applications or debuggers should not need to know the keys (and exposing > > the keys is a security risk), so the regsets are not exposed by default. Although we can live with this, I still think it gives a false sense of safety. Can we come up with an scenario where an attacker with ptrace or coredump access can do more damage with access to the pointer auth keys than without? A lot of systems will run with CONFIG_CHECKPOINT_RESTORE=y (like packaged Debian kernels for example). And more paranoid systems already restrict or disable ptrace anyway. > > > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > > --- > > > > The previous version of this patch was: > > https://lore.kernel.org/lkml/20181207183931.4285-12-kristina.martsenko@arm.com/ > > > > Changes in v2: > > - Convert each field individually between ptrauth_keys and > > user_pac_address_keys/user_pac_generic_keys > > For comparison, this is what the patch might look like if we instead > used struct user_pac_address_keys/user_pac_generic_keys in both ptrace > and the kernel: > > -- >8 -- > Subject: [PATCH] arm64: add ptrace regsets for ptrauth key management > > Add two new ptrace regsets, which can be used to request and change the > pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access > to the instruction/data address keys, and NT_ARM_PACG_KEYS to the > generic authentication key. The keys are also part of the core dump file > of the process. > > The regsets are only exposed if the kernel is compiled with > CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing > and restoring processes that are using pointer authentication. Normally > applications or debuggers should not need to know the keys (and exposing > the keys is a security risk), so the regsets are not exposed by default. > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > --- > Documentation/arm64/pointer-authentication.txt | 5 ++ > arch/arm64/include/asm/pointer_auth.h | 47 ++++++---------- > arch/arm64/include/asm/processor.h | 9 ++- > arch/arm64/include/uapi/asm/ptrace.h | 18 ++++++ > arch/arm64/kernel/pointer_auth.c | 19 +++++-- > arch/arm64/kernel/ptrace.c | 76 ++++++++++++++++++++++++++ > include/uapi/linux/elf.h | 2 + > 7 files changed, 136 insertions(+), 40 deletions(-) > > diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt > index a25cd21290e9..5baca42ba146 100644 > --- a/Documentation/arm64/pointer-authentication.txt > +++ b/Documentation/arm64/pointer-authentication.txt > @@ -78,6 +78,11 @@ bits can vary between the two. Note that the masks apply to TTBR0 > addresses, and are not valid to apply to TTBR1 addresses (e.g. kernel > pointers). > > +Additionally, when CONFIG_CHECKPOINT_RESTORE is also set, the kernel > +will expose the NT_ARM_PACA_KEYS and NT_ARM_PACG_KEYS regsets (struct > +user_pac_address_keys and struct user_pac_generic_keys). These can be > +used to get and set the keys for a thread. > + > > Virtualization > -------------- > diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h > index 15d49515efdd..2fde45d0e24f 100644 > --- a/arch/arm64/include/asm/pointer_auth.h > +++ b/arch/arm64/include/asm/pointer_auth.h > @@ -7,60 +7,45 @@ > > #include <asm/cpufeature.h> > #include <asm/memory.h> > +#include <asm/ptrace.h> > #include <asm/sysreg.h> > > #ifdef CONFIG_ARM64_PTR_AUTH > /* > - * Each key is a 128-bit quantity which is split across a pair of 64-bit > - * registers (Lo and Hi). > - */ > -struct ptrauth_key { > - unsigned long lo, hi; > -}; > - > -/* > * We give each process its own keys, which are shared by all threads. The keys > * are inherited upon fork(), and reinitialised upon exec*(). > */ > struct ptrauth_keys { > - struct ptrauth_key apia; > - struct ptrauth_key apib; > - struct ptrauth_key apda; > - struct ptrauth_key apdb; > - struct ptrauth_key apga; > + struct user_pac_address_keys addr_keys; > + struct user_pac_generic_keys gen_keys; > }; > > static inline void ptrauth_keys_init(struct ptrauth_keys *keys) > { > - if (system_supports_address_auth()) { > - get_random_bytes(&keys->apia, sizeof(keys->apia)); > - get_random_bytes(&keys->apib, sizeof(keys->apib)); > - get_random_bytes(&keys->apda, sizeof(keys->apda)); > - get_random_bytes(&keys->apdb, sizeof(keys->apdb)); > - } > + if (system_supports_address_auth()) > + get_random_bytes(&keys->addr_keys, sizeof(keys->addr_keys)); > > if (system_supports_generic_auth()) > - get_random_bytes(&keys->apga, sizeof(keys->apga)); > + get_random_bytes(&keys->gen_keys, sizeof(keys->gen_keys)); > } > > #define __ptrauth_key_install(k, v) \ > do { \ > - struct ptrauth_key __pki_v = (v); \ > - write_sysreg_s(__pki_v.lo, SYS_ ## k ## KEYLO_EL1); \ > - write_sysreg_s(__pki_v.hi, SYS_ ## k ## KEYHI_EL1); \ > + write_sysreg_s(v ## _lo, SYS_ ## k ## KEYLO_EL1); \ > + write_sysreg_s(v ## _hi, SYS_ ## k ## KEYHI_EL1); \ > } while (0) > > static inline void ptrauth_keys_switch(struct ptrauth_keys *keys) > { > if (system_supports_address_auth()) { > - __ptrauth_key_install(APIA, keys->apia); > - __ptrauth_key_install(APIB, keys->apib); > - __ptrauth_key_install(APDA, keys->apda); > - __ptrauth_key_install(APDB, keys->apdb); > + __ptrauth_key_install(APIA, keys->addr_keys.apiakey); > + __ptrauth_key_install(APIB, keys->addr_keys.apibkey); > + __ptrauth_key_install(APDA, keys->addr_keys.apdakey); > + __ptrauth_key_install(APDB, keys->addr_keys.apdbkey); Aren't the members of struct user_pac_address_keys split up into apiakey_lo, apiakey_hi etc.? However, I think there's no reason not to pair up the keys in nested structs in the user struct, so could we change that struct to be more like the old struct ptrauth_key and keep the above code? > } > > if (system_supports_generic_auth()) > - __ptrauth_key_install(APGA, keys->apga); > + __ptrauth_key_install(APGA, keys->gen_keys.apgakey); > } > > extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg); > @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > #define ptrauth_thread_init_user(tsk) \ > do { \ > struct task_struct *__ptiu_tsk = (tsk); \ Not added by this patch, but __ptiu_tsk doesn't seem to do anything except make the subsquent lines more verbose than otherwise (and pollute the identifier namespace -- though unlikely to be a problem). It may not be worth dropping it now that it's there though. > - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ > - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ > + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ > + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ > } while (0) > > #define ptrauth_thread_switch(tsk) \ > - ptrauth_keys_switch(&(tsk)->thread.keys_user) > + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) > > #else /* CONFIG_ARM64_PTR_AUTH */ > #define ptrauth_prctl_reset_keys(tsk, arg) (-EINVAL) > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index f1a7ab18faf3..f1d41bac9a23 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -137,6 +137,9 @@ struct thread_struct { > struct { > unsigned long tp_value; /* TLS register */ > unsigned long tp2_value; > +#ifdef CONFIG_ARM64_PTR_AUTH > + struct ptrauth_keys keys_user; > +#endif > struct user_fpsimd_state fpsimd_state; > } uw; > > @@ -147,9 +150,6 @@ struct thread_struct { > unsigned long fault_address; /* fault info */ > unsigned long fault_code; /* ESR_EL1 value */ > struct debug_info debug; /* debugging */ > -#ifdef CONFIG_ARM64_PTR_AUTH > - struct ptrauth_keys keys_user; > -#endif > }; > > static inline void arch_thread_struct_whitelist(unsigned long *offset, > @@ -159,6 +159,9 @@ static inline void arch_thread_struct_whitelist(unsigned long *offset, > BUILD_BUG_ON(sizeof_field(struct thread_struct, uw) != > sizeof_field(struct thread_struct, uw.tp_value) + > sizeof_field(struct thread_struct, uw.tp2_value) + > +#ifdef CONFIG_ARM64_PTR_AUTH > + sizeof_field(struct thread_struct, uw.keys_user) + > +#endif > sizeof_field(struct thread_struct, uw.fpsimd_state)); > > *offset = offsetof(struct thread_struct, uw); > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h > index 28d77c9ed531..0affa43602a5 100644 > --- a/arch/arm64/include/uapi/asm/ptrace.h > +++ b/arch/arm64/include/uapi/asm/ptrace.h > @@ -233,6 +233,24 @@ struct user_pac_mask { > __u64 insn_mask; > }; > > +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ > + > +struct user_pac_address_keys { > + __u64 apiakey_lo; > + __u64 apiakey_hi; > + __u64 apibkey_lo; > + __u64 apibkey_hi; > + __u64 apdakey_lo; > + __u64 apdakey_hi; > + __u64 apdbkey_lo; > + __u64 apdbkey_hi; > +}; > + > +struct user_pac_generic_keys { > + __u64 apgakey_lo; > + __u64 apgakey_hi; > +}; > + As noted above, I think we could happily have a struct user_pac_key to pack up the halves of each key, like the old kernel struct. > #endif /* __ASSEMBLY__ */ > > #endif /* _UAPI__ASM_PTRACE_H */ > diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c > index c507b584259d..3fd90ec5efb2 100644 > --- a/arch/arm64/kernel/pointer_auth.c > +++ b/arch/arm64/kernel/pointer_auth.c > @@ -7,9 +7,16 @@ > #include <asm/cpufeature.h> > #include <asm/pointer_auth.h> > > + > +#define ptrauth_reset_key(key) \ > +do { \ > + get_random_bytes(&key ## _lo, sizeof(key ## _lo)); \ > + get_random_bytes(&key ## _hi, sizeof(key ## _hi)); \ > +} while (0) > + > int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) > { > - struct ptrauth_keys *keys = &tsk->thread.keys_user; > + struct ptrauth_keys *keys = &tsk->thread.uw.keys_user; > unsigned long addr_key_mask = PR_PAC_APIAKEY | PR_PAC_APIBKEY | > PR_PAC_APDAKEY | PR_PAC_APDBKEY; > unsigned long key_mask = addr_key_mask | PR_PAC_APGAKEY; > @@ -31,15 +38,15 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) > return -EINVAL; > > if (arg & PR_PAC_APIAKEY) > - get_random_bytes(&keys->apia, sizeof(keys->apia)); > + ptrauth_reset_key(keys->addr_keys.apiakey); > if (arg & PR_PAC_APIBKEY) > - get_random_bytes(&keys->apib, sizeof(keys->apib)); > + ptrauth_reset_key(keys->addr_keys.apibkey); > if (arg & PR_PAC_APDAKEY) > - get_random_bytes(&keys->apda, sizeof(keys->apda)); > + ptrauth_reset_key(keys->addr_keys.apdakey); > if (arg & PR_PAC_APDBKEY) > - get_random_bytes(&keys->apdb, sizeof(keys->apdb)); > + ptrauth_reset_key(keys->addr_keys.apdbkey); > if (arg & PR_PAC_APGAKEY) > - get_random_bytes(&keys->apga, sizeof(keys->apga)); > + ptrauth_reset_key(keys->gen_keys.apgakey); > > ptrauth_keys_switch(keys); > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index 9dce33b0e260..cd537bd669e7 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -979,6 +979,60 @@ static int pac_mask_get(struct task_struct *target, > > return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &uregs, 0, -1); > } > + > +#ifdef CONFIG_CHECKPOINT_RESTORE > +static int pac_address_keys_get(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + void *kbuf, void __user *ubuf) > +{ > + if (!system_supports_address_auth()) > + return -EINVAL; > + > + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, > + &target->thread.uw.keys_user.addr_keys, > + 0, -1); > +} > + > +static int pac_address_keys_set(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, const void __user *ubuf) > +{ > + if (!system_supports_address_auth()) > + return -EINVAL; > + > + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + &target->thread.uw.keys_user.addr_keys, > + 0, -1); > +} > + > +static int pac_generic_keys_get(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + void *kbuf, void __user *ubuf) > +{ > + if (!system_supports_generic_auth()) > + return -EINVAL; > + > + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, > + &target->thread.uw.keys_user.gen_keys, > + 0, -1); > +} > + > +static int pac_generic_keys_set(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, const void __user *ubuf) > +{ > + if (!system_supports_generic_auth()) > + return -EINVAL; > + > + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + &target->thread.uw.keys_user.gen_keys, > + 0, -1); > +} > +#endif /* CONFIG_CHECKPOINT_RESTORE */ > #endif /* CONFIG_ARM64_PTR_AUTH */ > > enum aarch64_regset { > @@ -995,6 +1049,10 @@ enum aarch64_regset { > #endif > #ifdef CONFIG_ARM64_PTR_AUTH > REGSET_PAC_MASK, > +#ifdef CONFIG_CHECKPOINT_RESTORE > + REGSET_PACA_KEYS, > + REGSET_PACG_KEYS, > +#endif > #endif > }; > > @@ -1074,6 +1132,24 @@ static const struct user_regset aarch64_regsets[] = { > .get = pac_mask_get, > /* this cannot be set dynamically */ > }, > +#ifdef CONFIG_CHECKPOINT_RESTORE && defined(CONFIG_ARM64_PTR_AUTH) ? > + [REGSET_PACA_KEYS] = { > + .core_note_type = NT_ARM_PACA_KEYS, > + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), > + .size = sizeof(u64), > + .align = sizeof(u64), > + .get = pac_address_keys_get, > + .set = pac_address_keys_set, > + }, > + [REGSET_PACG_KEYS] = { > + .core_note_type = NT_ARM_PACG_KEYS, > + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), > + .size = sizeof(u64), > + .align = sizeof(u64), > + .get = pac_generic_keys_get, > + .set = pac_generic_keys_set, > + }, > +#endif > #endif > }; > [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-11 13:58 ` Dave Martin @ 2019-01-15 19:32 ` Kristina Martsenko 2019-01-16 15:13 ` Dave Martin 0 siblings, 1 reply; 10+ messages in thread From: Kristina Martsenko @ 2019-01-15 19:32 UTC (permalink / raw) To: Dave Martin Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On 11/01/2019 13:58, Dave Martin wrote: > On Thu, Jan 10, 2019 at 07:41:15PM +0000, Kristina Martsenko wrote: >> On 10/01/2019 19:35, Kristina Martsenko wrote: >>> Add two new ptrace regsets, which can be used to request and change the >>> pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access >>> to the instruction/data address keys, and NT_ARM_PACG_KEYS to the >>> generic authentication key. The keys are also part of the core dump file >>> of the process. >>> >>> The regsets are only exposed if the kernel is compiled with >>> CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing >>> and restoring processes that are using pointer authentication. Normally >>> applications or debuggers should not need to know the keys (and exposing >>> the keys is a security risk), so the regsets are not exposed by default. > > Although we can live with this, I still think it gives a false sense of > safety. > > Can we come up with an scenario where an attacker with ptrace or > coredump access can do more damage with access to the pointer auth keys > than without? > > A lot of systems will run with CONFIG_CHECKPOINT_RESTORE=y (like > packaged Debian kernels for example). And more paranoid systems already > restrict or disable ptrace anyway. I can't think of such a scenario, as ptrace gives access to registers and memory anyway. But I probably haven't thought of all scenarios. There are other ptrace options that are only available when CONFIG_CHECKPOINT_RESTORE=y, such as PTRACE_SECCOMP_GET_FILTER. I think Will had a preference for having this depend on CONFIG_CHECKPOINT_RESTORE, so I will keep it for now. >> #define __ptrauth_key_install(k, v) \ >> do { \ >> - struct ptrauth_key __pki_v = (v); \ >> - write_sysreg_s(__pki_v.lo, SYS_ ## k ## KEYLO_EL1); \ >> - write_sysreg_s(__pki_v.hi, SYS_ ## k ## KEYHI_EL1); \ >> + write_sysreg_s(v ## _lo, SYS_ ## k ## KEYLO_EL1); \ >> + write_sysreg_s(v ## _hi, SYS_ ## k ## KEYHI_EL1); \ >> } while (0) >> >> static inline void ptrauth_keys_switch(struct ptrauth_keys *keys) >> { >> if (system_supports_address_auth()) { >> - __ptrauth_key_install(APIA, keys->apia); >> - __ptrauth_key_install(APIB, keys->apib); >> - __ptrauth_key_install(APDA, keys->apda); >> - __ptrauth_key_install(APDB, keys->apdb); >> + __ptrauth_key_install(APIA, keys->addr_keys.apiakey); >> + __ptrauth_key_install(APIB, keys->addr_keys.apibkey); >> + __ptrauth_key_install(APDA, keys->addr_keys.apdakey); >> + __ptrauth_key_install(APDB, keys->addr_keys.apdbkey); > > Aren't the members of struct user_pac_address_keys split up into > apiakey_lo, apiakey_hi etc.? They are, which is why the __ptrauth_key_install macro pastes a "_lo" or "_hi" at the end of the field name. I don't think this is very nice, which is one of the reasons why I prefer the other patch (where we keep the kernel and ptrace structs separate). > However, I think there's no reason not to pair up the keys in nested > structs in the user struct, so could we change that struct to be more > like the old struct ptrauth_key and keep the above code? We don't currently have any other separate nested structs in the arm64 ptrace userspace interface. Other architectures also seem to only have a few rare instances of this. It seems odd to complicate the userspace interface because of a kernel implementation detail, but if you think it's better I could change the structs to: struct user_pac_key { __u64 lo; __u64 hi; }; struct user_pac_address_keys { struct user_pac_key apiakey; struct user_pac_key apibkey; struct user_pac_key apdakey; struct user_pac_key apdbkey; }; struct user_pac_generic_keys { struct user_pac_key apgakey; }; >> } >> >> if (system_supports_generic_auth()) >> - __ptrauth_key_install(APGA, keys->apga); >> + __ptrauth_key_install(APGA, keys->gen_keys.apgakey); >> } >> >> extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg); >> @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) >> #define ptrauth_thread_init_user(tsk) \ >> do { \ >> struct task_struct *__ptiu_tsk = (tsk); \ > > Not added by this patch, but __ptiu_tsk doesn't seem to do anything > except make the subsquent lines more verbose than otherwise (and pollute > the identifier namespace -- though unlikely to be a problem). > > It may not be worth dropping it now that it's there though. Using __ptiu_tsk prevents the argument (tsk) from being evaluated twice, which could have side effects. >> - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ >> - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ >> + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ >> + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ >> } while (0) >> >> #define ptrauth_thread_switch(tsk) \ >> - ptrauth_keys_switch(&(tsk)->thread.keys_user) >> + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) [...] >> --- a/arch/arm64/include/uapi/asm/ptrace.h >> +++ b/arch/arm64/include/uapi/asm/ptrace.h >> @@ -233,6 +233,24 @@ struct user_pac_mask { >> __u64 insn_mask; >> }; >> >> +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ >> + >> +struct user_pac_address_keys { >> + __u64 apiakey_lo; >> + __u64 apiakey_hi; >> + __u64 apibkey_lo; >> + __u64 apibkey_hi; >> + __u64 apdakey_lo; >> + __u64 apdakey_hi; >> + __u64 apdbkey_lo; >> + __u64 apdbkey_hi; >> +}; >> + >> +struct user_pac_generic_keys { >> + __u64 apgakey_lo; >> + __u64 apgakey_hi; >> +}; >> + > > As noted above, I think we could happily have a struct user_pac_key to > pack up the halves of each key, like the old kernel struct. See my answer above. Also note that with this patch we have struct "user_pac_address_keys" in struct ptrauth_keys, which may be confusing once we start using pointer authentication in the kernel and use struct ptrauth_keys for kernel keys as well, not just user keys. >> @@ -1074,6 +1132,24 @@ static const struct user_regset aarch64_regsets[] = { >> .get = pac_mask_get, >> /* this cannot be set dynamically */ >> }, >> +#ifdef CONFIG_CHECKPOINT_RESTORE > > && defined(CONFIG_ARM64_PTR_AUTH) ? No, this is already inside a larger "#ifdef CONFIG_ARM64_PTR_AUTH" block. >> + [REGSET_PACA_KEYS] = { >> + .core_note_type = NT_ARM_PACA_KEYS, >> + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), >> + .size = sizeof(u64), >> + .align = sizeof(u64), >> + .get = pac_address_keys_get, >> + .set = pac_address_keys_set, >> + }, >> + [REGSET_PACG_KEYS] = { >> + .core_note_type = NT_ARM_PACG_KEYS, >> + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), >> + .size = sizeof(u64), >> + .align = sizeof(u64), >> + .get = pac_generic_keys_get, >> + .set = pac_generic_keys_set, >> + }, >> +#endif >> #endif >> }; Kristina _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-15 19:32 ` Kristina Martsenko @ 2019-01-16 15:13 ` Dave Martin 2019-01-19 23:21 ` Will Deacon 2019-01-22 19:08 ` Kristina Martsenko 0 siblings, 2 replies; 10+ messages in thread From: Dave Martin @ 2019-01-16 15:13 UTC (permalink / raw) To: Kristina Martsenko Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On Tue, Jan 15, 2019 at 07:32:30PM +0000, Kristina Martsenko wrote: > On 11/01/2019 13:58, Dave Martin wrote: > > On Thu, Jan 10, 2019 at 07:41:15PM +0000, Kristina Martsenko wrote: > >> On 10/01/2019 19:35, Kristina Martsenko wrote: > >>> Add two new ptrace regsets, which can be used to request and change the > >>> pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access > >>> to the instruction/data address keys, and NT_ARM_PACG_KEYS to the > >>> generic authentication key. The keys are also part of the core dump file > >>> of the process. > >>> > >>> The regsets are only exposed if the kernel is compiled with > >>> CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing > >>> and restoring processes that are using pointer authentication. Normally > >>> applications or debuggers should not need to know the keys (and exposing > >>> the keys is a security risk), so the regsets are not exposed by default. > > > > Although we can live with this, I still think it gives a false sense of > > safety. > > > > Can we come up with an scenario where an attacker with ptrace or > > coredump access can do more damage with access to the pointer auth keys > > than without? > > > > A lot of systems will run with CONFIG_CHECKPOINT_RESTORE=y (like > > packaged Debian kernels for example). And more paranoid systems already > > restrict or disable ptrace anyway. > > I can't think of such a scenario, as ptrace gives access to registers > and memory anyway. But I probably haven't thought of all scenarios. > > There are other ptrace options that are only available when > CONFIG_CHECKPOINT_RESTORE=y, such as PTRACE_SECCOMP_GET_FILTER. > > I think Will had a preference for having this depend on > CONFIG_CHECKPOINT_RESTORE, so I will keep it for now. OK, this is up to the maintainers to judge. The code works either way, and I'm not too fussed. My main concern is that legitimate potential debug uses such as analysing coredumps or running tasks to detect stack corruption may now fail to work on a random subset of systems. But that's hypothetical: I'm not aware that anyone is actually asking for this ability right now. We could relax the rules later on though, if people really want this. > > >> #define __ptrauth_key_install(k, v) \ > >> do { \ > >> - struct ptrauth_key __pki_v = (v); \ > >> - write_sysreg_s(__pki_v.lo, SYS_ ## k ## KEYLO_EL1); \ > >> - write_sysreg_s(__pki_v.hi, SYS_ ## k ## KEYHI_EL1); \ > >> + write_sysreg_s(v ## _lo, SYS_ ## k ## KEYLO_EL1); \ > >> + write_sysreg_s(v ## _hi, SYS_ ## k ## KEYHI_EL1); \ > >> } while (0) > >> > >> static inline void ptrauth_keys_switch(struct ptrauth_keys *keys) > >> { > >> if (system_supports_address_auth()) { > >> - __ptrauth_key_install(APIA, keys->apia); > >> - __ptrauth_key_install(APIB, keys->apib); > >> - __ptrauth_key_install(APDA, keys->apda); > >> - __ptrauth_key_install(APDB, keys->apdb); > >> + __ptrauth_key_install(APIA, keys->addr_keys.apiakey); > >> + __ptrauth_key_install(APIB, keys->addr_keys.apibkey); > >> + __ptrauth_key_install(APDA, keys->addr_keys.apdakey); > >> + __ptrauth_key_install(APDB, keys->addr_keys.apdbkey); > > > > Aren't the members of struct user_pac_address_keys split up into > > apiakey_lo, apiakey_hi etc.? > > They are, which is why the __ptrauth_key_install macro pastes a "_lo" or > "_hi" at the end of the field name. I don't think this is very nice, > which is one of the reasons why I prefer the other patch (where we keep > the kernel and ptrace structs separate). Ah, I got confused by the fact that __ptrauth_key_install() is a macro. Fair enough. > > However, I think there's no reason not to pair up the keys in nested > > structs in the user struct, so could we change that struct to be more > > like the old struct ptrauth_key and keep the above code? > > We don't currently have any other separate nested structs in the arm64 > ptrace userspace interface. Other architectures also seem to only have a > few rare instances of this. > > It seems odd to complicate the userspace interface because of a kernel > implementation detail, but if you think it's better I could change the > structs to: > > struct user_pac_key { > __u64 lo; > __u64 hi; > }; > > struct user_pac_address_keys { > struct user_pac_key apiakey; > struct user_pac_key apibkey; > struct user_pac_key apdakey; > struct user_pac_key apdbkey; > }; > > struct user_pac_generic_keys { > struct user_pac_key apgakey; > }; I don't have a strong opinion really. There's another option that occurred to me just now, and that's simply to represent each key as a single __uint128_t in the user regset view (with the _hi half strictly in the high 64 bits). This avoids questions about how to represent the halves of the key, and prevents userspace from accessing half-keys. How the half-keys map to system registers is an architectural quirk that userspace perhaps need not care about. If you like that approach, you could keep the kernel code as-is for now and just do the conversion explicitly in the ptrace accessors (as in your original patch). > >> } > >> > >> if (system_supports_generic_auth()) > >> - __ptrauth_key_install(APGA, keys->apga); > >> + __ptrauth_key_install(APGA, keys->gen_keys.apgakey); > >> } > >> > >> extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg); > >> @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > >> #define ptrauth_thread_init_user(tsk) \ > >> do { \ > >> struct task_struct *__ptiu_tsk = (tsk); \ > > > > Not added by this patch, but __ptiu_tsk doesn't seem to do anything > > except make the subsquent lines more verbose than otherwise (and pollute > > the identifier namespace -- though unlikely to be a problem). > > > > It may not be worth dropping it now that it's there though. > > Using __ptiu_tsk prevents the argument (tsk) from being evaluated twice, > which could have side effects. Ah, right. Actually, could this be a function instead? That would avoid multiple- evaulation a clean way. > > >> - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ > >> - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ > >> + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ > >> + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ > >> } while (0) > >> > >> #define ptrauth_thread_switch(tsk) \ > >> - ptrauth_keys_switch(&(tsk)->thread.keys_user) > >> + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) Similarly, can this be a function? (Technically tsk may be evaulated twice in this macro. Given the way these macros are used, I'm not sure that matters though.) > >> --- a/arch/arm64/include/uapi/asm/ptrace.h > >> +++ b/arch/arm64/include/uapi/asm/ptrace.h > >> @@ -233,6 +233,24 @@ struct user_pac_mask { > >> __u64 insn_mask; > >> }; > >> > >> +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ > >> + > >> +struct user_pac_address_keys { > >> + __u64 apiakey_lo; > >> + __u64 apiakey_hi; > >> + __u64 apibkey_lo; > >> + __u64 apibkey_hi; > >> + __u64 apdakey_lo; > >> + __u64 apdakey_hi; > >> + __u64 apdbkey_lo; > >> + __u64 apdbkey_hi; > >> +}; > >> + > >> +struct user_pac_generic_keys { > >> + __u64 apgakey_lo; > >> + __u64 apgakey_hi; > >> +}; > >> + > > > > As noted above, I think we could happily have a struct user_pac_key to > > pack up the halves of each key, like the old kernel struct. > > See my answer above. > > Also note that with this patch we have struct "user_pac_address_keys" in > struct ptrauth_keys, which may be confusing once we start using pointer > authentication in the kernel and use struct ptrauth_keys for kernel keys > as well, not just user keys. Not a big deal either way. The main thing to freeze now are the user ABI and the UAPI header. We can refactor the kernel's internal implementation later if we want. > > >> @@ -1074,6 +1132,24 @@ static const struct user_regset aarch64_regsets[] = { > >> .get = pac_mask_get, > >> /* this cannot be set dynamically */ > >> }, > >> +#ifdef CONFIG_CHECKPOINT_RESTORE > > > > && defined(CONFIG_ARM64_PTR_AUTH) ? > > No, this is already inside a larger "#ifdef CONFIG_ARM64_PTR_AUTH" block. Oh, right. That starts outside the hunks in the diff, so I missed it. > > >> + [REGSET_PACA_KEYS] = { > >> + .core_note_type = NT_ARM_PACA_KEYS, > >> + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), > >> + .size = sizeof(u64), > >> + .align = sizeof(u64), > >> + .get = pac_address_keys_get, > >> + .set = pac_address_keys_set, > >> + }, > >> + [REGSET_PACG_KEYS] = { > >> + .core_note_type = NT_ARM_PACG_KEYS, > >> + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), > >> + .size = sizeof(u64), > >> + .align = sizeof(u64), > >> + .get = pac_generic_keys_get, > >> + .set = pac_generic_keys_set, > >> + }, > >> +#endif > >> #endif Adding a comment on the second #endif to say which #if is ending here may be useful, but it's no big deal. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-16 15:13 ` Dave Martin @ 2019-01-19 23:21 ` Will Deacon 2019-01-22 19:07 ` Kristina Martsenko 2019-01-22 19:08 ` Kristina Martsenko 1 sibling, 1 reply; 10+ messages in thread From: Will Deacon @ 2019-01-19 23:21 UTC (permalink / raw) To: Dave Martin Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, linux-arm-kernel, Kristina Martsenko On Wed, Jan 16, 2019 at 03:13:03PM +0000, Dave Martin wrote: > On Tue, Jan 15, 2019 at 07:32:30PM +0000, Kristina Martsenko wrote: > > We don't currently have any other separate nested structs in the arm64 > > ptrace userspace interface. Other architectures also seem to only have a > > few rare instances of this. > > > > It seems odd to complicate the userspace interface because of a kernel > > implementation detail, but if you think it's better I could change the > > structs to: > > > > struct user_pac_key { > > __u64 lo; > > __u64 hi; > > }; > > > > struct user_pac_address_keys { > > struct user_pac_key apiakey; > > struct user_pac_key apibkey; > > struct user_pac_key apdakey; > > struct user_pac_key apdbkey; > > }; > > > > struct user_pac_generic_keys { > > struct user_pac_key apgakey; > > }; > > I don't have a strong opinion really. > > There's another option that occurred to me just now, and that's simply > to represent each key as a single __uint128_t in the user regset view > (with the _hi half strictly in the high 64 bits). > > This avoids questions about how to represent the halves of the key, > and prevents userspace from accessing half-keys. How the half-keys > map to system registers is an architectural quirk that userspace perhaps > need not care about. > > If you like that approach, you could keep the kernel code as-is for now > and just do the conversion explicitly in the ptrace accessors (as in > your original patch). I think that would be worth persuing. We already have __uint128_t types in our uapi/ headers, so we're not introducing anything new by using them here. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-19 23:21 ` Will Deacon @ 2019-01-22 19:07 ` Kristina Martsenko 0 siblings, 0 replies; 10+ messages in thread From: Kristina Martsenko @ 2019-01-22 19:07 UTC (permalink / raw) To: Will Deacon, Dave Martin Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, linux-arm-kernel On 19/01/2019 23:21, Will Deacon wrote: > On Wed, Jan 16, 2019 at 03:13:03PM +0000, Dave Martin wrote: >> On Tue, Jan 15, 2019 at 07:32:30PM +0000, Kristina Martsenko wrote: >>> We don't currently have any other separate nested structs in the arm64 >>> ptrace userspace interface. Other architectures also seem to only have a >>> few rare instances of this. >>> >>> It seems odd to complicate the userspace interface because of a kernel >>> implementation detail, but if you think it's better I could change the >>> structs to: >>> >>> struct user_pac_key { >>> __u64 lo; >>> __u64 hi; >>> }; >>> >>> struct user_pac_address_keys { >>> struct user_pac_key apiakey; >>> struct user_pac_key apibkey; >>> struct user_pac_key apdakey; >>> struct user_pac_key apdbkey; >>> }; >>> >>> struct user_pac_generic_keys { >>> struct user_pac_key apgakey; >>> }; >> >> I don't have a strong opinion really. >> >> There's another option that occurred to me just now, and that's simply >> to represent each key as a single __uint128_t in the user regset view >> (with the _hi half strictly in the high 64 bits). >> >> This avoids questions about how to represent the halves of the key, >> and prevents userspace from accessing half-keys. How the half-keys >> map to system registers is an architectural quirk that userspace perhaps >> need not care about. >> >> If you like that approach, you could keep the kernel code as-is for now >> and just do the conversion explicitly in the ptrace accessors (as in >> your original patch). > > I think that would be worth persuing. We already have __uint128_t types in > our uapi/ headers, so we're not introducing anything new by using them here. Ok, I'll send v3 with __uint128_t keys. Thanks, Kristina _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-16 15:13 ` Dave Martin 2019-01-19 23:21 ` Will Deacon @ 2019-01-22 19:08 ` Kristina Martsenko 2019-01-23 11:23 ` Dave Martin 1 sibling, 1 reply; 10+ messages in thread From: Kristina Martsenko @ 2019-01-22 19:08 UTC (permalink / raw) To: Dave Martin Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On 16/01/2019 15:13, Dave Martin wrote: > On Tue, Jan 15, 2019 at 07:32:30PM +0000, Kristina Martsenko wrote: >> On 11/01/2019 13:58, Dave Martin wrote: >>> On Thu, Jan 10, 2019 at 07:41:15PM +0000, Kristina Martsenko wrote: >>>> On 10/01/2019 19:35, Kristina Martsenko wrote: >>>> @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) >>>> #define ptrauth_thread_init_user(tsk) \ >>>> do { \ >>>> struct task_struct *__ptiu_tsk = (tsk); \ >>> >>> Not added by this patch, but __ptiu_tsk doesn't seem to do anything >>> except make the subsquent lines more verbose than otherwise (and pollute >>> the identifier namespace -- though unlikely to be a problem). >>> >>> It may not be worth dropping it now that it's there though. >> >> Using __ptiu_tsk prevents the argument (tsk) from being evaluated twice, >> which could have side effects. > > Ah, right. > > Actually, could this be a function instead? That would avoid multiple- > evaulation a clean way. If it were a function, then this file (pointer_auth.h) would need to #include <linux/sched.h> (for the struct task_struct definition), which would create a circular header dependency, since sched.h #includes asm/processor.h, which #includes pointer_auth.h. Alternatively the function could be moved to pointer_auth.c, but that would prevent it from being inlined, so I'd prefer to keep it as a macro for now. >>>> - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ >>>> - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ >>>> + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ >>>> + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ >>>> } while (0) >>>> >>>> #define ptrauth_thread_switch(tsk) \ >>>> - ptrauth_keys_switch(&(tsk)->thread.keys_user) >>>> + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) > > Similarly, can this be a function? Same reasons as above. > (Technically tsk may be evaulated twice in this macro. Given the way > these macros are used, I'm not sure that matters though.) tsk is only used once here, so I think it's only evaluated once. >> Also note that with this patch we have struct "user_pac_address_keys" in >> struct ptrauth_keys, which may be confusing once we start using pointer >> authentication in the kernel and use struct ptrauth_keys for kernel keys >> as well, not just user keys. > > Not a big deal either way. > > The main thing to freeze now are the user ABI and the UAPI header. We > can refactor the kernel's internal implementation later if we want. Ok. Kristina _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-22 19:08 ` Kristina Martsenko @ 2019-01-23 11:23 ` Dave Martin 0 siblings, 0 replies; 10+ messages in thread From: Dave Martin @ 2019-01-23 11:23 UTC (permalink / raw) To: Kristina Martsenko Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On Tue, Jan 22, 2019 at 07:08:25PM +0000, Kristina Martsenko wrote: > On 16/01/2019 15:13, Dave Martin wrote: > > On Tue, Jan 15, 2019 at 07:32:30PM +0000, Kristina Martsenko wrote: > >> On 11/01/2019 13:58, Dave Martin wrote: > >>> On Thu, Jan 10, 2019 at 07:41:15PM +0000, Kristina Martsenko wrote: > >>>> On 10/01/2019 19:35, Kristina Martsenko wrote: > >>>> @@ -80,12 +65,12 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > >>>> #define ptrauth_thread_init_user(tsk) \ > >>>> do { \ > >>>> struct task_struct *__ptiu_tsk = (tsk); \ > >>> > >>> Not added by this patch, but __ptiu_tsk doesn't seem to do anything > >>> except make the subsquent lines more verbose than otherwise (and pollute > >>> the identifier namespace -- though unlikely to be a problem). > >>> > >>> It may not be worth dropping it now that it's there though. > >> > >> Using __ptiu_tsk prevents the argument (tsk) from being evaluated twice, > >> which could have side effects. > > > > Ah, right. > > > > Actually, could this be a function instead? That would avoid multiple- > > evaulation a clean way. > > If it were a function, then this file (pointer_auth.h) would need to > #include <linux/sched.h> (for the struct task_struct definition), which > would create a circular header dependency, since sched.h #includes > asm/processor.h, which #includes pointer_auth.h. Alternatively the > function could be moved to pointer_auth.c, but that would prevent it > from being inlined, so I'd prefer to keep it as a macro for now. Ah, right -- I hit that before in a couple of places. So long as there's a reason for these things being macros, I see no problem. > > >>>> - ptrauth_keys_init(&__ptiu_tsk->thread.keys_user); \ > >>>> - ptrauth_keys_switch(&__ptiu_tsk->thread.keys_user); \ > >>>> + ptrauth_keys_init(&__ptiu_tsk->thread.uw.keys_user); \ > >>>> + ptrauth_keys_switch(&__ptiu_tsk->thread.uw.keys_user); \ > >>>> } while (0) > >>>> > >>>> #define ptrauth_thread_switch(tsk) \ > >>>> - ptrauth_keys_switch(&(tsk)->thread.keys_user) > >>>> + ptrauth_keys_switch(&(tsk)->thread.uw.keys_user) > > > > Similarly, can this be a function? > > Same reasons as above. > > > (Technically tsk may be evaulated twice in this macro. Given the way > > these macros are used, I'm not sure that matters though.) > > tsk is only used once here, so I think it's only evaluated once. You're right ... I misread the changed line in the diff for two added lines (doh). Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] arm64: add ptrace regsets for ptrauth key management 2019-01-10 19:35 [PATCH v2] arm64: add ptrace regsets for ptrauth key management Kristina Martsenko 2019-01-10 19:41 ` Kristina Martsenko @ 2019-01-11 13:31 ` Dave Martin 1 sibling, 0 replies; 10+ messages in thread From: Dave Martin @ 2019-01-11 13:31 UTC (permalink / raw) To: Kristina Martsenko Cc: Mark Rutland, Catalin Marinas, Amit Kachhap, Will Deacon, linux-arm-kernel On Thu, Jan 10, 2019 at 07:35:08PM +0000, Kristina Martsenko wrote: > Add two new ptrace regsets, which can be used to request and change the > pointer authentication keys of a thread. NT_ARM_PACA_KEYS gives access > to the instruction/data address keys, and NT_ARM_PACG_KEYS to the > generic authentication key. The keys are also part of the core dump file > of the process. > > The regsets are only exposed if the kernel is compiled with > CONFIG_CHECKPOINT_RESTORE=y, as the intended use case is checkpointing > and restoring processes that are using pointer authentication. Normally > applications or debuggers should not need to know the keys (and exposing > the keys is a security risk), so the regsets are not exposed by default. I think the second variant of the patch is the way to go with this, since it seems to result in simpler code. So, I'll comment on the second patch. Cheers ---Dave > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > --- > > The previous version of this patch was: > https://lore.kernel.org/lkml/20181207183931.4285-12-kristina.martsenko@arm.com/ > > Changes in v2: > - Convert each field individually between ptrauth_keys and > user_pac_address_keys/user_pac_generic_keys > > Documentation/arm64/pointer-authentication.txt | 5 + > arch/arm64/include/uapi/asm/ptrace.h | 18 ++++ > arch/arm64/kernel/ptrace.c | 144 +++++++++++++++++++++++++ > include/uapi/linux/elf.h | 2 + > 4 files changed, 169 insertions(+) > > diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt > index a25cd21290e9..5baca42ba146 100644 > --- a/Documentation/arm64/pointer-authentication.txt > +++ b/Documentation/arm64/pointer-authentication.txt > @@ -78,6 +78,11 @@ bits can vary between the two. Note that the masks apply to TTBR0 > addresses, and are not valid to apply to TTBR1 addresses (e.g. kernel > pointers). > > +Additionally, when CONFIG_CHECKPOINT_RESTORE is also set, the kernel > +will expose the NT_ARM_PACA_KEYS and NT_ARM_PACG_KEYS regsets (struct > +user_pac_address_keys and struct user_pac_generic_keys). These can be > +used to get and set the keys for a thread. > + > > Virtualization > -------------- > diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h > index 28d77c9ed531..0affa43602a5 100644 > --- a/arch/arm64/include/uapi/asm/ptrace.h > +++ b/arch/arm64/include/uapi/asm/ptrace.h > @@ -233,6 +233,24 @@ struct user_pac_mask { > __u64 insn_mask; > }; > > +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ > + > +struct user_pac_address_keys { > + __u64 apiakey_lo; > + __u64 apiakey_hi; > + __u64 apibkey_lo; > + __u64 apibkey_hi; > + __u64 apdakey_lo; > + __u64 apdakey_hi; > + __u64 apdbkey_lo; > + __u64 apdbkey_hi; > +}; > + > +struct user_pac_generic_keys { > + __u64 apgakey_lo; > + __u64 apgakey_hi; > +}; > + > #endif /* __ASSEMBLY__ */ > > #endif /* _UAPI__ASM_PTRACE_H */ > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index 9dce33b0e260..a5f9f07e9eac 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -979,6 +979,128 @@ static int pac_mask_get(struct task_struct *target, > > return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &uregs, 0, -1); > } > + > +#ifdef CONFIG_CHECKPOINT_RESTORE > +static void pac_address_keys_to_user(struct user_pac_address_keys *ukeys, > + struct ptrauth_keys *keys) > +{ > + ukeys->apiakey_lo = keys->apia.lo; > + ukeys->apiakey_hi = keys->apia.hi; > + ukeys->apibkey_lo = keys->apib.lo; > + ukeys->apibkey_hi = keys->apib.hi; > + ukeys->apdakey_lo = keys->apda.lo; > + ukeys->apdakey_hi = keys->apda.hi; > + ukeys->apdbkey_lo = keys->apdb.lo; > + ukeys->apdbkey_hi = keys->apdb.hi; > +} > + > +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, > + struct ptrauth_keys *keys) > +{ > + keys->apia.lo = ukeys->apiakey_lo; > + keys->apia.hi = ukeys->apiakey_hi; > + keys->apib.lo = ukeys->apibkey_lo; > + keys->apib.hi = ukeys->apibkey_hi; > + keys->apda.lo = ukeys->apdakey_lo; > + keys->apda.hi = ukeys->apdakey_hi; > + keys->apdb.lo = ukeys->apdbkey_lo; > + keys->apdb.hi = ukeys->apdbkey_hi; > +} > + > +static int pac_address_keys_get(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + void *kbuf, void __user *ubuf) > +{ > + struct ptrauth_keys *keys = &target->thread.keys_user; > + struct user_pac_address_keys user_keys; > + > + if (!system_supports_address_auth()) > + return -EINVAL; > + > + memset(&user_keys, 0, sizeof(user_keys)); > + pac_address_keys_to_user(&user_keys, keys); > + > + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, > + &user_keys, 0, -1); > +} > + > +static int pac_address_keys_set(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, const void __user *ubuf) > +{ > + struct ptrauth_keys *keys = &target->thread.keys_user; > + struct user_pac_address_keys user_keys; > + int ret; > + > + if (!system_supports_address_auth()) > + return -EINVAL; > + > + pac_address_keys_to_user(&user_keys, keys); > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + &user_keys, 0, -1); > + if (ret) > + return ret; > + pac_address_keys_from_user(&user_keys, keys); > + > + return 0; > +} > + > +static void pac_generic_keys_to_user(struct user_pac_generic_keys *ukeys, > + struct ptrauth_keys *keys) > +{ > + ukeys->apgakey_lo = keys->apga.lo; > + ukeys->apgakey_hi = keys->apga.hi; > +} > + > +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, > + struct ptrauth_keys *keys) > +{ > + keys->apga.lo = ukeys->apgakey_lo; > + keys->apga.hi = ukeys->apgakey_hi; > +} > + > +static int pac_generic_keys_get(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + void *kbuf, void __user *ubuf) > +{ > + struct ptrauth_keys *keys = &target->thread.keys_user; > + struct user_pac_generic_keys user_keys; > + > + if (!system_supports_generic_auth()) > + return -EINVAL; > + > + memset(&user_keys, 0, sizeof(user_keys)); > + pac_generic_keys_to_user(&user_keys, keys); > + > + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, > + &user_keys, 0, -1); > +} > + > +static int pac_generic_keys_set(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, const void __user *ubuf) > +{ > + struct ptrauth_keys *keys = &target->thread.keys_user; > + struct user_pac_generic_keys user_keys; > + int ret; > + > + if (!system_supports_generic_auth()) > + return -EINVAL; > + > + pac_generic_keys_to_user(&user_keys, keys); > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, > + &user_keys, 0, -1); > + if (ret) > + return ret; > + pac_generic_keys_from_user(&user_keys, keys); > + > + return 0; > +} > +#endif /* CONFIG_CHECKPOINT_RESTORE */ > #endif /* CONFIG_ARM64_PTR_AUTH */ > > enum aarch64_regset { > @@ -995,6 +1117,10 @@ enum aarch64_regset { > #endif > #ifdef CONFIG_ARM64_PTR_AUTH > REGSET_PAC_MASK, > +#ifdef CONFIG_CHECKPOINT_RESTORE > + REGSET_PACA_KEYS, > + REGSET_PACG_KEYS, > +#endif > #endif > }; > > @@ -1074,6 +1200,24 @@ static const struct user_regset aarch64_regsets[] = { > .get = pac_mask_get, > /* this cannot be set dynamically */ > }, > +#ifdef CONFIG_CHECKPOINT_RESTORE > + [REGSET_PACA_KEYS] = { > + .core_note_type = NT_ARM_PACA_KEYS, > + .n = sizeof(struct user_pac_address_keys) / sizeof(u64), > + .size = sizeof(u64), > + .align = sizeof(u64), > + .get = pac_address_keys_get, > + .set = pac_address_keys_set, > + }, > + [REGSET_PACG_KEYS] = { > + .core_note_type = NT_ARM_PACG_KEYS, > + .n = sizeof(struct user_pac_generic_keys) / sizeof(u64), > + .size = sizeof(u64), > + .align = sizeof(u64), > + .get = pac_generic_keys_get, > + .set = pac_generic_keys_set, > + }, > +#endif > #endif > }; > > diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h > index e4d6ddd93567..34c02e4290fe 100644 > --- a/include/uapi/linux/elf.h > +++ b/include/uapi/linux/elf.h > @@ -421,6 +421,8 @@ typedef struct elf64_shdr { > #define NT_ARM_SYSTEM_CALL 0x404 /* ARM system call number */ > #define NT_ARM_SVE 0x405 /* ARM Scalable Vector Extension registers */ > #define NT_ARM_PAC_MASK 0x406 /* ARM pointer authentication code masks */ > +#define NT_ARM_PACA_KEYS 0x407 /* ARM pointer authentication address keys */ > +#define NT_ARM_PACG_KEYS 0x408 /* ARM pointer authentication generic key */ > #define NT_ARC_V2 0x600 /* ARCv2 accumulator/extra registers */ > #define NT_VMCOREDD 0x700 /* Vmcore Device Dump Note */ > #define NT_MIPS_DSP 0x800 /* MIPS DSP ASE registers */ > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-23 11:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-10 19:35 [PATCH v2] arm64: add ptrace regsets for ptrauth key management Kristina Martsenko 2019-01-10 19:41 ` Kristina Martsenko 2019-01-11 13:58 ` Dave Martin 2019-01-15 19:32 ` Kristina Martsenko 2019-01-16 15:13 ` Dave Martin 2019-01-19 23:21 ` Will Deacon 2019-01-22 19:07 ` Kristina Martsenko 2019-01-22 19:08 ` Kristina Martsenko 2019-01-23 11:23 ` Dave Martin 2019-01-11 13:31 ` Dave Martin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).