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

[Dave Martin <Dave.Martin@arm.com>]

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

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