On Fri, Jan 10, 2025 at 06:40:29PM +0000, Brendan Jackman wrote:
Subject: Re: [PATCH RFC v2 03/29] mm: asi: Introduce ASI core API
x86/asi: ...
Introduce core API for Address Space Isolation (ASI). Kernel address
space isolation provides the ability to run some kernel
code with a restricted kernel address space.
There can be multiple classes of such restricted kernel address spaces
(e.g. KPTI, KVM-PTI etc.). Each ASI class is identified by an index.
The ASI class can register some hooks to be called when
entering/exiting the restricted address space.
Currently, there is a fixed maximum number of ASI classes supported.
In addition, each process can have at most one restricted address space
from each ASI class. Neither of these are inherent limitations and
are merely simplifying assumptions for the time being.
To keep things simpler for the time being, we disallow context switches
Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.
Also, see section "Changelog" in
Documentation/process/maintainer-tip.rst
within the restricted address space. In the future, we would be able to
relax this limitation for the case of context switches to different
threads within the same process (or to the idle thread and back).
Note that this doesn't really support protecting sibling VM guests
within the same VMM process from one another. From first principles
it seems unlikely that anyone who cares about VM isolation would do
that, but there could be a use-case to think about. In that case need
something like the OTHER_MM logic might be needed, but specific to
intra-process guest separation.
[0]:
https://lore.kernel.org/kvm/1562855138-19507-1-git-send-email-alexandre.chartre@xxxxxxxxxx
Notes about RFC-quality implementation details:
- Ignoring checkpatch.pl AVOID_BUG.
- The dynamic registration of classes might be pointless complexity.
This was kept from RFCv1 without much thought.
- The other-mm logic is also perhaps overly complex, suggestions are
welcome for how best to tackle this (or we could just forget about
it for the moment, and rely on asi_exit() happening in process
switch).
- The taint flag definitions would probably be clearer with an enum or
something.
Checkpatch-args: --ignore=AVOID_BUG,COMMIT_LOG_LONG_LINE,EXPORT_SYMBOL
Co-developed-by: Ofir Weisse <oweisse@xxxxxxxxxx>
Signed-off-by: Ofir Weisse <oweisse@xxxxxxxxxx>
Co-developed-by: Junaid Shahid <junaids@xxxxxxxxxx>
Signed-off-by: Junaid Shahid <junaids@xxxxxxxxxx>
Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
---
arch/x86/include/asm/asi.h | 208 +++++++++++++++++++++++
arch/x86/include/asm/processor.h | 8 +
arch/x86/mm/Makefile | 1 +
arch/x86/mm/asi.c | 350 +++++++++++++++++++++++++++++++++++++++
arch/x86/mm/init.c | 3 +-
arch/x86/mm/tlb.c | 1 +
include/asm-generic/asi.h | 67 ++++++++
include/linux/mm_types.h | 7 +
kernel/fork.c | 3 +
kernel/sched/core.c | 9 +
mm/init-mm.c | 4 +
11 files changed, 660 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/asi.h b/arch/x86/include/asm/asi.h
new file mode 100644
index 0000000000000000000000000000000000000000..7cc635b6653a3970ba9dbfdc9c828a470e27bd44
--- /dev/null
+++ b/arch/x86/include/asm/asi.h
@@ -0,0 +1,208 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_ASI_H
+#define _ASM_X86_ASI_H
+
+#include <linux/sched.h>
+
+#include <asm-generic/asi.h>
+
+#include <asm/pgtable_types.h>
+#include <asm/percpu.h>
+#include <asm/processor.h>
+
+#ifdef CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION
+
+/*
+ * Overview of API usage by ASI clients:
+ *
+ * Setup: First call asi_init() to create a domain. At present only one domain
+ * can be created per mm per class, but it's safe to asi_init() this domain
+ * multiple times. For each asi_init() call you must call asi_destroy() AFTER
+ * you are certain all CPUs have exited the restricted address space (by
+ * calling asi_exit()).
+ *
+ * Runtime usage:
+ *
+ * 1. Call asi_enter() to switch to the restricted address space. This can't be
+ * from an interrupt or exception handler and preemption must be disabled.
+ *
+ * 2. Execute untrusted code.
+ *
+ * 3. Call asi_relax() to inform the ASI subsystem that untrusted code execution
+ * is finished. This doesn't cause any address space change. This can't be
+ * from an interrupt or exception handler and preemption must be disabled.
+ *
+ * 4. Either:
+ *
+ * a. Go back to 1.
+ *
+ * b. Call asi_exit() before returning to userspace. This immediately
+ * switches to the unrestricted address space.
So only from reading this, it does sound weird. Maybe the code does it
differently - I'll see soon - but this basically says:
I asi_enter(), do something, asi_relax() and then I decide to do something
more and to asi_enter() again!? And then I can end it all with a *single*
asi_exit() call?
Hm, definitely weird API. Why?
/*
* Leave the "tense" state if we are in it, i.e. end the critical section. We
* will stay relaxed until the next asi_enter.
*/
void asi_relax(void);
Yeah, so there's no API functions balance between enter() and relax()...
+ *
+ * The region between 1 and 3 is called the "ASI critical section". During the
+ * critical section, it is a bug to access any sensitive data, and you mustn't
+ * sleep.
+ *
+ * The restriction on sleeping is not really a fundamental property of ASI.
+ * However for performance reasons it's important that the critical section is
+ * absolutely as short as possible. So the ability to do sleepy things like
+ * taking mutexes oughtn't to confer any convenience on API users.
+ *
+ * Similarly to the issue of sleeping, the need to asi_exit in case 4b is not a
+ * fundamental property of the system but a limitation of the current
+ * implementation. With further work it is possible to context switch
+ * from and/or to the restricted address space, and to return to userspace
+ * directly from the restricted address space, or _in_ it.
+ *
+ * Note that the critical section only refers to the direct execution path from
+ * asi_enter to asi_relax: it's fine to access sensitive data from exceptions
+ * and interrupt handlers that occur during that time. ASI will re-enter the
+ * restricted address space before returning from the outermost
+ * exception/interrupt.
+ *
+ * Note: ASI does not modify KPTI behaviour; when ASI and KPTI run together
+ * there are 2+N address spaces per task: the unrestricted kernel address space,
+ * the user address space, and one restricted (kernel) address space for each of
+ * the N ASI classes.
+ */
+
+/*
+ * ASI uses a per-CPU tainting model to track what mitigation actions are
+ * required on domain transitions. Taints exist along two dimensions:
+ *
+ * - Who touched the CPU (guest, unprotected kernel, userspace).
+ *
+ * - What kind of state might remain: "data" means there might be data owned by
+ * a victim domain left behind in a sidechannel. "Control" means there might
+ * be state controlled by an attacker domain left behind in the branch
+ * predictor.
+ *
+ * In principle the same domain can be both attacker and victim, thus we have
+ * both data and control taints for userspace, although there's no point in
+ * trying to protect against attacks from the kernel itself, so there's no
+ * ASI_TAINT_KERNEL_CONTROL.
+ */
+#define ASI_TAINT_KERNEL_DATA ((asi_taints_t)BIT(0))
+#define ASI_TAINT_USER_DATA ((asi_taints_t)BIT(1))
+#define ASI_TAINT_GUEST_DATA ((asi_taints_t)BIT(2))
+#define ASI_TAINT_OTHER_MM_DATA ((asi_taints_t)BIT(3))
+#define ASI_TAINT_USER_CONTROL ((asi_taints_t)BIT(4))
+#define ASI_TAINT_GUEST_CONTROL ((asi_taints_t)BIT(5))
+#define ASI_TAINT_OTHER_MM_CONTROL ((asi_taints_t)BIT(6))
+#define ASI_NUM_TAINTS 6
+static_assert(BITS_PER_BYTE * sizeof(asi_taints_t) >= ASI_NUM_TAINTS);
Why is this a typedef at all to make the code more unreadable than it needs to
be? Why not a simple unsigned int or char or whatever you need?
+
+#define ASI_TAINTS_CONTROL_MASK \
+ (ASI_TAINT_USER_CONTROL | ASI_TAINT_GUEST_CONTROL | ASI_TAINT_OTHER_MM_CONTROL)
+
+#define ASI_TAINTS_DATA_MASK \
+ (ASI_TAINT_KERNEL_DATA | ASI_TAINT_USER_DATA | ASI_TAINT_OTHER_MM_DATA)
+
+#define ASI_TAINTS_GUEST_MASK (ASI_TAINT_GUEST_DATA | ASI_TAINT_GUEST_CONTROL)
+#define ASI_TAINTS_USER_MASK (ASI_TAINT_USER_DATA | ASI_TAINT_USER_CONTROL)
+
+/* The taint policy tells ASI how a class interacts with the CPU taints */
+struct asi_taint_policy {
+ /*
+ * What taints would necessitate a flush when entering the domain, to
+ * protect it from attack by prior domains?
+ */
+ asi_taints_t prevent_control;
So if those necessitate a flush, why isn't this var called "taints_to_flush"
or whatever which exactly explains what it is?
+ /*
+ * What taints would necessetate a flush when entering the domain, to
+ * What taints would necessetate a flush when entering the domain, to
Unknown word [necessetate] in comment.
Suggestions: ['necessitate',
Spellchecker please. Go over your whole set.
+ * protect former domains from attack by this domain?
+ */
+ asi_taints_t protect_data;
Same.
+ /* What taints should be set when entering the domain? */
+ asi_taints_t set;
So "required_taints" or so... hm?
+};
+
+/*
+ * An ASI domain (struct asi) represents a restricted address space. The
no need for "(struct asi)" - it is right below :).
+ * unrestricted address space (and user address space under PTI) are not
+ * represented as a domain.
+ */
+struct asi {
+ pgd_t *pgd;
+ struct mm_struct *mm;
+ int64_t ref_count;
+ enum asi_class_id class_id;
+};
+
+DECLARE_PER_CPU_ALIGNED(struct asi *, curr_asi);
Or simply "asi" - this per-CPU var will be so prominent so that when you do
"per_cpu(asi)" you know what exactly it is
+
+void asi_init_mm_state(struct mm_struct *mm);
+
+int asi_init_class(enum asi_class_id class_id, struct asi_taint_policy *taint_policy);
+void asi_uninit_class(enum asi_class_id class_id);
"uninit", meh. "exit" perhaps? or "destroy"?
And you have "asi_destroy" already so I guess you can do:
asi_class_init()
asi_class_destroy()
to have the namespace correct.
+const char *asi_class_name(enum asi_class_id class_id);
+
+int asi_init(struct mm_struct *mm, enum asi_class_id class_id, struct asi **out_asi);
+void asi_destroy(struct asi *asi);
+
+/* Enter an ASI domain (restricted address space) and begin the critical section. */
+void asi_enter(struct asi *asi);
+
+/*
+ * Leave the "tense" state if we are in it, i.e. end the critical section. We
+ * will stay relaxed until the next asi_enter.
+ */
+void asi_relax(void);
+
+/* Immediately exit the restricted address space if in it */
+void asi_exit(void);
+
+/* The target is the domain we'll enter when returning to process context. */
+static __always_inline struct asi *asi_get_target(struct task_struct *p)
+{
+ return p->thread.asi_state.target;
+}
+
+static __always_inline void asi_set_target(struct task_struct *p,
+ struct asi *target)
+{
+ p->thread.asi_state.target = target;
+}
+
+static __always_inline struct asi *asi_get_current(void)
+{
+ return this_cpu_read(curr_asi);
+}
+
+/* Are we currently in a restricted address space? */
+static __always_inline bool asi_is_restricted(void)
+{
+ return (bool)asi_get_current();
+}
+
+/* If we exit/have exited, can we stay that way until the next asi_enter? */
+static __always_inline bool asi_is_relaxed(void)
+{
+ return !asi_get_target(current);
+}
+
+/*
+ * Is the current task in the critical section?
+ *
+ * This is just the inverse of !asi_is_relaxed(). We have both functions in order to
+ * help write intuitive client code. In particular, asi_is_tense returns false
+ * when ASI is disabled, which is judged to make user code more obvious.
+ */
+static __always_inline bool asi_is_tense(void)
+{
+ return !asi_is_relaxed();
+}
So can we tone down the silly helpers above? You don't really need
asi_is_tense() for example. It is still very intuitive if I read
if (!asi_is_relaxed())
...
+
+static __always_inline pgd_t *asi_pgd(struct asi *asi)
+{
+ return asi ? asi->pgd : NULL;
+}
+
+#define INIT_MM_ASI(init_mm) \
+ .asi_init_lock = __MUTEX_INITIALIZER(init_mm.asi_init_lock),
+
+void asi_handle_switch_mm(void);
+
+#endif /* CONFIG_MITIGATION_ADDRESS_SPACE_ISOLATION */
+
+#endif
Splitting the patch here and will continue with the next one as this one is
kinda big for one mail.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
