| Message ID | 20190123153519.7874-1-kristina.martsenko@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] arm64: add ptrace regsets for ptrauth key management | expand |
On Wed, Jan 23, 2019 at 03:35:19PM +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. Is there a use case for including these in the coredump? Perhaps it would make sense to suppress them. If the core code doesn't make that easy it may not be worth it though. Including the keys in a coredump doesn't feel like a security risk, unless the checkpoint restore folks have a way of resurrecting dead processes from coredumps. Otherwise, (statistically) no new process will have the same keys anyway. > The regsets are only exposed if the kernel is compiled with > CONFIG_CHECKPOINT_RESTORE=y, as the only intended use case is > checkpointing and restoring processes that are using pointer > authentication. (This can be changed later if there are other use > cases.) > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> I think this looks nicer overall. A few comments, which are all nits except for the questions about the memset() calls. > --- > > Changes in v3: > - 128-bit keys in uapi struct, conversions to/from ptrauth_keys fields > - Commit message tweaks > > Documentation/arm64/pointer-authentication.txt | 5 + > arch/arm64/include/uapi/asm/ptrace.h | 13 +++ > arch/arm64/kernel/ptrace.c | 139 +++++++++++++++++++++++++ > include/uapi/linux/elf.h | 2 + > 4 files changed, 159 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..d78623acb649 100644 > --- a/arch/arm64/include/uapi/asm/ptrace.h > +++ b/arch/arm64/include/uapi/asm/ptrace.h > @@ -233,6 +233,19 @@ struct user_pac_mask { > __u64 insn_mask; > }; > > +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ > + > +struct user_pac_address_keys { > + __uint128_t apiakey; > + __uint128_t apibkey; > + __uint128_t apdakey; > + __uint128_t apdbkey; > +}; > + > +struct user_pac_generic_keys { > + __uint128_t apgakey; > +}; > + > #endif /* __ASSEMBLY__ */ > > #endif /* _UAPI__ASM_PTRACE_H */ > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > index 9dce33b0e260..102f71fe0288 100644 > --- a/arch/arm64/kernel/ptrace.c > +++ b/arch/arm64/kernel/ptrace.c > @@ -979,6 +979,123 @@ 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) const? > +{ > + ukeys->apiakey = ((__uint128_t)keys->apia.hi << 64) | keys->apia.lo; We could have a helper static __uint128_t key_to_user(struct ptrauth_key const *key) { return (__uint128_t)key->hi << 64 | key->lo; } and then do ukeys->apiakey = key_to_user(&keys->apia); ukeys->apibkey = key_to_user(&keys->apib); > + ukeys->apibkey = ((__uint128_t)keys->apib.hi << 64) | keys->apib.lo; > + ukeys->apdakey = ((__uint128_t)keys->apda.hi << 64) | keys->apda.lo; > + ukeys->apdbkey = ((__uint128_t)keys->apdb.hi << 64) | keys->apdb.lo; > +} > + > +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, const? > + struct ptrauth_keys *keys) Is the argument order for this function intentional? It means that for pac_address_keys_to_user() the destination argument is on the left, but in pac_address_keys_from_user() the destination argument is on the right. This is mildly confusing, but because the arguments are of different types the compiler should catch mistakes (especially if the const is added appropriately.) If this is following a pattern already set elsewhere, then fair enough. > +{ > + keys->apia.lo = (unsigned long)ukeys->apiakey; > + keys->apia.hi = (unsigned long)(ukeys->apiakey >> 64); Similarly, we could have static struct ptrauth_key key_from_user(__uint128_t ukey) { struct ptrauth_key key = { .lo = (unsigned long)ukey; .hi = (unsigned long)(ukey >> 64); }; return key; } keys->apia = key_from_user(ukeys->apiakey); keys->apib = key_from_user(ukeys->apibkey); These factorings may be overkill though. > + keys->apib.lo = (unsigned long)ukeys->apibkey; > + keys->apib.hi = (unsigned long)(ukeys->apibkey >> 64); > + keys->apda.lo = (unsigned long)ukeys->apdakey; > + keys->apda.hi = (unsigned long)(ukeys->apdakey >> 64); > + keys->apdb.lo = (unsigned long)ukeys->apdbkey; > + keys->apdb.hi = (unsigned long)(ukeys->apdbkey >> 64); > +} > + > +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)); Why this memset? (In particular, we have no similar memset in pac_address_keys_get(). Maybe it's needed for some reason I've missed, but I don't see it.) > + 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) const? > +{ > + ukeys->apgakey = ((__uint128_t)keys->apga.hi << 64) | keys->apga.lo; > +} > + > +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, const? > + struct ptrauth_keys *keys) > +{ > + keys->apga.lo = (unsigned long)ukeys->apgakey; > + keys->apga.hi = (unsigned long)(ukeys->apgakey >> 64); (The above helpers could be used here too.) > +} > + > +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)); Similarly to above, is this needed? > + 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; > +} [...] Cheers ---Dave
On 23/01/2019 17:37, Dave Martin wrote: > On Wed, Jan 23, 2019 at 03:35:19PM +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. > > Is there a use case for including these in the coredump? Perhaps it > would make sense to suppress them. If the core code doesn't make that > easy it may not be worth it though. From what I can see there is no proper way to exclude the keys from the core dump. There is an ->active() callback on struct user_regset, which could be implemented to always return 0, which would exclude the keys from the core dump. But while the callback seems to currently only be used for the core dump, its purpose seems to be more general, so I don't think it would be right to have it always return 0. > Including the keys in a coredump doesn't feel like a security risk, > unless the checkpoint restore folks have a way of resurrecting dead > processes from coredumps. Otherwise, (statistically) no new process > will have the same keys anyway. > >> The regsets are only exposed if the kernel is compiled with >> CONFIG_CHECKPOINT_RESTORE=y, as the only intended use case is >> checkpointing and restoring processes that are using pointer >> authentication. (This can be changed later if there are other use >> cases.) >> >> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > > I think this looks nicer overall. > > A few comments, which are all nits except for the questions about the > memset() calls. > >> --- >> >> Changes in v3: >> - 128-bit keys in uapi struct, conversions to/from ptrauth_keys fields >> - Commit message tweaks >> >> Documentation/arm64/pointer-authentication.txt | 5 + >> arch/arm64/include/uapi/asm/ptrace.h | 13 +++ >> arch/arm64/kernel/ptrace.c | 139 +++++++++++++++++++++++++ >> include/uapi/linux/elf.h | 2 + >> 4 files changed, 159 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..d78623acb649 100644 >> --- a/arch/arm64/include/uapi/asm/ptrace.h >> +++ b/arch/arm64/include/uapi/asm/ptrace.h >> @@ -233,6 +233,19 @@ struct user_pac_mask { >> __u64 insn_mask; >> }; >> >> +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ >> + >> +struct user_pac_address_keys { >> + __uint128_t apiakey; >> + __uint128_t apibkey; >> + __uint128_t apdakey; >> + __uint128_t apdbkey; >> +}; >> + >> +struct user_pac_generic_keys { >> + __uint128_t apgakey; >> +}; >> + >> #endif /* __ASSEMBLY__ */ >> >> #endif /* _UAPI__ASM_PTRACE_H */ >> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c >> index 9dce33b0e260..102f71fe0288 100644 >> --- a/arch/arm64/kernel/ptrace.c >> +++ b/arch/arm64/kernel/ptrace.c >> @@ -979,6 +979,123 @@ 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) > > const? Done. >> +{ >> + ukeys->apiakey = ((__uint128_t)keys->apia.hi << 64) | keys->apia.lo; > > We could have a helper > > static __uint128_t key_to_user(struct ptrauth_key const *key) > { > return (__uint128_t)key->hi << 64 | key->lo; > } > > and then do > > ukeys->apiakey = key_to_user(&keys->apia); > ukeys->apibkey = key_to_user(&keys->apib); Done. >> + ukeys->apibkey = ((__uint128_t)keys->apib.hi << 64) | keys->apib.lo; >> + ukeys->apdakey = ((__uint128_t)keys->apda.hi << 64) | keys->apda.lo; >> + ukeys->apdbkey = ((__uint128_t)keys->apdb.hi << 64) | keys->apdb.lo; >> +} >> + >> +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, > > const? Done. >> + struct ptrauth_keys *keys) > > Is the argument order for this function intentional? > > It means that for pac_address_keys_to_user() the destination argument is > on the left, but in pac_address_keys_from_user() the destination > argument is on the right. > > This is mildly confusing, but because the arguments are of different > types the compiler should catch mistakes (especially if the const is > added appropriately.) > > If this is following a pattern already set elsewhere, then fair enough. I think the order is accidental here. I'll move the destination argument to the left. >> +{ >> + keys->apia.lo = (unsigned long)ukeys->apiakey; >> + keys->apia.hi = (unsigned long)(ukeys->apiakey >> 64); > > Similarly, we could have > > static struct ptrauth_key key_from_user(__uint128_t ukey) > { > struct ptrauth_key key = { > .lo = (unsigned long)ukey; > .hi = (unsigned long)(ukey >> 64); > }; > > return key; > } > > keys->apia = key_from_user(ukeys->apiakey); > keys->apib = key_from_user(ukeys->apibkey); > > These factorings may be overkill though. Done. >> + keys->apib.lo = (unsigned long)ukeys->apibkey; >> + keys->apib.hi = (unsigned long)(ukeys->apibkey >> 64); >> + keys->apda.lo = (unsigned long)ukeys->apdakey; >> + keys->apda.hi = (unsigned long)(ukeys->apdakey >> 64); >> + keys->apdb.lo = (unsigned long)ukeys->apdbkey; >> + keys->apdb.hi = (unsigned long)(ukeys->apdbkey >> 64); >> +} >> + >> +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)); > > Why this memset? From your previous email [1], I understood that you wanted the user struct zeroed to avoid leaking kernel stack to userspace through padding bits, even if the keys are 128-bit (or 64-bit) and there shouldn't be any padding bits anyway. I don't personally think that the struct needs to be zeroed (since there shouldn't be any padding), so if it's alright with you I'll remove the memset. [1] https://lore.kernel.org/lkml/20181212152347.GY3505@e103592.cambridge.arm.com/ > (In particular, we have no similar memset in pac_address_keys_get(). I guess you mean pac_address_keys_set. We only copy the fields into the kernel struct there, not the padding, so this issue doesn't apply. > Maybe it's needed for some reason I've missed, but I don't see it.) > >> + 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) > > const? Done. >> +{ >> + ukeys->apgakey = ((__uint128_t)keys->apga.hi << 64) | keys->apga.lo; >> +} >> + >> +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, > > const? Done. >> + struct ptrauth_keys *keys) >> +{ >> + keys->apga.lo = (unsigned long)ukeys->apgakey; >> + keys->apga.hi = (unsigned long)(ukeys->apgakey >> 64); > > (The above helpers could be used here too.) > >> +} >> + >> +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)); > > Similarly to above, is this needed? Same reason as above; I'll remove it. Kristina >> + 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; >> +}
On Tue, Jan 29, 2019 at 06:44:38PM +0000, Kristina Martsenko wrote: > On 23/01/2019 17:37, Dave Martin wrote: > > On Wed, Jan 23, 2019 at 03:35:19PM +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. > > > > Is there a use case for including these in the coredump? Perhaps it > > would make sense to suppress them. If the core code doesn't make that > > easy it may not be worth it though. > > From what I can see there is no proper way to exclude the keys from the > core dump. There is an ->active() callback on struct user_regset, which > could be implemented to always return 0, which would exclude the keys > from the core dump. But while the callback seems to currently only be > used for the core dump, its purpose seems to be more general, so I don't > think it would be right to have it always return 0. OK, if there's no straightforward answer here, I suggest we leave it as-is, especially if nothing else handles this specially. [...] > >> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c > >> index 9dce33b0e260..102f71fe0288 100644 > >> --- a/arch/arm64/kernel/ptrace.c > >> +++ b/arch/arm64/kernel/ptrace.c > >> @@ -979,6 +979,123 @@ 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) > > > > const? > > Done. > > >> +{ > >> + ukeys->apiakey = ((__uint128_t)keys->apia.hi << 64) | keys->apia.lo; > > > > We could have a helper > > > > static __uint128_t key_to_user(struct ptrauth_key const *key) > > { > > return (__uint128_t)key->hi << 64 | key->lo; > > } > > > > and then do > > > > ukeys->apiakey = key_to_user(&keys->apia); > > ukeys->apibkey = key_to_user(&keys->apib); > > Done. > > >> + ukeys->apibkey = ((__uint128_t)keys->apib.hi << 64) | keys->apib.lo; > >> + ukeys->apdakey = ((__uint128_t)keys->apda.hi << 64) | keys->apda.lo; > >> + ukeys->apdbkey = ((__uint128_t)keys->apdb.hi << 64) | keys->apdb.lo; > >> +} > >> + > >> +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, > > > > const? > > Done. > > >> + struct ptrauth_keys *keys) > > > > Is the argument order for this function intentional? > > > > It means that for pac_address_keys_to_user() the destination argument is > > on the left, but in pac_address_keys_from_user() the destination > > argument is on the right. > > > > This is mildly confusing, but because the arguments are of different > > types the compiler should catch mistakes (especially if the const is > > added appropriately.) > > > > If this is following a pattern already set elsewhere, then fair enough. > > I think the order is accidental here. I'll move the destination argument > to the left. OK, that sounds fine. > >> +{ > >> + keys->apia.lo = (unsigned long)ukeys->apiakey; > >> + keys->apia.hi = (unsigned long)(ukeys->apiakey >> 64); > > > > Similarly, we could have > > > > static struct ptrauth_key key_from_user(__uint128_t ukey) > > { > > struct ptrauth_key key = { > > .lo = (unsigned long)ukey; > > .hi = (unsigned long)(ukey >> 64); > > }; > > > > return key; > > } > > > > keys->apia = key_from_user(ukeys->apiakey); > > keys->apib = key_from_user(ukeys->apibkey); > > > > These factorings may be overkill though. > > Done. OK, fair enough. > >> + keys->apib.lo = (unsigned long)ukeys->apibkey; > >> + keys->apib.hi = (unsigned long)(ukeys->apibkey >> 64); > >> + keys->apda.lo = (unsigned long)ukeys->apdakey; > >> + keys->apda.hi = (unsigned long)(ukeys->apdakey >> 64); > >> + keys->apdb.lo = (unsigned long)ukeys->apdbkey; > >> + keys->apdb.hi = (unsigned long)(ukeys->apdbkey >> 64); > >> +} > >> + > >> +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)); > > > > Why this memset? > > From your previous email [1], I understood that you wanted the user > struct zeroed to avoid leaking kernel stack to userspace through padding > bits, even if the keys are 128-bit (or 64-bit) and there shouldn't be > any padding bits anyway. I don't personally think that the struct needs > to be zeroed (since there shouldn't be any padding), so if it's alright > with you I'll remove the memset. > > [1] https://lore.kernel.org/lkml/20181212152347.GY3505@e103592.cambridge.arm.com/ > > > (In particular, we have no similar memset in pac_address_keys_get(). > > I guess you mean pac_address_keys_set. We only copy the fields into the Ah yes, sorry. > kernel struct there, not the padding, so this issue doesn't apply. I could have explained myself better. C allows padding to be populated with arbitrary data any time a member of the struct is written, so there's really no option but to ensure that the struct contains no padding at all. Since the affected structs are homogeneous we should be OK here, and explicitly computing and assiging appropriate values rather than doing a straight copy minimises the possiblility for unexpected things happening, as does forbidding half-keys from being modified by userspace. So I agree, the memset() doesn't look like it's adding anything and it can go. > > Maybe it's needed for some reason I've missed, but I don't see it.) > > > >> + 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) > > > > const? > > Done. > > >> +{ > >> + ukeys->apgakey = ((__uint128_t)keys->apga.hi << 64) | keys->apga.lo; > >> +} > >> + > >> +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, > > > > const? > > Done. > > >> + struct ptrauth_keys *keys) > >> +{ > >> + keys->apga.lo = (unsigned long)ukeys->apgakey; > >> + keys->apga.hi = (unsigned long)(ukeys->apgakey >> 64); > > > > (The above helpers could be used here too.) > > > >> +} > >> + > >> +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)); > > > > Similarly to above, is this needed? > > Same reason as above; I'll remove it. > > Kristina [...] OK, thanks ---Dave
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..d78623acb649 100644 --- a/arch/arm64/include/uapi/asm/ptrace.h +++ b/arch/arm64/include/uapi/asm/ptrace.h @@ -233,6 +233,19 @@ struct user_pac_mask { __u64 insn_mask; }; +/* pointer authentication keys (NT_ARM_PACA_KEYS, NT_ARM_PACG_KEYS) */ + +struct user_pac_address_keys { + __uint128_t apiakey; + __uint128_t apibkey; + __uint128_t apdakey; + __uint128_t apdbkey; +}; + +struct user_pac_generic_keys { + __uint128_t apgakey; +}; + #endif /* __ASSEMBLY__ */ #endif /* _UAPI__ASM_PTRACE_H */ diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 9dce33b0e260..102f71fe0288 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -979,6 +979,123 @@ 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 = ((__uint128_t)keys->apia.hi << 64) | keys->apia.lo; + ukeys->apibkey = ((__uint128_t)keys->apib.hi << 64) | keys->apib.lo; + ukeys->apdakey = ((__uint128_t)keys->apda.hi << 64) | keys->apda.lo; + ukeys->apdbkey = ((__uint128_t)keys->apdb.hi << 64) | keys->apdb.lo; +} + +static void pac_address_keys_from_user(struct user_pac_address_keys *ukeys, + struct ptrauth_keys *keys) +{ + keys->apia.lo = (unsigned long)ukeys->apiakey; + keys->apia.hi = (unsigned long)(ukeys->apiakey >> 64); + keys->apib.lo = (unsigned long)ukeys->apibkey; + keys->apib.hi = (unsigned long)(ukeys->apibkey >> 64); + keys->apda.lo = (unsigned long)ukeys->apdakey; + keys->apda.hi = (unsigned long)(ukeys->apdakey >> 64); + keys->apdb.lo = (unsigned long)ukeys->apdbkey; + keys->apdb.hi = (unsigned long)(ukeys->apdbkey >> 64); +} + +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 = ((__uint128_t)keys->apga.hi << 64) | keys->apga.lo; +} + +static void pac_generic_keys_from_user(struct user_pac_generic_keys *ukeys, + struct ptrauth_keys *keys) +{ + keys->apga.lo = (unsigned long)ukeys->apgakey; + keys->apga.hi = (unsigned long)(ukeys->apgakey >> 64); +} + +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 +1112,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 +1195,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(__uint128_t), + .size = sizeof(__uint128_t), + .align = sizeof(__uint128_t), + .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(__uint128_t), + .size = sizeof(__uint128_t), + .align = sizeof(__uint128_t), + .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 */
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 only intended use case is checkpointing and restoring processes that are using pointer authentication. (This can be changed later if there are other use cases.) Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> --- Changes in v3: - 128-bit keys in uapi struct, conversions to/from ptrauth_keys fields - Commit message tweaks Documentation/arm64/pointer-authentication.txt | 5 + arch/arm64/include/uapi/asm/ptrace.h | 13 +++ arch/arm64/kernel/ptrace.c | 139 +++++++++++++++++++++++++ include/uapi/linux/elf.h | 2 + 4 files changed, 159 insertions(+)