Re: [PATCH RFC v9 01/25] mm: Introduce kpkeys

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/18/26 16:08, Kevin Brodsky wrote:
> kpkeys is a simple framework to enable the use of protection keys
> (pkeys) to harden the kernel itself. This patch introduces the basic
> API in <linux/kpkeys.h>: a couple of functions to enter/leave a
> kpkeys context and macros to define guard objects.
> 
> kpkeys introduces a new concept on top of pkeys: the kpkeys context.
> Each context is associated with a set of permissions for the pkeys
> managed by the kpkeys framework. kpkeys_enter_context(ctx) sets
> those permissions according to ctx, and returns the original kpkeys
> state (typically the arch-specific pkeys register) that is later
> restored by calling kpkeys_leave_context(). To start with, only
> KPKEYS_CTX_DEFAULT is available, which is meant to grant RW access
> to KPKEYS_PKEY_DEFAULT (i.e. all memory since this is the only
> available pkey for now).
> 
> The underlying representation of each kpkeys context is entirely
> architecture-specific. Support for kpkeys must be explicitly
> indicated by selecting ARCH_HAS_KPKEYS and defining the following
> functions in <asm/kpkeys.h>:
> 
> * arch_kpkeys_enter_context()
> * arch_kpkeys_leave_context()
> * arch_supports_kpkeys()
> 
> Additionally, <asm/kpkeys_types.h> must define struct arch_kpkeys_state,
> which typically provides storage for the pkeys register.
> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
>  include/linux/kpkeys.h       | 104 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/kpkeys_types.h |  25 +++++++++++
>  mm/Kconfig                   |   2 +
>  3 files changed, 131 insertions(+)
> 
> diff --git a/include/linux/kpkeys.h b/include/linux/kpkeys.h
> new file mode 100644
> index 000000000000..eac522f55214
> --- /dev/null
> +++ b/include/linux/kpkeys.h
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _LINUX_KPKEYS_H
> +#define _LINUX_KPKEYS_H
> +
> +#include <linux/bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/kpkeys_types.h>
> +
> +/**
> + * KPKEYS_GUARD_NOOP() - define a guard type that does nothing
> + * @name: the name of the guard type
> + *
> + * Define a guard type that does nothing, useful to match a real guard type
> + * that is defined under an #ifdef.
> + */
> +#define KPKEYS_GUARD_NOOP(name)						\
> +	__DEFINE_CLASS_IS_CONDITIONAL(name, false);			\
> +	DEFINE_CLASS(name, bool, (void)_T, false, void);		\
> +	static inline void *class_##name##_lock_ptr(bool *_T)		\
> +	{ return _T; }
> +
> +#ifdef CONFIG_ARCH_HAS_KPKEYS
> +
> +#include <asm/kpkeys.h>
> +
> +/**
> + * KPKEYS_GUARD_COND() - define a guard type that conditionally switches to
> + *                       a given kpkeys context
> + * @name: the name of the guard type
> + * @ctx: the kpkeys context to switch to
> + * @cond: an expression that is evaluated as condition
> + *
> + * Define a guard type that switches to @ctx if @cond evaluates to true,
> + * and does nothing otherwise.
> + */
> +#define KPKEYS_GUARD_COND(name, ctx, cond)				\
> +	__DEFINE_CLASS_IS_CONDITIONAL(name, false);			\
> +	DEFINE_CLASS(name, struct kpkeys_state,				\
> +		     kpkeys_leave_context(&_T),				\
> +		     (cond) ? kpkeys_enter_context(ctx) :		\
> +			      (struct kpkeys_state) {}, void);		\
> +	static inline							\
> +	void *class_##name##_lock_ptr(struct kpkeys_state *_T)		\
> +	{ return _T; }
> +
> +/**
> + * KPKEYS_GUARD() - define a guard type that switches to a given kpkeys context
> + *                  if kpkeys are supported
> + * @name: the name of the guard type
> + * @ctx: the kpkeys context to switch to
> + *
> + * Define a guard type that switches to @ctx if the system supports kpkeys.
> + */
> +#define KPKEYS_GUARD(name, ctx)						\
> +	KPKEYS_GUARD_COND(name, ctx, kpkeys_supported())
> +
> +/**
> + * kpkeys_enter_context() - enter a kpkeys context
> + * @ctx: the context to switch to
> + *
> + * Enters the specified kpkeys context. @ctx must be a compile-time constant.
> + *
> + * Return: state to be passed to kpkeys_leave_context().
> + */
> +static __always_inline
> +struct kpkeys_state kpkeys_enter_context(enum kpkeys_ctx ctx)
> +{
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(ctx),
> +			 "kpkeys_enter_context() only takes constant values");
> +	BUILD_BUG_ON_MSG(ctx < 0 || ctx >= KPKEYS_CTX_COUNT,
> +			 "Invalid value passed to kpkeys_enter_context()");
> +
> +	return arch_kpkeys_enter_context(ctx);
> +}
> +
> +/**
> + * kpkeys_leave_context() - leave a kpkeys context
> + * @state: state returned by kpkeys_enter_context()
> + *
> + * Restores the state saved when entering a kpkeys context. If no context was
> + * entered, this function does nothing.
> + */
> +static __always_inline
> +void kpkeys_leave_context(const struct kpkeys_state *state)
> +{
> +	if (state->entered_context)
> +		arch_kpkeys_leave_context(state);
state->entered_context is a common code variable, but it's not set by
commoncode. Is there a reason?

IOW, should arch_kpkeys_enter_context() only return the arch parts, and
entered_context would be set in common code?

Also, should entering bail out if the context was already entered.

Last but not least, when would we expect to call kpkeys_leave_context() but the
context was not entered?

If this is really arch-specific stuff, probably it should go entirely into arch
doe. If this is common code stuff, likely it should be maintained entirely in
common code.


Overall this looks much cleaner to me compared to what I reviewed the last time
(was that v8? I don't remember :D )


-- 
Cheers,

David