Re: [PATCH RFC v8 01/24] mm: Introduce kpkeys

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

On 5/26/26 13:15, 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 set and restore
> the pkey register and macros to define guard objects.
> 
> kpkeys introduces a new concept on top of pkeys: the kpkeys context.
> Each context is associated to a set of permissions for the pkeys
> managed by the kpkeys framework. kpkeys_set_context(ctx) sets those
> permissions according to ctx, and returns the original pkey
> register, to be later restored by kpkeys_restore_pkey_reg(). 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).
> 
> Because each architecture implementing pkeys uses a different
> representation for the pkey register, and may reserve certain pkeys
> for specific uses, support for kpkeys must be explicitly indicated
> by selecting ARCH_HAS_KPKEYS and defining the following functions in
> <asm/kpkeys.h>, in addition to the macros provided in
> <asm-generic/kpkeys.h>:
> 
> - arch_kpkeys_set_context()
> - arch_kpkeys_restore_pkey_reg()

Looking at this, and wondering about "why do we get registers involved in this
API" I would probably have an interface like:

	arch_kpkeys_enter_context()
	arch_kpkeys_leave_context()

Whereby you return a "struct kpkeys_state" or sth like that.

You could either let the architecture define what's in the state, or
alternatively store some generic data in there as well.

struct kpkeys_state {
	bool entered_context;
	struct arch_pkey_state arch;
};

Maybe the "entered_context" or however you would want to call it could avoid the
KPKEYS_PKEY_REG_INVAL (which confuses me ;) )?

But the KPKEYS_PKEY_REG_INVAL usage confuses me. I understand the
KPKEYS_GUARD_COND + kpkeys_restore_pkey_reg() one, but not the one where
arch_kpkeys_set_context() would return that value.


> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
>  include/asm-generic/kpkeys.h |  17 ++++++
>  include/linux/kpkeys.h       | 122 +++++++++++++++++++++++++++++++++++++++++++
>  mm/Kconfig                   |   2 +
>  3 files changed, 141 insertions(+)
> 
> diff --git a/include/asm-generic/kpkeys.h b/include/asm-generic/kpkeys.h
> new file mode 100644
> index 000000000000..ab819f157d6a
> --- /dev/null
> +++ b/include/asm-generic/kpkeys.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_KPKEYS_H
> +#define __ASM_GENERIC_KPKEYS_H
> +
> +#ifndef KPKEYS_PKEY_DEFAULT
> +#define KPKEYS_PKEY_DEFAULT	0
> +#endif

Do we currently expect an architecture to overwrite this? How does this interact
with KPKEYS_CTX_DEFAULT?

Nobody in this patch uses it, so maybe it should be added where actually needed.

> +
> +/*
> + * Represents a pkey register value that cannot be used, typically disabling
> + * access to all keys.
> + */
> +#ifndef KPKEYS_PKEY_REG_INVAL
> +#define KPKEYS_PKEY_REG_INVAL	0
> +#endif
> +
> +#endif	/* __ASM_GENERIC_KPKEYS_H */
> diff --git a/include/linux/kpkeys.h b/include/linux/kpkeys.h
> new file mode 100644
> index 000000000000..0ce0db80b4ee
> --- /dev/null
> +++ b/include/linux/kpkeys.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _LINUX_KPKEYS_H
> +#define _LINUX_KPKEYS_H
> +
> +#include <linux/bug.h>
> +#include <linux/cleanup.h>
> +
> +#define KPKEYS_CTX_DEFAULT	0


Agreed with enum.


> +
> +#define KPKEYS_CTX_MIN		KPKEYS_CTX_DEFAULT
> +#define KPKEYS_CTX_MAX		KPKEYS_CTX_DEFAULT
> +
> +/*
> + * ... is used to discard extra arguments - this allows users of this macro
> + * to have set_arg default to void.
> + */
> +#define __KPKEYS_GUARD(name, set_context, restore_pkey_reg, set_arg, ...) \
> +	__DEFINE_CLASS_IS_CONDITIONAL(name, false);			\
> +	DEFINE_CLASS(name, u64,						\
> +		     restore_pkey_reg, set_context, set_arg);		\
> +	static inline void *class_##name##_lock_ptr(u64 *_T)		\
> +	{ return _T; }
> +
> +/**
> + * KPKEYS_GUARD_NOOP() - define a guard type that does nothing
> + * @name: the name of the guard type
> + * @cond_arg: an argument specification (optional)
> + *
> + * Define a guard type that does nothing, useful to match a real guard type
> + * that is defined under an #ifdef. @cond_arg may optionally be passed to match
> + * a guard defined using KPKEYS_GUARD_COND().
> + */
> +#define KPKEYS_GUARD_NOOP(name, ...)					\
> +	__KPKEYS_GUARD(name, 0, (void)_T, ##__VA_ARGS__, void)
> +
> +#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
> + * @cond_arg: an argument specification for the condition (optional)
> + *
> + * Define a guard type that switches to @ctx if @cond evaluates to true,
> + * and does nothing otherwise. @cond_arg may be specified to give access to a
> + * caller-defined argument to @cond.
> + */
> +#define KPKEYS_GUARD_COND(name, ctx, cond, ...)				\
> +	__KPKEYS_GUARD(name,						\
> +		       (cond) ? kpkeys_set_context(ctx)			\
> +			      : KPKEYS_PKEY_REG_INVAL,			\
> +		       kpkeys_restore_pkey_reg(_T),			\
> +		       ##__VA_ARGS__, void)
> +
> +/**
> + * KPKEYS_GUARD() - define a guard type that switches to a given kpkeys context
> + *                  if kpkeys are enabled
> + * @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_enabled())
> +
> +/**
> + * kpkeys_set_context() - switch kpkeys context
> + * @ctx: the context to switch to
> + *
> + * Switches to specified kpkeys context. @ctx must be a compile-time
> + * constant. The arch-specific pkey register will be updated accordingly, and
> + * the original value returned.

Are these arch details and registers relevant? Ideally, we'd keep it very simple
here ...

> + *
> + * Return: the original pkey register value if the register was written to, or
> + *         KPKEYS_PKEY_REG_INVAL otherwise (no write to the register was
> + *         required).

... and here. Not sure if any caller cares about these details. Again, with some
abstract state we could maybe handle that internally.

"Return: the pkey state to pass to kpkeys_restore_pkey_reg" (or however that
function will be called)

> + */
> +static __always_inline u64 kpkeys_set_context(int ctx)
> +{
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(ctx),
> +			 "kpkeys_set_context() only takes constant values");
> +	BUILD_BUG_ON_MSG(ctx < KPKEYS_CTX_MIN || ctx > KPKEYS_CTX_MAX,
> +			 "Invalid value passed to kpkeys_set_context()");
> +
> +	return arch_kpkeys_set_context(ctx);
> +}
> +
> +/**
> + * kpkeys_restore_pkey_reg() - restores a pkey register value
> + * @pkey_reg: the pkey register value to restore
> + *
> + * This function is meant to be passed the value returned by
> + * kpkeys_set_context(), in order to restore the pkey register to its original
> + * value (thus restoring the original kpkeys context).
> + */
> +static __always_inline void kpkeys_restore_pkey_reg(u64 pkey_reg)
> +{
> +	if (pkey_reg != KPKEYS_PKEY_REG_INVAL)
> +		arch_kpkeys_restore_pkey_reg(pkey_reg);
> +}
> +
> +static inline bool kpkeys_enabled(void)

Is the enabled vs. supported intentional?

> +{
> +	return arch_supports_kpkeys();
> +}
> +



-- 
Cheers,

David