Re: [PATCH v3] arm64: add ptrace regsets for ptrauth key management

[Kristina Martsenko <kristina.martsenko@arm.com>]

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;
>> +}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel