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

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

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