[PATCH v1 0/5] Introduce SandBox Mode (SBM)

[PATCH v1 0/5] Introduce SandBox Mode (SBM)

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

The ultimate goal of SandBox Mode is to execute native kernel code
in an environment which permits memory access only to predefined
addresses, so potential vulnerabilities cannot be exploited or will
have no impact on the rest of the kernel.

This patch series adds the API and arch-independent infrastructure
of SandBox Mode to the kernel. It runs the target function on a
vmalloc()'ed copy of all input and output data. This alone prevents
some out-of-bounds accesses thanks to guard pages.

Patch 4/5 adds KUnit tests. It is also a good starting point to
understand how SandBox Mode is supposed to be used.

Detailed description of SandBox Mode goals, usage and future plans
can be found in patch 5/5 of this series and is not repeated in
this cover letter.

Petr Tesarik (5):
  sbm: SandBox Mode core data types and functions
  sbm: sandbox input and output buffers
  sbm: call helpers and thunks
  sbm: SandBox Mode KUnit test suite
  sbm: SandBox Mode documentation

 Documentation/security/index.rst        |   1 +
 Documentation/security/sandbox-mode.rst | 180 ++++++
 include/linux/sbm.h                     | 516 +++++++++++++++++
 init/Kconfig                            |   2 +
 kernel/Kconfig.sbm                      |  43 ++
 kernel/Makefile                         |   2 +
 kernel/sbm.c                            | 133 +++++
 kernel/sbm_test.c                       | 735 ++++++++++++++++++++++++
 8 files changed, 1612 insertions(+)
 create mode 100644 Documentation/security/sandbox-mode.rst
 create mode 100644 include/linux/sbm.h
 create mode 100644 kernel/Kconfig.sbm
 create mode 100644 kernel/sbm.c
 create mode 100644 kernel/sbm_test.c

-- 
2.34.1

[PATCH v1 1/5] sbm: SandBox Mode core data types and functions

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Introduce SandBox Mode (SBM) core infrastructure and make the feature
configurable at build time with CONFIG_SANDBOX_MODE.

Provide an API to execute a function in a sandbox. This target function
runs in an address space that is similar to but distinct from the caller's
address space. This is why the target function cannot be called directly.
Instead, it is called via sbm_exec(), which takes the target function as a
parameter and executes it inside the sandbox.

All target functions take one void parameter and return an integer which
can be interpreted as error status (zero is success, negative is error).

Store sandbox parameters and state in struct sbm, and define these
operations on it:

* sbm_init() - set up a sandbox
* sbm_destroy() - clean up sandbox resources
* sbm_error() - query error status
* sbm_exec() - execute code in sandbox

Allow to defer error checking until after the last operation. When a SBM
operation fails, set an error value in struct sbm and make it stick, that
is fail all subsequent operations and return this error instead. The error
value can be explicitly retrieved with sbm_error(), but simple use cases
can get by with the return value of sbm_exec() alone.

Also declare these arch hooks:

* arch_sbm_init() - arch-specific setup
* arch_sbm_destroy() - arch-specific cleanup
* arch_sbm_exec() - arch-specific code execution

These hooks are required to provide strong isolation. The availability of
arch hooks is indicated by CONFIG_HAVE_ARCH_SBM. Initially, no architecture
provides SBM arch hooks, falling back to a trivial no-op implementation.

Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 include/linux/sbm.h | 154 ++++++++++++++++++++++++++++++++++++++++++++
 init/Kconfig        |   2 +
 kernel/Kconfig.sbm  |  31 +++++++++
 kernel/Makefile     |   1 +
 kernel/sbm.c        |  45 +++++++++++++
 5 files changed, 233 insertions(+)
 create mode 100644 include/linux/sbm.h
 create mode 100644 kernel/Kconfig.sbm
 create mode 100644 kernel/sbm.c

diff --git a/include/linux/sbm.h b/include/linux/sbm.h
new file mode 100644
index 000000000000..8e0c63fb9fb2
--- /dev/null
+++ b/include/linux/sbm.h
@@ -0,0 +1,154 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Petr Tesarik <petr.tesarik1@huawei-partners.com>
+ *
+ * SandBox Mode (SBM) public API declarations.
+ */
+#ifndef __LINUX_SBM_H
+#define __LINUX_SBM_H
+
+/**
+ * struct sbm - SandBox Mode instance.
+ * @error:    Error code. Initialized to zero by sbm_init() and updated when
+ *            a SBM operation fails.
+ * @private:  Arch-specific private data.
+ */
+struct sbm {
+#ifdef CONFIG_SANDBOX_MODE
+	int error;
+	void *private;
+#endif
+};
+
+/**
+ * typedef sbm_func - Sandbox mode function pointer.
+ * @data:  Arbitrary data passed via sbm_exec().
+ *
+ * Return: Zero on success, negative on error.
+ */
+typedef int (*sbm_func)(void *data);
+
+#ifdef CONFIG_SANDBOX_MODE
+
+/**
+ * sbm_init() - Initialize a SandBox Mode instance.
+ * @sbm:     SBM instance.
+ *
+ * Initialize a SBM instance structure.
+ *
+ * Return: Zero on success, negative on error.
+ */
+int sbm_init(struct sbm *sbm);
+
+/**
+ * sbm_destroy() - Clean up a SandBox Mode instance.
+ * @sbm:    SBM instance to be cleaned up.
+ */
+void sbm_destroy(struct sbm *sbm);
+
+/**
+ * sbm_error() - Get SBM error status.
+ * @sbm:  SBM instance.
+ *
+ * Get the SBM error code. This can be used to distinguish between
+ * errors returned by the target function and errors from setting
+ * up the sandbox environment.
+ */
+static inline int sbm_error(const struct sbm *sbm)
+{
+	return sbm->error;
+}
+
+/**
+ * sbm_exec() - Execute function in a sandbox.
+ * @sbm:   SBM instance.
+ * @func:  Function to be called.
+ * @data:  Argument for @func.
+ *
+ * Execute @func in a fully prepared SBM instance.
+ *
+ * Return: Return value of @func on success, or a negative error code.
+ */
+int sbm_exec(struct sbm *sbm, sbm_func func, void *data);
+
+#ifdef CONFIG_HAVE_ARCH_SBM
+
+/**
+ * arch_sbm_init() - Arch hook to initialize a SBM instance.
+ * @sbm:  Instance to be initialized.
+ *
+ * Perform any arch-specific initialization. This hook is called by sbm_init()
+ * immediately after zeroing out @sbm.
+ *
+ * Return: Zero on success, negative error code on failure.
+ */
+int arch_sbm_init(struct sbm *sbm);
+
+/**
+ * arch_sbm_destroy() - Arch hook to clean up a SBM instance.
+ * @sbm:  Instance to be cleaned up.
+ *
+ * Perform any arch-specific cleanup. This hook is called by sbm_destroy() as
+ * the very last operation on @sbm.
+ */
+void arch_sbm_destroy(struct sbm *sbm);
+
+/**
+ * arch_sbm_exec() - Arch hook to execute code in a sandbox.
+ * @sbm:   SBM instance.
+ * @func:  Function to be executed in a sandbox.
+ * @data:  Argument passed to @func.
+ *
+ * Execute @func in a fully prepared SBM instance. If sandbox mode
+ * cannot be set up or is aborted, set &sbm->error to a negative error
+ * value. This error is then returned by sbm_exec(), overriding the
+ * return value of arch_sbm_exec().
+ *
+ * Return: Return value of @func.
+ */
+int arch_sbm_exec(struct sbm *sbm, sbm_func func, void *data);
+
+#else /* !CONFIG_HAVE_ARCH_SBM */
+
+static inline int arch_sbm_init(struct sbm *sbm)
+{
+	return 0;
+}
+
+static inline void arch_sbm_destroy(struct sbm *sbm)
+{
+}
+
+static inline int arch_sbm_exec(struct sbm *sbm, sbm_func func, void *data)
+{
+	return func(data);
+}
+
+#endif /* CONFIG_HAVE_ARCH_SBM */
+
+#else /* !CONFIG_SANDBOX_MODE */
+
+static inline int sbm_init(struct sbm *sbm)
+{
+	return 0;
+}
+
+static inline void sbm_destroy(struct sbm *sbm)
+{
+}
+
+static inline int sbm_error(const struct sbm *sbm)
+{
+	return 0;
+}
+
+static inline int sbm_exec(struct sbm *sbm, sbm_func func, void *data)
+{
+	return func(data);
+}
+
+#endif /* CONFIG_SANDBOX_MODE */
+
+#endif /* __LINUX_SBM_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8d4e836e1b6b..253ac8c45527 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1932,6 +1932,8 @@ config TRACEPOINTS
 
 source "kernel/Kconfig.kexec"
 
+source "kernel/Kconfig.sbm"
+
 endmenu		# General setup
 
 source "arch/Kconfig"
diff --git a/kernel/Kconfig.sbm b/kernel/Kconfig.sbm
new file mode 100644
index 000000000000..64d683cefd4d
--- /dev/null
+++ b/kernel/Kconfig.sbm
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
+#
+# Author: Petr Tesarik <petr.tesarik1@huawei-partners.com>
+#
+# SandBox Mode (SBM) config options.
+#
+
+config HAVE_ARCH_SBM
+       def_bool n
+
+config SANDBOX_MODE
+	bool "SandBox Mode (SBM)"
+	default n
+	help
+	  SandBox Mode provides kernel API to run native kernel functions in a
+	  sandbox, preventing out-of-bounds memory accesses. On targets which
+	  implement SBM arch hooks, the isolation is strong, preventing all
+	  memory accesses outside the sandbox; after a protection violation,
+	  the affected kernel thread can continue running. On all other
+	  targets, the isolation is weak, preventing only buffer overflows
+	  within a guard page; after a violation, the kernel thread usually
+	  terminates.
+
+	  This is an opt-in self-defense mechanism, i.e. kernel source code
+	  must be modified to run in SandBox Mode. For such code, there is
+	  some run-time overhead (CPU time, memory) associated with entering
+	  and leaving the sandbox.
+
+	  If unsure, say N.
diff --git a/kernel/Makefile b/kernel/Makefile
index ce105a5558fc..ecc4bfd6213f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_HAVE_STATIC_CALL) += static_call.o
 obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o
 obj-$(CONFIG_CFI_CLANG) += cfi.o
 obj-$(CONFIG_NUMA) += numa.o
+obj-$(CONFIG_SANDBOX_MODE) += sbm.o
 
 obj-$(CONFIG_PERF_EVENTS) += events/
 
diff --git a/kernel/sbm.c b/kernel/sbm.c
new file mode 100644
index 000000000000..9a5b89a71a23
--- /dev/null
+++ b/kernel/sbm.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Petr Tesarik <petr.tesarik1@huawei-partners.com>
+ *
+ * SandBox Mode (SBM) public API and generic functions.
+ */
+
+#include <linux/export.h>
+#include <linux/sbm.h>
+#include <linux/string.h>
+
+int sbm_init(struct sbm *sbm)
+{
+	memset(sbm, 0, sizeof(*sbm));
+
+	sbm->error = arch_sbm_init(sbm);
+	if (sbm->error)
+		return sbm->error;
+
+	return 0;
+}
+EXPORT_SYMBOL(sbm_init);
+
+void sbm_destroy(struct sbm *sbm)
+{
+	arch_sbm_destroy(sbm);
+}
+EXPORT_SYMBOL(sbm_destroy);
+
+int sbm_exec(struct sbm *sbm, sbm_func func, void *args)
+{
+	int ret;
+
+	if (sbm->error)
+		return sbm->error;
+
+	ret = arch_sbm_exec(sbm, func, args);
+	if (sbm->error)
+		return sbm->error;
+
+	return ret;
+}
+EXPORT_SYMBOL(sbm_exec);
-- 
2.34.1

[PATCH v1 2/5] sbm: sandbox input and output buffers

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Provide SBM_COPY_IN(), SBM_COPY_OUT() and SBM_COPY_INOUT() macros to
allocate sandbox mode buffers for input/output data. Input data is copied
from kernel mode to the allocated buffer before calling the target
function. Output data is copied from the allocated buffer to kernel mode
after the target function returns.

Define two new arch hooks to map input/output buffers:

* arch_sbm_map_readonly()
* arch_sbm_map_writable()

Before calling the target function, use these hooks to create mappings for
all buffers, read-only or writable as appropriate. Provide a fallback no-op
implementation.

Upon expansion, the SBM_COPY_xxx() macros evaluate to the address of the
buffer in sandbox mode, cast back to the original type. This pointer should
be used by code running in the sandbox. It should not be used in kernel
mode; although the address is valid, the buffers are overwritten by
sbm_exec().

To do the typecast, prefer typeof(({x;})) over typeof(x). The statement
expression forces array-to-pointer decay, which allows to pass an array as
an argument to these macros.

Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 include/linux/sbm.h | 154 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/sbm.c        |  88 +++++++++++++++++++++++++
 2 files changed, 242 insertions(+)

diff --git a/include/linux/sbm.h b/include/linux/sbm.h
index 8e0c63fb9fb2..9671b3c556c7 100644
--- a/include/linux/sbm.h
+++ b/include/linux/sbm.h
@@ -9,16 +9,27 @@
 #ifndef __LINUX_SBM_H
 #define __LINUX_SBM_H
 
+struct sbm_buf;
+
 /**
  * struct sbm - SandBox Mode instance.
  * @error:    Error code. Initialized to zero by sbm_init() and updated when
  *            a SBM operation fails.
  * @private:  Arch-specific private data.
+ * @input:    Input data. Copied to a temporary buffer before starting sandbox
+ *            mode.
+ * @output:   Output data. Copied from a temporary buffer after return from
+ *            sandbox mode.
+ * @io:       Input and output data. Copied to a temporary buffer before
+ *            starting sandbox mode and copied back after return.
  */
 struct sbm {
 #ifdef CONFIG_SANDBOX_MODE
 	int error;
 	void *private;
+	struct sbm_buf *input;
+	struct sbm_buf *output;
+	struct sbm_buf *io;
 #endif
 };
 
@@ -73,6 +84,103 @@ static inline int sbm_error(const struct sbm *sbm)
  */
 int sbm_exec(struct sbm *sbm, sbm_func func, void *data);
 
+/**
+ * struct sbm_buf - Description of an input/output buffer.
+ * @next:      Pointer to the next buffer in the list.
+ * @kern_ptr:  Buffer address in kernel mode.
+ * @sbm_ptr:   Buffer address in sandbox mode.
+ * @size:      Size of the buffer.
+ */
+struct sbm_buf {
+	struct sbm_buf *next;
+	void *kern_ptr;
+	void *sbm_ptr;
+	size_t size;
+};
+
+/**
+ * sbm_alloc_buf() - Allocate a new input/output buffer.
+ * @sbm:   SBM instance.
+ * @size:  Size of the buffer.
+ *
+ * Allocate a new &struct sbm_buf and the corresponding sandbox mode
+ * input/output buffer. If either allocation fails, update &sbm->error.
+ *
+ * Return: New buffer descriptor, or %NULL on allocation failure.
+ */
+struct sbm_buf *sbm_alloc_buf(struct sbm *sbm, size_t size);
+
+/**
+ * sbm_add_buf() - Add a new I/O buffer to the SBM instance.
+ * @sbm:   SBM instance.
+ * @list:  Target argument buffer list.
+ * @buf:   Buffer virtual address.
+ * @size:  Size of the buffer.
+ *
+ * Add a new buffer to @list.
+ *
+ * Return: SBM address of the buffer, or %NULL on error.
+ */
+static inline void *sbm_add_buf(struct sbm *sbm, struct sbm_buf **list,
+				const void *buf, size_t size)
+{
+	struct sbm_buf *io;
+
+	io = sbm_alloc_buf(sbm, size);
+	if (!io)
+		return NULL;
+
+	io->kern_ptr = (void *)buf;
+	io->next = *list;
+	*list = io;
+	return io->sbm_ptr;
+}
+
+/**
+ * SBM_COPY_IN() - Mark an input buffer for copying into SBM.
+ * @sbm:   SBM instance.
+ * @buf:   Buffer virtual address.
+ * @size:  Size of the buffer.
+ *
+ * Add a buffer to the input buffer list for @sbm. The content of the
+ * buffer is copied to sandbox mode before calling the target function.
+ *
+ * It is OK to modify the input buffer after invoking this macro.
+ *
+ * Return: Buffer address in sandbox mode.
+ */
+#define SBM_COPY_IN(sbm, buf, size) \
+	((typeof(({buf; })))sbm_add_buf((sbm), &(sbm)->input, (buf), (size)))
+
+/**
+ * SBM_COPY_OUT() - Mark an output buffer for copying out of SBM.
+ * @sbm:   SBM instance.
+ * @buf:   Buffer virtual address.
+ * @size:  Size of the buffer.
+ *
+ * Add a buffer to the output buffer list for @sbm. The content of the
+ * buffer is copied to kernel mode after calling the target function.
+ *
+ * Return: Buffer address in sandbox mode.
+ */
+#define SBM_COPY_OUT(sbm, buf, size) \
+	((typeof(({buf; })))sbm_add_buf((sbm), &(sbm)->output, (buf), (size)))
+
+/**
+ * SBM_COPY_INOUT() - Mark an input buffer for copying into SBM and out of SBM.
+ * @sbm:   SBM instance.
+ * @buf:   Buffer virtual address.
+ * @size:  Size of the buffer.
+ *
+ * Add a buffer to the input and output buffer list for @sbm. The content
+ * of the buffer is copied to sandbox mode before calling the target function
+ * and copied back to kernel mode after the call.
+ *
+ * Return: Buffer address in sandbox mode.
+ */
+#define SBM_COPY_INOUT(sbm, buf, size) \
+	((typeof(({buf; })))sbm_add_buf((sbm), &(sbm)->io, (buf), (size)))
+
 #ifdef CONFIG_HAVE_ARCH_SBM
 
 /**
@@ -95,6 +203,30 @@ int arch_sbm_init(struct sbm *sbm);
  */
 void arch_sbm_destroy(struct sbm *sbm);
 
+/**
+ * arch_sbm_map_readonly() - Arch hook to map a buffer for reading.
+ * @sbm:  SBM instance.
+ * @buf:  Buffer to be mapped.
+ *
+ * Make the specified buffer readable by sandbox code. See also
+ * arch_sbm_map_writable().
+ *
+ * Return: Zero on success, negative on error.
+ */
+int arch_sbm_map_readonly(struct sbm *sbm, const struct sbm_buf *buf);
+
+/**
+ * arch_sbm_map_writable() - Arch hook to map a buffer for reading and writing.
+ * @sbm:  SBM instance.
+ * @buf:  Buffer to be mapped.
+ *
+ * Make the specified buffer readable and writable by sandbox code.
+ * See also arch_sbm_map_readonly().
+ *
+ * Return: Zero on success, negative on error.
+ */
+int arch_sbm_map_writable(struct sbm *sbm, const struct sbm_buf *buf);
+
 /**
  * arch_sbm_exec() - Arch hook to execute code in a sandbox.
  * @sbm:   SBM instance.
@@ -121,6 +253,18 @@ static inline void arch_sbm_destroy(struct sbm *sbm)
 {
 }
 
+static inline int arch_sbm_map_readonly(struct sbm *sbm,
+					const struct sbm_buf *buf)
+{
+	return 0;
+}
+
+static inline int arch_sbm_map_writable(struct sbm *sbm,
+					const struct sbm_buf *buf)
+{
+	return 0;
+}
+
 static inline int arch_sbm_exec(struct sbm *sbm, sbm_func func, void *data)
 {
 	return func(data);
@@ -149,6 +293,16 @@ static inline int sbm_exec(struct sbm *sbm, sbm_func func, void *data)
 	return func(data);
 }
 
+/* Evaluate expression exactly once, avoiding warnings about a "statement
+ * with no effect". GCC doesn't issue this warning for the return value
+ * of a statement expression.
+ */
+#define __SBM_EVAL(x) ({ typeof(({x; })) __tmp = (x); __tmp; })
+
+#define SBM_COPY_IN(sbm, buf, size)  __SBM_EVAL(buf)
+#define SBM_COPY_OUT(sbm, buf, size) __SBM_EVAL(buf)
+#define SBM_COPY_INOUT(sbm, buf, size) __SBM_EVAL(buf)
+
 #endif /* CONFIG_SANDBOX_MODE */
 
 #endif /* __LINUX_SBM_H */
diff --git a/kernel/sbm.c b/kernel/sbm.c
index 9a5b89a71a23..df57184f5d87 100644
--- a/kernel/sbm.c
+++ b/kernel/sbm.c
@@ -9,7 +9,46 @@
 
 #include <linux/export.h>
 #include <linux/sbm.h>
+#include <linux/slab.h>
 #include <linux/string.h>
+#include <linux/vmalloc.h>
+
+struct sbm_buf *sbm_alloc_buf(struct sbm *sbm, size_t size)
+{
+	struct sbm_buf *buf;
+
+	if (sbm->error)
+		return NULL;
+
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf) {
+		sbm->error = -ENOMEM;
+		goto out;
+	}
+	buf->sbm_ptr = vzalloc(size);
+	if (!buf->sbm_ptr) {
+		sbm->error = -ENOMEM;
+		goto out;
+	}
+	buf->size = size;
+
+out:
+	return buf;
+}
+EXPORT_SYMBOL(sbm_alloc_buf);
+
+/* Free a buffer list. */
+static void sbm_free_buf_list(const struct sbm_buf *buf)
+{
+	const struct sbm_buf *nextbuf;
+
+	while (buf) {
+		vfree(buf->sbm_ptr);
+		nextbuf = buf->next;
+		kfree(buf);
+		buf = nextbuf;
+	}
+}
 
 int sbm_init(struct sbm *sbm)
 {
@@ -25,14 +64,61 @@ EXPORT_SYMBOL(sbm_init);
 
 void sbm_destroy(struct sbm *sbm)
 {
+	sbm_free_buf_list(sbm->input);
+	sbm_free_buf_list(sbm->output);
+	sbm_free_buf_list(sbm->io);
 	arch_sbm_destroy(sbm);
 }
 EXPORT_SYMBOL(sbm_destroy);
 
+/* Copy input buffers into a sandbox. */
+static int sbm_copy_in(struct sbm *sbm)
+{
+	const struct sbm_buf *buf;
+	int err = 0;
+
+	for (buf = sbm->input; buf; buf = buf->next) {
+		err = arch_sbm_map_readonly(sbm, buf);
+		if (err)
+			return err;
+		memcpy(buf->sbm_ptr, buf->kern_ptr, buf->size);
+	}
+
+	for (buf = sbm->io; buf; buf = buf->next) {
+		err = arch_sbm_map_writable(sbm, buf);
+		if (err)
+			return err;
+		memcpy(buf->sbm_ptr, buf->kern_ptr, buf->size);
+	}
+
+	for (buf = sbm->output; buf; buf = buf->next) {
+		err = arch_sbm_map_writable(sbm, buf);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/* Copy output buffers out of a sandbox. */
+static void sbm_copy_out(struct sbm *sbm)
+{
+	const struct sbm_buf *buf;
+
+	for (buf = sbm->output; buf; buf = buf->next)
+		memcpy(buf->kern_ptr, buf->sbm_ptr, buf->size);
+	for (buf = sbm->io; buf; buf = buf->next)
+		memcpy(buf->kern_ptr, buf->sbm_ptr, buf->size);
+}
+
 int sbm_exec(struct sbm *sbm, sbm_func func, void *args)
 {
 	int ret;
 
+	if (sbm->error)
+		return sbm->error;
+
+	sbm->error = sbm_copy_in(sbm);
 	if (sbm->error)
 		return sbm->error;
 
@@ -40,6 +126,8 @@ int sbm_exec(struct sbm *sbm, sbm_func func, void *args)
 	if (sbm->error)
 		return sbm->error;
 
+	sbm_copy_out(sbm);
+
 	return ret;
 }
 EXPORT_SYMBOL(sbm_exec);
-- 
2.34.1

[PATCH v1 3/5] sbm: call helpers and thunks

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

The sbm_exec() function allows to pass only a single void pointer to the
target function running in sandbox mode. Provide a set of macros which make
it easier to pass a wide variety of parameters from kernel mode to sandbox
mode, preserving C type safety.

To use this mechanism with a target function foo(), define the matching
call helper and thunk like this:

    /* This can go into a header file: */
    int foo(struct bar *data);
    SBM_DEFINE_CALL(foo, struct bar *, data);

    /* This should be defined together with foo(): */
    SBM_DEFINE_THUNK(foo, struct bar *, data);

The call helper, running in kernel mode, accepts the same set of parameters
as the target function. It saves them in a target-specific struct and calls
sbm_exec(), passing it a pointer to this struct. This pointer becomes the
data parameter of the matching thunk function, running in sandbox mode. The
thunk interprets the parameter as a pointer to the target-specific struct,
loads the saved arguments from this struct and calls the target function.

Define a shorthand macro SBM_DEFINE_FUNC() that can be used if the target
function, thunk and call helper are all used only in one file:

    static SBM_DEFINE_FUNC(foo, struct bar *, data)
    {
	/* do something with data */
	return 0;
    }

Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 include/linux/sbm.h | 208 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 208 insertions(+)

diff --git a/include/linux/sbm.h b/include/linux/sbm.h
index 9671b3c556c7..98fd27cd58d0 100644
--- a/include/linux/sbm.h
+++ b/include/linux/sbm.h
@@ -305,4 +305,212 @@ static inline int sbm_exec(struct sbm *sbm, sbm_func func, void *data)
 
 #endif /* CONFIG_SANDBOX_MODE */
 
+/**
+ * __SBM_MAP() - Convert parameters to comma-separated expressions.
+ * @m: Macro used to convert each pair.
+ * @e: Expansion if no arguments are given.
+ */
+#define __SBM_MAP(m, e, ...) \
+	CONCATENATE(__SBM_MAP, COUNT_ARGS(__VA_ARGS__))(m, e, ##__VA_ARGS__)
+#define __SBM_MAP0(m, e)             e
+#define __SBM_MAP2(m, e, t, a)       m(t, a)
+#define __SBM_MAP4(m, e, t, a, ...)  m(t, a), __SBM_MAP2(m, e, __VA_ARGS__)
+#define __SBM_MAP6(m, e, t, a, ...)  m(t, a), __SBM_MAP4(m, e, __VA_ARGS__)
+#define __SBM_MAP8(m, e, t, a, ...)  m(t, a), __SBM_MAP6(m, e, __VA_ARGS__)
+#define __SBM_MAP10(m, e, t, a, ...) m(t, a), __SBM_MAP8(m, e, __VA_ARGS__)
+#define __SBM_MAP12(m, e, t, a, ...) m(t, a), __SBM_MAP10(m, e, __VA_ARGS__)
+
+/**
+ * __SBM_MEMBERS() - Convert parameters to struct declaration body.
+ *
+ * This macro is similar to __SBM_MAP(), but the declarations are delimited by
+ * semicolons, not commas.
+ */
+#define __SBM_MEMBERS(...) \
+	CONCATENATE(__SBM_MEMBERS, COUNT_ARGS(__VA_ARGS__))(__VA_ARGS__)
+#define __SBM_MEMBERS0()
+#define __SBM_MEMBERS2(t, a)       t a;
+#define __SBM_MEMBERS4(t, a, ...)  t a; __SBM_MEMBERS2(__VA_ARGS__)
+#define __SBM_MEMBERS6(t, a, ...)  t a; __SBM_MEMBERS4(__VA_ARGS__)
+#define __SBM_MEMBERS8(t, a, ...)  t a; __SBM_MEMBERS6(__VA_ARGS__)
+#define __SBM_MEMBERS10(t, a, ...) t a; __SBM_MEMBERS8(__VA_ARGS__)
+#define __SBM_MEMBERS12(t, a, ...) t a; __SBM_MEMBERS10(__VA_ARGS__)
+
+/************************* Target function **************************/
+
+/**
+ * __SBM_DECL() - Map a parameter to a declaration.
+ * @type: Parameter type.
+ * @id:   Parameter identifier.
+ *
+ * Use this macro with __SBM_MAP() to get variable or function parameter
+ * declarations.
+ */
+#define __SBM_DECL(type, id)   type id
+
+/**
+ * __SBM_DECLARE_FUNC() - Declare a target function.
+ * @f:   Target function name.
+ * @...: Parameters as type-identifier pairs.
+ *
+ * Target function parameters are specified as type-identifier pairs, somewhat
+ * similar to SYSCALL_DEFINEn(). The function name @f is followed by up to 6
+ * type and identifier pairs, one for each parameter. The number of parameters
+ * is determined automatically.
+ *
+ * For example, if your target function is declared like this:
+ *
+ * .. code-block:: c
+ *   static int foo(struct bar *baz);
+ *
+ * it would be declared with __SBM_DECLARE_FUNC() like this:
+ *
+ * .. code-block:: c
+ *   static __SBM_DECLARE_FUNC(foo, struct bar *, baz);
+ *
+ */
+#define __SBM_DECLARE_FUNC(f, ...) \
+	int f(__SBM_MAP(__SBM_DECL, void, ##__VA_ARGS__))
+
+/*************************** Call helper ****************************/
+
+/**
+ * __SBM_CALL() - Call helper function identifier.
+ * @f: Target function name.
+ */
+#define __SBM_CALL(f)	__sbm_call_##f
+
+/**
+ * __SBM_VAR() - Map a parameter to its identifier.
+ * @type: Parameter type (unused).
+ * @id:   Parameter identifier.
+ *
+ * Use this macro with __SBM_MAP() to get only the identifier from each
+ * type-identifier pair.
+ */
+#define __SBM_VAR(type, id)    id
+
+/**
+ * __SBM_OPT_ARG() - Define an optional macro argument.
+ * @...: Optional parameters.
+ *
+ * Expand to a comma followed by all macro parameters, but if the parameter
+ * list is empty, expand to nothing (not even the comma).
+ */
+#define __SBM_OPT_ARG(...)	__SBM_OPT_ARG_1(__VA_ARGS__)
+#define __SBM_OPT_ARG_1(...)	, ##__VA_ARGS__
+
+/**
+ * SBM_DEFINE_CALL() - Define a call helper.
+ * @f:   Target function name.
+ * @...: Parameters as type-identifier pairs.
+ *
+ * Declare an argument-passing struct and define the corresponding call
+ * helper. The call helper stores its arguments in an automatic variable of
+ * the corresponding type and calls sbm_exec().
+ *
+ * The call helper is an inline function, so it is OK to use this macro in
+ * header files.
+ *
+ * Target function parameters are specified as type-identifier pairs, see
+ * __SBM_DECLARE_FUNC().
+ */
+#define SBM_DEFINE_CALL(f, ...) \
+	int __SBM_THUNK(f)(void *__p);					\
+	struct __SBM_ARG(f) {						\
+		__SBM_MEMBERS(__VA_ARGS__)				\
+	};								\
+	static inline int __SBM_CALL(f)(				\
+		struct sbm *__sbm					\
+		__SBM_OPT_ARG(__SBM_MAP(__SBM_DECL, , ##__VA_ARGS__)))	\
+	{								\
+		struct __SBM_ARG(f) __args = {				\
+			__SBM_MAP(__SBM_VAR, , ##__VA_ARGS__)		\
+		};							\
+		return sbm_exec(__sbm, __SBM_THUNK(f), &__args);	\
+	}
+
+/************************** Thunk function **************************/
+
+/**
+ * __SBM_ARG() - Struct tag for target function arguments.
+ * @f: Target function name.
+ */
+#define __SBM_ARG(f)	__sbm_arg_##f
+
+/**
+ * __SBM_DEREF() - Map a parameter to a struct __SBM_ARG() field.
+ * @type: Parameter type (unused).
+ * @id:   Parameter identifier.
+ *
+ * Use this macro with __SBM_MAP() to dereference a struct __SBM_ARG()
+ * pointer.
+ */
+#define __SBM_DEREF(type, id)  __arg->id
+
+/**
+ * __SBM_THUNK() - Thunk function identifier.
+ * @f: Target function name.
+ *
+ * Use this macro to generate the thunk function identifier for a given target
+ * function.
+ */
+#define __SBM_THUNK(f)	__sbm_thunk_##f
+
+/**
+ * SBM_DEFINE_THUNK() - Define a thunk function.
+ * @f:   Target function name.
+ * @...: Parameters as type-identifier pairs.
+ *
+ * The thunk function casts its parameter back to the argument-passing struct
+ * and calls the target function @f with parameters stored there by the call
+ * helper.
+ *
+ * Target function parameters are specified as type-identifier pairs, see
+ * __SBM_DECLARE_FUNC().
+ */
+#define SBM_DEFINE_THUNK(f, ...) \
+	int __SBM_THUNK(f)(void *__p)					\
+	{								\
+		struct __SBM_ARG(f) *__arg __maybe_unused = __p;	\
+		return (f)(__SBM_MAP(__SBM_DEREF, , ##__VA_ARGS__));	\
+	}
+
+/**************************** Shorthands ****************************/
+
+/**
+ * SBM_DEFINE_FUNC() - Define target function, thunk and call helper.
+ * @f:   Target function name.
+ * @...: Parameters as type-identifier pairs.
+ *
+ * Declare or define a target function and also the corresponding
+ * thunk and call helper. Use this shorthand to avoid repeating the
+ * target function signature.
+ *
+ * The target function is declared twice. The first declaration allows to
+ * precede the macro with storage-class specifiers. The second declaration
+ * allows to follow the macro with the function body. You can also put a
+ * semicolon after the macro to make it only a declaration.
+ *
+ * Target function parameters are specified as type-identifier pairs, see
+ * __SBM_DECLARE_FUNC().
+ */
+#define SBM_DEFINE_FUNC(f, ...) \
+	__SBM_DECLARE_FUNC(f, ##__VA_ARGS__);		\
+	static SBM_DEFINE_CALL(f, ##__VA_ARGS__)	\
+	static SBM_DEFINE_THUNK(f, ##__VA_ARGS__)	\
+	__SBM_DECLARE_FUNC(f, ##__VA_ARGS__)
+
+/**
+ * sbm_call() - Call a function in sandbox mode.
+ * @sbm:    SBM instance.
+ * @func:   Function to be called.
+ * @...:    Target function arguments.
+ *
+ * Call a function using a call helper which was previously defined with
+ * SBM_DEFINE_FUNC().
+ */
+#define sbm_call(sbm, func, ...) \
+	__SBM_CALL(func)(sbm, ##__VA_ARGS__)
+
 #endif /* __LINUX_SBM_H */
-- 
2.34.1

[PATCH v1 4/5] sbm: SandBox Mode KUnit test suite

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Test that a function called in sandbox mode can use input and output
buffers but blocks attempts to read or write outside the pre-defined
memory areas.

Some tests intentionally access a non-present page, which generates a page
fault in kernel space if SBM implementation cannot recover from failures.
Theoretically, the page fault handler could recover from such page faults,
but the exact details differ across architectures, and there is no
universal method. The most portable approach is to let the page fault
handler treat the fault as unrecoverable and kill the current task. This is
why a child task is used to run these tests. The parent KUnit thread can
check if the child was killed with a signal.

Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 kernel/Kconfig.sbm |  12 +
 kernel/Makefile    |   1 +
 kernel/sbm_test.c  | 735 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 748 insertions(+)
 create mode 100644 kernel/sbm_test.c

diff --git a/kernel/Kconfig.sbm b/kernel/Kconfig.sbm
index 64d683cefd4d..d28a754bc704 100644
--- a/kernel/Kconfig.sbm
+++ b/kernel/Kconfig.sbm
@@ -29,3 +29,15 @@ config SANDBOX_MODE
 	  and leaving the sandbox.
 
 	  If unsure, say N.
+
+config SBM_KUNIT_TEST
+	tristate "Unit tests for SandBox Mode" if !KUNIT_ALL_TESTS
+	depends on KUNIT && SANDBOX_MODE
+	default KUNIT_ALL_TESTS
+	help
+	  Build unit tests for SandBox Mode.
+
+	  For more information on KUnit and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
diff --git a/kernel/Makefile b/kernel/Makefile
index ecc4bfd6213f..012df6fd817a 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_HAVE_STATIC_CALL_INLINE) += static_call_inline.o
 obj-$(CONFIG_CFI_CLANG) += cfi.o
 obj-$(CONFIG_NUMA) += numa.o
 obj-$(CONFIG_SANDBOX_MODE) += sbm.o
+obj-$(CONFIG_SBM_KUNIT_TEST) += sbm_test.o
 
 obj-$(CONFIG_PERF_EVENTS) += events/
 
diff --git a/kernel/sbm_test.c b/kernel/sbm_test.c
new file mode 100644
index 000000000000..f3ad24ccf332
--- /dev/null
+++ b/kernel/sbm_test.c
@@ -0,0 +1,735 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
+ */
+
+#include <kunit/test.h>
+
+#include <linux/sbm.h>
+#include <linux/sched.h>
+#include <linux/sched/task.h>
+#include <linux/sched/task_stack.h>
+#include <linux/signal.h>
+
+#define PASS	0x600d600d
+#define FAIL	0xbad00bad
+
+struct data {
+	int value;
+};
+
+struct page_over {
+	union {
+		unsigned char page[PAGE_SIZE];
+		struct data data;
+	};
+	int nextpage;
+};
+
+struct thread_data {
+	const struct data *in;
+	struct data *out;
+};
+
+typedef int (*sbm_threadfn)(struct kunit *test, struct thread_data *data);
+
+struct thread_param {
+	struct kunit *test;
+	sbm_threadfn threadfn;
+	struct thread_data *data;
+	int err;
+};
+
+static void check_safe_sbm(struct kunit *test)
+{
+	if (!IS_ENABLED(CONFIG_HAVE_ARCH_SBM))
+		kunit_skip(test, "requires arch hooks");
+}
+
+/**************************************************************
+ * Helpers to handle Oops in a sandbox mode kernel thread
+ *
+ * The kernel does not offer a general method for recovering
+ * from page faults in kernel mode. To intercept a planned
+ * page fault, let a helper thread oops and die.
+ */
+
+static int call_sbm_threadfn(void *arg)
+{
+	struct thread_param *params = arg;
+
+	params->err = params->threadfn(params->test, params->data);
+	do_exit(0);
+}
+
+static int run_sbm_kthread(struct kunit *test, sbm_threadfn threadfn,
+			   struct thread_data *data)
+{
+	struct thread_param params = {
+		.test = test,
+		.threadfn = threadfn,
+		.data = data,
+	};
+	int pid, status;
+
+	/* Do not let the child autoreap. */
+	kernel_sigaction(SIGCHLD, SIG_DFL);
+
+	pid = kernel_thread(call_sbm_threadfn, &params, test->name,
+			    CLONE_FS | CLONE_FILES | SIGCHLD);
+	KUNIT_ASSERT_GT(test, pid, 0);
+	KUNIT_ASSERT_EQ(test, kernel_wait(pid, &status), pid);
+
+	/* Ignore SIGCHLD again. */
+	kernel_sigaction(SIGCHLD, SIG_IGN);
+
+	/* Killed by a signal? */
+	if (status & 0x7f)
+		return -EFAULT;
+
+	KUNIT_ASSERT_EQ(test, status, 0);
+	return params.err;
+}
+
+/**************************************************************
+ * Simple write to output buffer.
+ *
+ * Verify that the output buffer is copied back to the caller.
+ */
+
+static SBM_DEFINE_FUNC(write, struct data *, out)
+{
+	out->value = PASS;
+	return 0;
+}
+
+static void sbm_write(struct kunit *test)
+{
+	struct sbm sbm;
+	struct data out;
+	int err;
+
+	out.value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write,
+		       SBM_COPY_OUT(&sbm, &out, sizeof(out)));
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+	KUNIT_EXPECT_EQ(test, out.value, PASS);
+}
+
+/**************************************************************
+ * Memory write past output buffer within the same page.
+ *
+ * Writes beyond buffer end are ignored.
+ */
+
+static SBM_DEFINE_FUNC(write_past, struct data *, out)
+{
+	out[0].value = PASS;
+	out[1].value = FAIL;
+
+	return 0;
+}
+
+static void sbm_write_past(struct kunit *test)
+{
+	struct sbm sbm;
+	struct data out[2];
+	int err;
+
+	out[0].value = FAIL;
+	out[1].value = PASS;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write_past,
+		       SBM_COPY_OUT(&sbm, &out[0], sizeof(out[0])));
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+	KUNIT_EXPECT_EQ(test, out[0].value, PASS);
+	KUNIT_EXPECT_EQ(test, out[1].value, PASS);
+}
+
+/**************************************************************
+ * Memory write to next page past output buffer.
+ *
+ * There is a guard page after the output buffer. Writes to
+ * this guard page should generate a page fault.
+ */
+
+static SBM_DEFINE_FUNC(write_past_page, struct data *, out)
+{
+	struct page_over *over = (struct page_over *)out;
+
+	over->nextpage = FAIL;
+	barrier();
+	out[0].value = FAIL;
+	return 0;
+}
+
+static int sbm_write_past_page_threadfn(struct kunit *text,
+					struct thread_data *data)
+{
+	struct sbm sbm;
+	int err;
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write_past_page,
+		       SBM_COPY_OUT(&sbm, data->out, sizeof(*data->out)));
+	sbm_destroy(&sbm);
+	return err;
+}
+
+static void sbm_write_past_page(struct kunit *test)
+{
+	struct page_over *over;
+	int err;
+
+	over = kunit_kzalloc(test, sizeof(*over), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, over);
+	over->data.value = PASS;
+	over->nextpage = PASS;
+	if (IS_ENABLED(CONFIG_HAVE_ARCH_SBM)) {
+		struct sbm sbm;
+
+		sbm_init(&sbm);
+		err = sbm_call(&sbm, write_past_page,
+			       SBM_COPY_OUT(&sbm, &over->data,
+					    sizeof(over->data)));
+		KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+		sbm_destroy(&sbm);
+	} else {
+		struct thread_data data;
+
+		data.out = &over->data;
+		err = run_sbm_kthread(test, sbm_write_past_page_threadfn,
+				      &data);
+	}
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, over->data.value, PASS);
+	KUNIT_EXPECT_EQ(test, over->nextpage, PASS);
+}
+
+/**************************************************************
+ * Memory write before output buffer.
+ *
+ * There is a guard page before the output buffer. Writes to
+ * this guard page should generate a page fault.
+ */
+
+static SBM_DEFINE_FUNC(write_before, struct data *, out)
+{
+	out[-1].value = FAIL;
+	barrier();
+	out[0].value = FAIL;
+	return 0;
+}
+
+static int sbm_write_before_threadfn(struct kunit *test,
+				     struct thread_data *data)
+{
+	struct sbm sbm;
+	int err;
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write_before,
+		       SBM_COPY_OUT(&sbm, data->out, sizeof(*data->out)));
+	sbm_destroy(&sbm);
+	return err;
+}
+
+static void sbm_write_before(struct kunit *test)
+{
+	struct data out;
+	int err;
+
+	out.value = PASS;
+	if (IS_ENABLED(CONFIG_HAVE_ARCH_SBM)) {
+		struct sbm sbm;
+
+		sbm_init(&sbm);
+		err = sbm_call(&sbm, write_before,
+			       SBM_COPY_OUT(&sbm, &out, sizeof(out)));
+		KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+		sbm_destroy(&sbm);
+	} else {
+		struct thread_data data;
+
+		data.out = &out;
+		err = run_sbm_kthread(test, sbm_write_before_threadfn, &data);
+	}
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, out.value, PASS);
+}
+
+/**************************************************************
+ * Memory write to kernel static data.
+ *
+ * Sandbox mode cannot write to a kernel static data.
+ */
+
+struct data static_data = { .value = FAIL };
+
+static void sbm_write_static(struct kunit *test)
+{
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write, &static_data);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+	KUNIT_EXPECT_EQ(test, static_data.value, FAIL);
+}
+
+/**************************************************************
+ * Memory write to kernel BSS.
+ *
+ * Sandbox mode cannot write to kernel BSS.
+ */
+
+struct data static_bss;
+
+static void sbm_write_bss(struct kunit *test)
+{
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	static_bss.value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write, &static_bss);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+	KUNIT_EXPECT_EQ(test, static_bss.value, FAIL);
+}
+
+/**************************************************************
+ * Memory write to unrelated buffer.
+ *
+ * Sandbox mode cannot write to the wrong buffer.
+ */
+
+static void sbm_write_wrong(struct kunit *test)
+{
+	struct data *out;
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	out = kunit_kzalloc(test, sizeof(*out), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, out);
+	out->value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write, out);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+	KUNIT_EXPECT_EQ(test, out->value, FAIL);
+}
+
+/**************************************************************
+ * Memory write to kernel stack.
+ *
+ * Sandbox mode runs on its own stack. The kernel stack cannot
+ * be modified.
+ */
+
+static void sbm_write_stack(struct kunit *test)
+{
+	struct data out;
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	out.value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write, &out);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+	KUNIT_EXPECT_EQ(test, out.value, FAIL);
+}
+
+/**************************************************************
+ * Simple update of an input/output buffer.
+ *
+ * Verify that the input buffer is copied in and also back to the caller.
+ */
+
+static SBM_DEFINE_FUNC(update, struct data *, data)
+{
+	data->value ^= FAIL ^ PASS;
+	return 0;
+}
+
+static void sbm_update(struct kunit *test)
+{
+	struct sbm sbm;
+	struct data data;
+	int err;
+
+	data.value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, update,
+		       SBM_COPY_INOUT(&sbm, &data, sizeof(data)));
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+	KUNIT_EXPECT_EQ(test, data.value, PASS);
+}
+
+/**************************************************************
+ * Copy from input buffer to output buffer.
+ *
+ * Verify that sandbox mode can read from the input buffer and
+ * write to the output buffer.
+ */
+
+static int copy_value(const struct data *in, struct data *out)
+{
+	out->value = in->value;
+	return 0;
+}
+
+/*
+ * Define call helper and thunk explicitly to verify that this syntax also
+ * works.
+ */
+static SBM_DEFINE_CALL(copy_value, const struct data *, in,
+		       struct data *, out);
+static SBM_DEFINE_THUNK(copy_value, const struct data *, in,
+			struct data *, out);
+
+static void sbm_copy_value(struct kunit *test)
+{
+	struct sbm sbm;
+	struct data in[1];
+	struct data out[1];
+	int err;
+
+	in[0].value = PASS;
+	out[0].value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, copy_value,
+		       SBM_COPY_IN(&sbm, in, sizeof(in)),
+		       SBM_COPY_OUT(&sbm, out, sizeof(out)));
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+	KUNIT_EXPECT_EQ(test, out[0].value, PASS);
+}
+
+/**************************************************************
+ * Memory read past input buffer within the same page.
+ *
+ * The page beyond the input buffer is initialized to zero.
+ */
+
+static SBM_DEFINE_FUNC(read_past, const struct data *, in)
+{
+	return in[1].value;
+}
+
+static void sbm_read_past(struct kunit *test)
+{
+	struct sbm sbm;
+	struct data in[2];
+	int err;
+
+	in[0].value = PASS;
+	in[1].value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, read_past,
+		       SBM_COPY_IN(&sbm, &in[0], sizeof(in[0])));
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, 0);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+}
+
+/**************************************************************
+ * Memory read from next page past input buffer.
+ *
+ * There is a guard page after the input buffer. Reading from
+ * that page should generate a page fault.
+ */
+
+static SBM_DEFINE_FUNC(read_past_page, const struct data *, in)
+{
+	const struct page_over *over = (const struct page_over *)in;
+
+	return over->nextpage;
+}
+
+static int sbm_read_past_page_threadfn(struct kunit *test,
+				       struct thread_data *data)
+{
+	struct sbm sbm;
+	int err;
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, read_past_page,
+		       SBM_COPY_IN(&sbm, data->in, sizeof(*data->in)));
+	sbm_destroy(&sbm);
+	return err;
+}
+
+static void sbm_read_past_page(struct kunit *test)
+{
+	struct page_over *over;
+	int err;
+
+	over = kunit_kzalloc(test, sizeof(*over), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, over);
+	over->data.value = PASS;
+	over->nextpage = FAIL;
+	if (IS_ENABLED(CONFIG_HAVE_ARCH_SBM)) {
+		struct sbm sbm;
+
+		sbm_init(&sbm);
+		err = sbm_call(&sbm, read_past_page,
+			       SBM_COPY_IN(&sbm, &over->data,
+					   sizeof(over->data)));
+		KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+		sbm_destroy(&sbm);
+	} else {
+		struct thread_data data;
+
+		data.in = &over->data;
+		err = run_sbm_kthread(test, sbm_read_past_page_threadfn, &data);
+	}
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+}
+
+/**************************************************************
+ * Memory read before input buffer.
+ *
+ * There is a guard page before the input buffer. Reading from
+ * that page should generate a page fault.
+ */
+
+static SBM_DEFINE_FUNC(read_before, const struct data *, in)
+{
+	return in[-1].value;
+}
+
+static int sbm_read_before_threadfn(struct kunit *test,
+				    struct thread_data *data)
+{
+	struct sbm sbm;
+	int err;
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, read_before,
+		       SBM_COPY_IN(&sbm, data->in, sizeof(*data->in)));
+	sbm_destroy(&sbm);
+	return err;
+}
+
+static void sbm_read_before(struct kunit *test)
+{
+	struct data in;
+	int err;
+
+	in.value = PASS;
+	if (IS_ENABLED(CONFIG_HAVE_ARCH_SBM)) {
+		struct sbm sbm;
+
+		sbm_init(&sbm);
+		err = sbm_call(&sbm, read_before,
+			       SBM_COPY_IN(&sbm, &in, sizeof(in)));
+		KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+		sbm_destroy(&sbm);
+	} else {
+		struct thread_data data;
+
+		data.in = &in;
+		err = run_sbm_kthread(test, sbm_read_before_threadfn, &data);
+	}
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+}
+
+/**************************************************************
+ * Memory read from unrelated buffer.
+ *
+ * Sandbox mode cannot read from the wrong buffer.
+ */
+
+static SBM_DEFINE_FUNC(read_wrong, const struct data *, in)
+{
+	return in->value;
+}
+
+static void sbm_read_wrong(struct kunit *test)
+{
+	struct data *in;
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	in = kunit_kzalloc(test, sizeof(*in), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, in);
+	in->value = FAIL;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, read_wrong, in);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+}
+
+/**************************************************************
+ * Stack bottom.
+ *
+ * Sandbox mode can read from the bottom of the kernel stack.
+ * Verify that all of the kernel stack is mapped.
+ */
+
+static SBM_DEFINE_FUNC(stack_bottom)
+{
+	return *end_of_stack(current);
+}
+
+static void sbm_stack_bottom(struct kunit *test)
+{
+	struct sbm sbm;
+	unsigned long *bottom;
+	int err;
+
+	bottom = end_of_stack(current);
+	*bottom = PASS;
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, stack_bottom);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, PASS);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), 0);
+	KUNIT_EXPECT_EQ(test, *bottom, PASS);
+}
+
+/**************************************************************
+ * Stack overflow.
+ *
+ * Sandbox mode cannot overflow the stack.
+ *
+ * This test is not safe without SBM arch hooks, because the kernel may panic
+ * when kernel stack overflow is detected.
+ */
+
+static noinline int kaboom(void)
+{
+	return 0;
+}
+
+static SBM_DEFINE_FUNC(stack_overflow)
+{
+	unsigned long old_sp = current_stack_pointer;
+	int err;
+
+	current_stack_pointer = (unsigned long)end_of_stack(current);
+	barrier();
+	err = kaboom();
+	current_stack_pointer = old_sp;
+	return err;
+}
+
+static void sbm_stack_overflow(struct kunit *test)
+{
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, stack_overflow);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+}
+
+#ifdef CONFIG_X86_64
+
+/**************************************************************
+ * [X86-64] Non-canonical address.
+ *
+ * Sandbox mode recovers from a #GP exception. Test it by
+ * memory write to a non-canonical address.
+ */
+
+static void sbm_x86_64_gp(struct kunit *test)
+{
+	void *non_canonical;
+	struct sbm sbm;
+	int err;
+
+	check_safe_sbm(test);
+
+	non_canonical = (void *)(1ULL << 63);
+	sbm_init(&sbm);
+	err = sbm_call(&sbm, write, non_canonical);
+	sbm_destroy(&sbm);
+
+	KUNIT_EXPECT_EQ(test, err, -EFAULT);
+	KUNIT_EXPECT_EQ(test, sbm_error(&sbm), -EFAULT);
+}
+
+#endif
+
+/**************************************************************
+ * Test suite metadata.
+ */
+
+static struct kunit_case sbm_test_cases[] = {
+	KUNIT_CASE(sbm_write),
+	KUNIT_CASE(sbm_write_past),
+	KUNIT_CASE(sbm_write_past_page),
+	KUNIT_CASE(sbm_write_before),
+	KUNIT_CASE(sbm_write_static),
+	KUNIT_CASE(sbm_write_bss),
+	KUNIT_CASE(sbm_write_wrong),
+	KUNIT_CASE(sbm_write_stack),
+	KUNIT_CASE(sbm_copy_value),
+	KUNIT_CASE(sbm_read_past),
+	KUNIT_CASE(sbm_read_past_page),
+	KUNIT_CASE(sbm_read_before),
+	KUNIT_CASE(sbm_read_wrong),
+	KUNIT_CASE(sbm_update),
+	KUNIT_CASE(sbm_stack_bottom),
+	KUNIT_CASE(sbm_stack_overflow),
+#ifdef CONFIG_X86_64
+	KUNIT_CASE(sbm_x86_64_gp),
+#endif
+	{}
+};
+
+static struct kunit_suite sbm_test_suite = {
+	.name = "sandbox_mode",
+	.test_cases = sbm_test_cases,
+};
+
+kunit_test_suites(&sbm_test_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Add a SandBox Mode document under Documentation/security. Describe the
concept, usage and known limitations.

Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 Documentation/security/index.rst        |   1 +
 Documentation/security/sandbox-mode.rst | 180 ++++++++++++++++++++++++
 2 files changed, 181 insertions(+)
 create mode 100644 Documentation/security/sandbox-mode.rst

diff --git a/Documentation/security/index.rst b/Documentation/security/index.rst
index 59f8fc106cb0..680a0b8bf28b 100644
--- a/Documentation/security/index.rst
+++ b/Documentation/security/index.rst
@@ -14,6 +14,7 @@ Security Documentation
    sak
    SCTP
    self-protection
+   sandbox-mode
    siphash
    tpm/index
    digsig
diff --git a/Documentation/security/sandbox-mode.rst b/Documentation/security/sandbox-mode.rst
new file mode 100644
index 000000000000..4405b8858c4a
--- /dev/null
+++ b/Documentation/security/sandbox-mode.rst
@@ -0,0 +1,180 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============
+SandBox Mode
+============
+
+Introduction
+============
+
+The primary goal of SandBox Mode (SBM) is to reduce the impact of potential
+memory safety bugs in kernel code by decomposing the kernel. The SBM API
+allows to run each component inside an isolated execution environment. In
+particular, memory areas used as input and/or output are isolated from the
+rest of the kernel and surrounded by guard pages. Without arch hooks, this
+common base provides *weak isolation*.
+
+On architectures which implement the necessary arch hooks, SandBox Mode
+leverages hardware paging facilities and CPU privilege levels to enforce the
+use of only these predefined memory areas. With arch support, SBM can also
+recover from protection violations. This means that SBM forcibly terminates
+the sandbox and returns an error code (e.g. ``-EFAULT``) to the caller, so
+execution can continue. Such implementation provides *strong isolation*.
+
+A target function in a sandbox communicates with the rest of the kernel
+through a caller-defined interface, comprising read-only buffers (input),
+read-write buffers (output) and the return value. The caller can explicitly
+share other data with the sandbox, but doing so may reduce isolation strength.
+
+Protection of sensitive kernel data is currently out of scope. SandBox Mode is
+meant to run kernel code which would otherwise have full access to all system
+resources. SBM allows to impose a scoped access control policy on which
+resources are available to the sandbox. That said, protection of sensitive
+data is foreseen as a future goal, and that's why the API is designed to
+control not only memory writes but also memory reads.
+
+The expected use case for SandBox Mode is parsing data from untrusted sources,
+especially if the parsing cannot be reasonably done by a user mode helper.
+Keep in mind that a sandbox doesn't guarantee that the output data is correct.
+The result may be corrupt (e.g. as a result of an exploited bug) and where
+applicable, it should be sanitized before further use.
+
+Using SandBox Mode
+==================
+
+SandBox Mode is an optional feature, enabled with ``CONFIG_SANDBOX_MODE``.
+However, the SBM API is always defined regardless of the kernel configuration.
+It will call a function with the best available isolation, which is:
+
+* *strong isolation* if both ``CONFIG_SANDBOX_MODE`` and
+  ``CONFIG_ARCH_HAVE_SBM`` are set,
+* *weak isolation* if ``CONFIG_SANDBOX_MODE`` is set, but
+  ``CONFIG_ARCH_HAVE_SBM`` is unset,
+* *no isolation* if ``CONFIG_SANDBOX_MODE`` is unset.
+
+Code which cannot safely run with no isolation should depend on the relevant
+config option(s).
+
+The API can be used like this:
+
+.. code-block:: c
+
+  #include <linux/sbm.h>
+
+  /* Function to be executed in a sandbox. */
+  static SBM_DEFINE_FUNC(my_func, const struct my_input *, in,
+			 struct my_output *, out)
+  {
+	/* Read from in, write to out. */
+	return 0;
+  }
+
+  int caller(...)
+  {
+	/* Declare a SBM instance. */
+	struct sbm sbm;
+
+	/* Initialize SBM instance. */
+	sbm_init(&sbm);
+
+	/* Execute my_func() using the SBM instance. */
+	err = sbm_call(&sbm, my_func,
+		       SBM_COPY_IN(&sbm, input, in_size),
+		       SBM_COPY_OUT(&sbm, output, out_size));
+
+	/* Clean up. */
+	sbm_destroy(&sbm);
+
+The return type of a sandbox mode function is always ``int``. The return value
+is zero on success and negative on error. That's because the SBM helpers
+return an error code (such as ``-ENOMEM``) if the call cannot be performed.
+
+If sbm_call() returns an error, you can use sbm_error() to decide whether the
+error was returned by the target function or because sandbox mode was aborted
+(or failed to run entirely).
+
+Public API
+----------
+
+.. kernel-doc:: include/linux/sbm.h
+		:identifiers: sbm sbm_init sbm_destroy sbm_exec sbm_error
+			      SBM_COPY_IN SBM_COPY_OUT SBM_COPY_INOUT
+			      SBM_DEFINE_CALL SBM_DEFINE_THUNK SBM_DEFINE_FUNC
+			      sbm_call
+
+Arch Hooks
+----------
+
+These hooks must be implemented to select HAVE_ARCH_SBM.
+
+.. kernel-doc:: include/linux/sbm.h
+		:identifiers: arch_sbm_init arch_sbm_destroy arch_sbm_exec
+			      arch_sbm_map_readonly arch_sbm_map_writable
+
+Current Limitations
+===================
+
+This section lists know limitations of the current SBM implementation, which
+are planned to be removed in the future.
+
+Stack
+-----
+
+There is no generic kernel API to run a function on an alternate stack, so SBM
+runs on the normal kernel stack by default. The kernel already offers
+self-protection against stack overflows and underflows as well as against
+overwriting on-stack data outside the current frame, but violations are
+usually fatal.
+
+This limitation can be solved for specific targets. Arch hooks can set up a
+separate stack and recover from stack frame overruns.
+
+Inherent Limitations
+====================
+
+This section lists limitations which are inherent to the concept.
+
+Explicit Code
+-------------
+
+The main idea behind SandBox Mode is decomposition of one big program (the
+Linux kernel) into multiple smaller programs that can be sandboxed. AFAIK
+there is no way to automate this task for an existing code base in C.
+
+Given the performance impact of running code in a sandbox, this limitation may
+be perceived as a benefit. It is expected that sandbox mode is introduced only
+knowingly and only where safety is more important than performance.
+
+Complex Data
+------------
+
+Although data structures are not serialized and deserialized between kernel
+mode and sandbox mode, all directly and indirectly referenced data structures
+must be explicitly mapped into the sandbox, which requires some manual effort.
+
+Copying of input/output buffers also incurs some runtime overhead. This
+overhead can be reduced by sharing data directly with the sandbox, but the
+resulting isolation is weaker, so it may or may not be acceptable, depending
+on the overall safety requirements.
+
+Page Granularity
+----------------
+
+Since paging is used to enforce memory safety, page size is the smallest unit.
+Objects mapped into the sandbox must be aligned to a page boundary, and buffer
+overflows may not be detected if they fit into the same page.
+
+On the other hand, even though such writes are not detected, they do not
+corrupt kernel data, because only the output buffer is copied back to kernel
+mode, and the (corrupted) rest of the page is ignored.
+
+Transitions
+-----------
+
+Transitions between kernel mode and sandbox mode are synchronous. That is,
+whenever entering or leaving sandbox mode, the currently running CPU executes
+the instructions necessary to save/restore its kernel-mode state. The API is
+generic enough to allow asynchronous transitions, e.g. to pass data to another
+CPU which is already running in sandbox mode. However, to see the benefits, a
+hypothetical implementation would require far-reaching changes in the kernel
+scheduler. This is (currently) out of scope.
-- 
2.34.1

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Andrew Morton]

On Wed, 14 Feb 2024 12:30:35 +0100 Petr Tesarik <petrtesarik@huaweicloud.com> wrote:

> +Although data structures are not serialized and deserialized between kernel
> +mode and sandbox mode, all directly and indirectly referenced data structures
> +must be explicitly mapped into the sandbox, which requires some manual effort.

Maybe I'm missing something here, but...

The requirement that the sandboxed function only ever touch two linear
blocks of memory (yes?) seems a tremendous limitation.  I mean, how can
the sandboxed function call kmalloc()?  How can it call any useful
kernel functions?  They'll all touch memory which lies outside the
sandbox areas?

Perhaps a simple but real-world example would help clarify.

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Greg Kroah-Hartman]

On Wed, Feb 14, 2024 at 05:30:53AM -0800, Andrew Morton wrote:
> On Wed, 14 Feb 2024 12:30:35 +0100 Petr Tesarik <petrtesarik@huaweicloud.com> wrote:
> 
> > +Although data structures are not serialized and deserialized between kernel
> > +mode and sandbox mode, all directly and indirectly referenced data structures
> > +must be explicitly mapped into the sandbox, which requires some manual effort.
> 
> Maybe I'm missing something here, but...
> 
> The requirement that the sandboxed function only ever touch two linear
> blocks of memory (yes?) seems a tremendous limitation.  I mean, how can
> the sandboxed function call kmalloc()?  How can it call any useful
> kernel functions?  They'll all touch memory which lies outside the
> sandbox areas?
> 
> Perhaps a simple but real-world example would help clarify.

I agree, this looks like an "interesting" framework, but we don't add
code to the kernel without a real, in-kernel user for it.

Without such a thing, we can't even consider it for inclusion as we
don't know how it will actually work and how any subsystem would use it.

Petr, do you have an user for this today?

thanks,

greg k-h

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Wed, 14 Feb 2024 15:01:25 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Feb 14, 2024 at 05:30:53AM -0800, Andrew Morton wrote:
> > On Wed, 14 Feb 2024 12:30:35 +0100 Petr Tesarik <petrtesarik@huaweicloud.com> wrote:
> >   
> > > +Although data structures are not serialized and deserialized between kernel
> > > +mode and sandbox mode, all directly and indirectly referenced data structures
> > > +must be explicitly mapped into the sandbox, which requires some manual effort.  
> > 
> > Maybe I'm missing something here, but...
> > 
> > The requirement that the sandboxed function only ever touch two linear
> > blocks of memory (yes?) seems a tremendous limitation.  I mean, how can
> > the sandboxed function call kmalloc()?  How can it call any useful
> > kernel functions?  They'll all touch memory which lies outside the
> > sandbox areas?
> > 
> > Perhaps a simple but real-world example would help clarify.  
> 
> I agree, this looks like an "interesting" framework, but we don't add
> code to the kernel without a real, in-kernel user for it.
> 
> Without such a thing, we can't even consider it for inclusion as we
> don't know how it will actually work and how any subsystem would use it.
> 
> Petr, do you have an user for this today?

Hi Greg & Andrew,

your observations is correct. In this form, the framework is quite
limited, and exactly this objections was expected. You have even
spotted one of the first enhancements I tested on top of this framework
(dynamic memory allocation).

The intended use case is code that processes untrusted data that is not
properly sanitized, but where performance is not critical. Some
examples include decompressing initramfs, loading a kernel module. Or
decoding a boot logo; I think I've noticed a vulnerability in another
project recently... ;-)

Of course, even decompression needs dynamic memory. My plan is to
extend the mechanism. Right now I'm mapping all of kernel text into the
sandbox. Later, I'd like to decompose the text section too. The pages
which contain sandboxed code should be mapped, but rest of the kernel
should not. If the sandbox tries to call kmalloc(), vmalloc(), or
schedule(), the attempt will generate a page fault. Sandbox page faults
are already intercepted, so handle_sbm_call() can decide if the call
should be allowed or not. If the sandbox policy says ALLOW, the page
fault handler will perform the API call on behalf of the sandboxed code
and return results, possibly with some post-call action, e.g. map some
more pages to the address space.

The fact that all communication with the rest of the kernel happens
through CPU exceptions is the reason this mechanism is unsuitable for
performance-critical applications.

OK, so why didn't I send the whole thing?

Decomposition of the kernel requires many more changes, e.g. in linker
scripts. Some of them depend on this patch series. Before I go and
clean up my code into something that can be submitted, I want to get
feedback from guys like you, to know if the whole idea would be even
considered, aka "Fail Fast".

Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Greg Kroah-Hartman]

On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:
> OK, so why didn't I send the whole thing?
> 
> Decomposition of the kernel requires many more changes, e.g. in linker
> scripts. Some of them depend on this patch series. Before I go and
> clean up my code into something that can be submitted, I want to get
> feedback from guys like you, to know if the whole idea would be even
> considered, aka "Fail Fast".

We can't honestly consider this portion without seeing how it would
work, as we don't even see a working implementation that uses it to
verify it at all.

The joy of adding new frameworks is that you need a user before anyone
can spend the time to review it, sorry.

thanks,

greg k-h

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Wed, 14 Feb 2024 16:11:05 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:
> > OK, so why didn't I send the whole thing?
> > 
> > Decomposition of the kernel requires many more changes, e.g. in linker
> > scripts. Some of them depend on this patch series. Before I go and
> > clean up my code into something that can be submitted, I want to get
> > feedback from guys like you, to know if the whole idea would be even
> > considered, aka "Fail Fast".  
> 
> We can't honestly consider this portion without seeing how it would
> work, as we don't even see a working implementation that uses it to
> verify it at all.
> 
> The joy of adding new frameworks is that you need a user before anyone
> can spend the time to review it, sorry.

Thank your for a quick assessment. Will it be sufficient if I send some
code for illustration (with some quick&dirty hacks to bridge the gaps),
or do you need clean and nice kernel code?

Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Greg Kroah-Hartman]

On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> On Wed, 14 Feb 2024 16:11:05 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:
> > > OK, so why didn't I send the whole thing?
> > > 
> > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > scripts. Some of them depend on this patch series. Before I go and
> > > clean up my code into something that can be submitted, I want to get
> > > feedback from guys like you, to know if the whole idea would be even
> > > considered, aka "Fail Fast".  
> > 
> > We can't honestly consider this portion without seeing how it would
> > work, as we don't even see a working implementation that uses it to
> > verify it at all.
> > 
> > The joy of adding new frameworks is that you need a user before anyone
> > can spend the time to review it, sorry.
> 
> Thank your for a quick assessment. Will it be sufficient if I send some
> code for illustration (with some quick&dirty hacks to bridge the gaps),
> or do you need clean and nice kernel code?

We need a real user in the kernel, otherwise why would we even consider
it?  Would you want to review a new subsystem that does nothing and has
no real users?  If not, why would you want us to?  :)

thanks,

greg k-h

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Kent Overstreet]

On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> On Wed, 14 Feb 2024 16:11:05 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:
> > > OK, so why didn't I send the whole thing?
> > > 
> > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > scripts. Some of them depend on this patch series. Before I go and
> > > clean up my code into something that can be submitted, I want to get
> > > feedback from guys like you, to know if the whole idea would be even
> > > considered, aka "Fail Fast".  
> > 
> > We can't honestly consider this portion without seeing how it would
> > work, as we don't even see a working implementation that uses it to
> > verify it at all.
> > 
> > The joy of adding new frameworks is that you need a user before anyone
> > can spend the time to review it, sorry.
> 
> Thank your for a quick assessment. Will it be sufficient if I send some
> code for illustration (with some quick&dirty hacks to bridge the gaps),
> or do you need clean and nice kernel code?

Given that code is going to need a rewrite to make use of this anyways -
why not just do the rewrite in Rust?

Then you get memory safety, which seems to be what you're trying to
achieve here.

Or, you say this is for when performance isn't critical - why not a user
mode helper?

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Wed, 14 Feb 2024 19:48:52 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> > On Wed, 14 Feb 2024 16:11:05 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:  
> > > > OK, so why didn't I send the whole thing?
> > > > 
> > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > scripts. Some of them depend on this patch series. Before I go and
> > > > clean up my code into something that can be submitted, I want to get
> > > > feedback from guys like you, to know if the whole idea would be even
> > > > considered, aka "Fail Fast".    
> > > 
> > > We can't honestly consider this portion without seeing how it would
> > > work, as we don't even see a working implementation that uses it to
> > > verify it at all.
> > > 
> > > The joy of adding new frameworks is that you need a user before anyone
> > > can spend the time to review it, sorry.  
> > 
> > Thank your for a quick assessment. Will it be sufficient if I send some
> > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > or do you need clean and nice kernel code?  
> 
> We need a real user in the kernel, otherwise why would we even consider
> it?  Would you want to review a new subsystem that does nothing and has
> no real users?  If not, why would you want us to?  :)

Greg, please enlighten me on the process. How is something like this
supposed to get in?

Subsystem maintainers will not review code that depends on core features
not yet reviewed by the respective maintainers. If I add only the API
and a stub implementation, then it brings no benefit and attempts to
introduce the API will be dismissed. I would certainly do just that if
I was a maintainer...

I could try to pack everything (base infrastructure, arch
implementations, API users) into one big patch with pretty much
everybody on the Cc list, but how is that ever going to get reviewed?

Should I just go and maintain an out-of-tree repo for a few years,
hoping that it gets merged one day, like bcachefs? Is this the way?

Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Wed, 14 Feb 2024 13:54:54 -0500
Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> > On Wed, 14 Feb 2024 16:11:05 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:  
> > > > OK, so why didn't I send the whole thing?
> > > > 
> > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > scripts. Some of them depend on this patch series. Before I go and
> > > > clean up my code into something that can be submitted, I want to get
> > > > feedback from guys like you, to know if the whole idea would be even
> > > > considered, aka "Fail Fast".    
> > > 
> > > We can't honestly consider this portion without seeing how it would
> > > work, as we don't even see a working implementation that uses it to
> > > verify it at all.
> > > 
> > > The joy of adding new frameworks is that you need a user before anyone
> > > can spend the time to review it, sorry.  
> > 
> > Thank your for a quick assessment. Will it be sufficient if I send some
> > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > or do you need clean and nice kernel code?  
> 
> Given that code is going to need a rewrite to make use of this anyways -
> why not just do the rewrite in Rust?

Thank you for this question! I concur that rewriting the whole kernel
in Rust would be a better option. I see two differences:

1. amount of work
2. regressions

Rewriting something in Rust means pretty much writing it from scratch.
Doing that necessarily introduces regressions. Old code has been used.
It has been tested. In many corner cases. Lots of bugs have been found,
and they’ve been fixed. If you write code from scratch, you lose much
of the accumulated knowledge.

It may still pay off in the long run.

More importantly, sandbox mode can be viewed as a tool that enforces
decomposition of kernel code. This decomposition is the main benefit.
It requires understanding the dependencies among different parts of the
kernel (both code flow and data flow), and that will in turn promote
better design.

> Then you get memory safety, which seems to be what you're trying to
> achieve here.
> 
> Or, you say this is for when performance isn't critical - why not a user
> mode helper?

Processes in user mode are susceptible to all kinds of attacks you may
want to avoid. Sandbox mode can be used in situations where user mode
does not exist, e.g. to display a boot logo or to unpack initramfs.

Sandbox mode is part of the kernel, hence signed, which may be relevant
if the kernel is locked down, so you can use it e.g. to parse a key
from the bootloader and put it on the trusted keyring.

Regarding performance, sandbox overhead is somewhere between kernel
mode and UMH. It is not suitable for time-critical code (like handling
NIC interrupts), but it's still much faster than UMH.

Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Kent Overstreet]

On Wed, Feb 14, 2024 at 09:09:37PM +0100, Petr Tesařík wrote:
> On Wed, 14 Feb 2024 13:54:54 -0500
> Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> > On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> > > On Wed, 14 Feb 2024 16:11:05 +0100
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:  
> > > > > OK, so why didn't I send the whole thing?
> > > > > 
> > > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > > scripts. Some of them depend on this patch series. Before I go and
> > > > > clean up my code into something that can be submitted, I want to get
> > > > > feedback from guys like you, to know if the whole idea would be even
> > > > > considered, aka "Fail Fast".    
> > > > 
> > > > We can't honestly consider this portion without seeing how it would
> > > > work, as we don't even see a working implementation that uses it to
> > > > verify it at all.
> > > > 
> > > > The joy of adding new frameworks is that you need a user before anyone
> > > > can spend the time to review it, sorry.  
> > > 
> > > Thank your for a quick assessment. Will it be sufficient if I send some
> > > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > > or do you need clean and nice kernel code?  
> > 
> > Given that code is going to need a rewrite to make use of this anyways -
> > why not just do the rewrite in Rust?
> 
> Thank you for this question! I concur that rewriting the whole kernel
> in Rust would be a better option. I see two differences:
> 
> 1. amount of work
> 2. regressions
> 
> Rewriting something in Rust means pretty much writing it from scratch.
> Doing that necessarily introduces regressions. Old code has been used.
> It has been tested. In many corner cases. Lots of bugs have been found,
> and they’ve been fixed. If you write code from scratch, you lose much
> of the accumulated knowledge.

But it's work that already has some growing momentum behind it,
especially in the area you cited - decompression algorithms.

> More importantly, sandbox mode can be viewed as a tool that enforces
> decomposition of kernel code. This decomposition is the main benefit.
> It requires understanding the dependencies among different parts of the
> kernel (both code flow and data flow), and that will in turn promote
> better design.

You see this as a tool for general purpose code...?

Typical kernel code tends to be quite pointer heavy.

> > Then you get memory safety, which seems to be what you're trying to
> > achieve here.
> > 
> > Or, you say this is for when performance isn't critical - why not a user
> > mode helper?
> 
> Processes in user mode are susceptible to all kinds of attacks you may
> want to avoid. Sandbox mode can be used in situations where user mode
> does not exist, e.g. to display a boot logo or to unpack initramfs.

[citation needed]

Running code in the kernel does not make it more secure from attack, and
that's a pretty strange idea. One of the central jobs of the kernel is
to provide isolation between different users.

> Sandbox mode is part of the kernel, hence signed, which may be relevant
> if the kernel is locked down, so you can use it e.g. to parse a key
> from the bootloader and put it on the trusted keyring.
> 
> Regarding performance, sandbox overhead is somewhere between kernel
> mode and UMH. It is not suitable for time-critical code (like handling
> NIC interrupts), but it's still much faster than UMH.

yeah, this doesn't seem like a worthwhile direction to go in.

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Wed, 14 Feb 2024 15:19:04 -0500
Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Wed, Feb 14, 2024 at 09:09:37PM +0100, Petr Tesařík wrote:
> > On Wed, 14 Feb 2024 13:54:54 -0500
> > Kent Overstreet <kent.overstreet@linux.dev> wrote:
> >   
> > > On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:  
> > > > On Wed, 14 Feb 2024 16:11:05 +0100
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > >     
> > > > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:    
> > > > > > OK, so why didn't I send the whole thing?
> > > > > > 
> > > > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > > > scripts. Some of them depend on this patch series. Before I go and
> > > > > > clean up my code into something that can be submitted, I want to get
> > > > > > feedback from guys like you, to know if the whole idea would be even
> > > > > > considered, aka "Fail Fast".      
> > > > > 
> > > > > We can't honestly consider this portion without seeing how it would
> > > > > work, as we don't even see a working implementation that uses it to
> > > > > verify it at all.
> > > > > 
> > > > > The joy of adding new frameworks is that you need a user before anyone
> > > > > can spend the time to review it, sorry.    
> > > > 
> > > > Thank your for a quick assessment. Will it be sufficient if I send some
> > > > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > > > or do you need clean and nice kernel code?    
> > > 
> > > Given that code is going to need a rewrite to make use of this anyways -
> > > why not just do the rewrite in Rust?  
> > 
> > Thank you for this question! I concur that rewriting the whole kernel
> > in Rust would be a better option. I see two differences:
> > 
> > 1. amount of work
> > 2. regressions
> > 
> > Rewriting something in Rust means pretty much writing it from scratch.
> > Doing that necessarily introduces regressions. Old code has been used.
> > It has been tested. In many corner cases. Lots of bugs have been found,
> > and they’ve been fixed. If you write code from scratch, you lose much
> > of the accumulated knowledge.  
> 
> But it's work that already has some growing momentum behind it,
> especially in the area you cited - decompression algorithms.

Fair enough, this is indeed going for a better solution.

> > More importantly, sandbox mode can be viewed as a tool that enforces
> > decomposition of kernel code. This decomposition is the main benefit.
> > It requires understanding the dependencies among different parts of the
> > kernel (both code flow and data flow), and that will in turn promote
> > better design.  
> 
> You see this as a tool for general purpose code...?
> 
> Typical kernel code tends to be quite pointer heavy.

Yes. I believe this fact contributes to the difficulty of ensuring
memory safety in the kernel. With so much code potentially depnding on
any other kernel data structure, it does not help much that you protect
it as a whole. A finer-grained protection would make more sense.

> > > Then you get memory safety, which seems to be what you're trying to
> > > achieve here.
> > > 
> > > Or, you say this is for when performance isn't critical - why not a user
> > > mode helper?  
> > 
> > Processes in user mode are susceptible to all kinds of attacks you may
> > want to avoid. Sandbox mode can be used in situations where user mode
> > does not exist, e.g. to display a boot logo or to unpack initramfs.  
> 
> [citation needed]

I assume you mean citation for the kinds of attacks, not for the
unavailability of user space before initramfs is unpacked. ;-)

Here you go:

On Wed, 2023-03-22 at 15:27 -0700, Alexei Starovoitov wrote:
> On Wed, Mar 22, 2023 at 5:08 AM Roberto Sassu
[...]
> <roberto.sassu@huaweicloud.com> wrote:
> > possible use case. The main goal is to move something that was running
> > in the kernel to user space, with the same isolation guarantees as if
> > the code was executed in the kernel.
> 
> They are not the same guarantees.
> UMD is exactly equivalent to root process running in user space.
> Meaning it can be killed, ptraced, priority inverted, etc

https://lore.kernel.org/lkml/CAADnVQJC0h7rtuntt0tqS5BbxWsmyWs3ZSbboZMmUKetMG2VhA@mail.gmail.com/

Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Roberto Sassu]

On Wed, 2024-02-14 at 15:19 -0500, Kent Overstreet wrote:
> On Wed, Feb 14, 2024 at 09:09:37PM +0100, Petr Tesařík wrote:
> > On Wed, 14 Feb 2024 13:54:54 -0500
> > Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > 
> > > On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> > > > On Wed, 14 Feb 2024 16:11:05 +0100
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > >   
> > > > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:  
> > > > > > OK, so why didn't I send the whole thing?
> > > > > > 
> > > > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > > > scripts. Some of them depend on this patch series. Before I go and
> > > > > > clean up my code into something that can be submitted, I want to get
> > > > > > feedback from guys like you, to know if the whole idea would be even
> > > > > > considered, aka "Fail Fast".    
> > > > > 
> > > > > We can't honestly consider this portion without seeing how it would
> > > > > work, as we don't even see a working implementation that uses it to
> > > > > verify it at all.
> > > > > 
> > > > > The joy of adding new frameworks is that you need a user before anyone
> > > > > can spend the time to review it, sorry.  
> > > > 
> > > > Thank your for a quick assessment. Will it be sufficient if I send some
> > > > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > > > or do you need clean and nice kernel code?  
> > > 
> > > Given that code is going to need a rewrite to make use of this anyways -
> > > why not just do the rewrite in Rust?
> > 
> > Thank you for this question! I concur that rewriting the whole kernel
> > in Rust would be a better option. I see two differences:
> > 
> > 1. amount of work
> > 2. regressions
> > 
> > Rewriting something in Rust means pretty much writing it from scratch.
> > Doing that necessarily introduces regressions. Old code has been used.
> > It has been tested. In many corner cases. Lots of bugs have been found,
> > and they’ve been fixed. If you write code from scratch, you lose much
> > of the accumulated knowledge.
> 
> But it's work that already has some growing momentum behind it,
> especially in the area you cited - decompression algorithms.
> 
> > More importantly, sandbox mode can be viewed as a tool that enforces
> > decomposition of kernel code. This decomposition is the main benefit.
> > It requires understanding the dependencies among different parts of the
> > kernel (both code flow and data flow), and that will in turn promote
> > better design.
> 
> You see this as a tool for general purpose code...?
> 
> Typical kernel code tends to be quite pointer heavy.
> 
> > > Then you get memory safety, which seems to be what you're trying to
> > > achieve here.
> > > 
> > > Or, you say this is for when performance isn't critical - why not a user
> > > mode helper?
> > 
> > Processes in user mode are susceptible to all kinds of attacks you may
> > want to avoid. Sandbox mode can be used in situations where user mode
> > does not exist, e.g. to display a boot logo or to unpack initramfs.
> 
> [citation needed]
> 
> Running code in the kernel does not make it more secure from attack, and
> that's a pretty strange idea. One of the central jobs of the kernel is
> to provide isolation between different users.

+ linux-security-module, keyrings

It is not exactly about being more secure, but more privileged.

I had a question in the past:

https://lore.kernel.org/linux-integrity/eb31920bd00e2c921b0aa6ebed8745cb0130b0e1.camel@huaweicloud.com/


I basically need to parse PGP keys, to verify RPM package headers,
extract the file digests from them, and use those digests as reference
values for the kernel to decide whether or not files can be accessed
depending on their integrity.

It is very important that, in a locked-down system, even root is
subject to integrity checks. So, the kernel has higher privileges than
root.

Can the kernel be trusted to provide strong enough process isolation,
even against root, in a way that it can offload a workload to a user
space process (PGP parsing), and yet maintain its privilege level
(which would drop to root, if isolation cannot be guaranteed)?

So, since we got no as an answer, we thought about something in the
middle, we still run the code in the kernel, to keep the higher
privilege, but at the same time we mitigate the risk of kernel memory
corruption.

Roberto

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Greg Kroah-Hartman]

On Wed, Feb 14, 2024 at 08:42:54PM +0100, Petr Tesařík wrote:
> On Wed, 14 Feb 2024 19:48:52 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:
> > > On Wed, 14 Feb 2024 16:11:05 +0100
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > >   
> > > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:  
> > > > > OK, so why didn't I send the whole thing?
> > > > > 
> > > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > > scripts. Some of them depend on this patch series. Before I go and
> > > > > clean up my code into something that can be submitted, I want to get
> > > > > feedback from guys like you, to know if the whole idea would be even
> > > > > considered, aka "Fail Fast".    
> > > > 
> > > > We can't honestly consider this portion without seeing how it would
> > > > work, as we don't even see a working implementation that uses it to
> > > > verify it at all.
> > > > 
> > > > The joy of adding new frameworks is that you need a user before anyone
> > > > can spend the time to review it, sorry.  
> > > 
> > > Thank your for a quick assessment. Will it be sufficient if I send some
> > > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > > or do you need clean and nice kernel code?  
> > 
> > We need a real user in the kernel, otherwise why would we even consider
> > it?  Would you want to review a new subsystem that does nothing and has
> > no real users?  If not, why would you want us to?  :)
> 
> Greg, please enlighten me on the process. How is something like this
> supposed to get in?

If you were in our shoes, what would you want to see in order to be able
to properly review and judge if a new subsystem was ok to accept?

> Subsystem maintainers will not review code that depends on core features
> not yet reviewed by the respective maintainers. If I add only the API
> and a stub implementation, then it brings no benefit and attempts to
> introduce the API will be dismissed. I would certainly do just that if
> I was a maintainer...

Exactly, you need a real user.

> I could try to pack everything (base infrastructure, arch
> implementations, API users) into one big patch with pretty much
> everybody on the Cc list, but how is that ever going to get reviewed?

How are we supposed to know if any of this even works at all if you
don't show that it actually works and is useful?  Has any of that work
even been done yet?  I'm guessing it has (otherwise you wouldn't have
posted this), but you are expecting us to just "trust us, stuff in the
future is going to use this and need it" here.

Again, we can not add new infrastructure for things that have no users,
nor do you want us to.  Ideally you will have at least 3 different
users, as that seems to be the "magic number" that shows that the
api/interface will actually work well, and is flexible enough.  Just
one user is great for proof-of-concept, but that usually isn't good
enough to determine if it will work for others (and so it wouldn't need
to be infrastructure at all, but rather just part of that one feature on
its own.)

> Should I just go and maintain an out-of-tree repo for a few years,
> hoping that it gets merged one day, like bcachefs? Is this the way?

No, show us how this is going to be used.

Again, think about what you would want if you had to review this.

thanks,

greg k-h

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Petr Tesařík]

On Thu, 15 Feb 2024 10:11:05 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Wed, Feb 14, 2024 at 08:42:54PM +0100, Petr Tesařík wrote:
> > On Wed, 14 Feb 2024 19:48:52 +0100
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Wed, Feb 14, 2024 at 05:31:12PM +0100, Petr Tesařík wrote:  
> > > > On Wed, 14 Feb 2024 16:11:05 +0100
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > > >     
> > > > > On Wed, Feb 14, 2024 at 03:55:24PM +0100, Petr Tesařík wrote:    
> > > > > > OK, so why didn't I send the whole thing?
> > > > > > 
> > > > > > Decomposition of the kernel requires many more changes, e.g. in linker
> > > > > > scripts. Some of them depend on this patch series. Before I go and
> > > > > > clean up my code into something that can be submitted, I want to get
> > > > > > feedback from guys like you, to know if the whole idea would be even
> > > > > > considered, aka "Fail Fast".      
> > > > > 
> > > > > We can't honestly consider this portion without seeing how it would
> > > > > work, as we don't even see a working implementation that uses it to
> > > > > verify it at all.
> > > > > 
> > > > > The joy of adding new frameworks is that you need a user before anyone
> > > > > can spend the time to review it, sorry.    
> > > > 
> > > > Thank your for a quick assessment. Will it be sufficient if I send some
> > > > code for illustration (with some quick&dirty hacks to bridge the gaps),
> > > > or do you need clean and nice kernel code?    
> > > 
> > > We need a real user in the kernel, otherwise why would we even consider
> > > it?  Would you want to review a new subsystem that does nothing and has
> > > no real users?  If not, why would you want us to?  :)  
> > 
> > Greg, please enlighten me on the process. How is something like this
> > supposed to get in?  
> 
> If you were in our shoes, what would you want to see in order to be able
> to properly review and judge if a new subsystem was ok to accept?
> 
> > Subsystem maintainers will not review code that depends on core features
> > not yet reviewed by the respective maintainers. If I add only the API
> > and a stub implementation, then it brings no benefit and attempts to
> > introduce the API will be dismissed. I would certainly do just that if
> > I was a maintainer...  
> 
> Exactly, you need a real user.

Er, what I was trying to say was rather: You need a real implementation
of a core feature before a subsystem maintainer considers using it for
their subsystem.

But I get your point. I need *both*.

> > I could try to pack everything (base infrastructure, arch
> > implementations, API users) into one big patch with pretty much
> > everybody on the Cc list, but how is that ever going to get reviewed?  
> 
> How are we supposed to know if any of this even works at all if you
> don't show that it actually works and is useful?  Has any of that work
> even been done yet?  I'm guessing it has (otherwise you wouldn't have
> posted this), but you are expecting us to just "trust us, stuff in the
> future is going to use this and need it" here.

Understood.

> Again, we can not add new infrastructure for things that have no users,
> nor do you want us to.  Ideally you will have at least 3 different
> users, as that seems to be the "magic number" that shows that the
> api/interface will actually work well, and is flexible enough.  Just
> one user is great for proof-of-concept, but that usually isn't good
> enough to determine if it will work for others (and so it wouldn't need
> to be infrastructure at all, but rather just part of that one feature on
> its own.)
> 
> > Should I just go and maintain an out-of-tree repo for a few years,
> > hoping that it gets merged one day, like bcachefs? Is this the way?  
> 
> No, show us how this is going to be used.

OK, working on it.

> Again, think about what you would want if you had to review this.

Review, or merge? For a review, I would want enough information to
understand what it is *and* where it is going.

As a matter of fact, hpa does not like the x86 implementation. For
reasons that I do not fully understand (yet), but if the concept turns
out to be impractical, then my submission will serve a purpose, as I
can save myself (and anybody else with a similar idea) a lot of work by
failing fast.

Is this a valid way to get early feedback?

Thanks,
Petr T

Re: [PATCH v1 5/5] sbm: SandBox Mode documentation

[Greg Kroah-Hartman]

On Thu, Feb 15, 2024 at 10:45:15AM +0100, Petr Tesařík wrote:
> As a matter of fact, hpa does not like the x86 implementation. For
> reasons that I do not fully understand (yet), but if the concept turns
> out to be impractical, then my submission will serve a purpose, as I
> can save myself (and anybody else with a similar idea) a lot of work by
> failing fast.
> 
> Is this a valid way to get early feedback?

Asking for feedback is one thing (that's what RFC is for, right?), but
submitting something and expecting us to review and accept it as-is like
this, just doesn't work well.

thanks,

greg k-h

Re: [PATCH v1 4/5] sbm: SandBox Mode KUnit test suite

[kernel test robot]

Hi Petr,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v6.8-rc4 next-20240215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Petr-Tesarik/sbm-SandBox-Mode-core-data-types-and-functions/20240214-193528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240214113035.2117-5-petrtesarik%40huaweicloud.com
patch subject: [PATCH v1 4/5] sbm: SandBox Mode KUnit test suite
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240216/202402160357.4hi1ecdG-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 1c10821022f1799452065fb57474e894e2562b7f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240216/202402160357.4hi1ecdG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402160357.4hi1ecdG-lkp@intel.com/

All errors (new ones prefixed by >>):

>> kernel/sbm_test.c:648:25: error: use of undeclared identifier 'current_stack_pointer'
     648 |         unsigned long old_sp = current_stack_pointer;
         |                                ^
   kernel/sbm_test.c:651:2: error: use of undeclared identifier 'current_stack_pointer'
     651 |         current_stack_pointer = (unsigned long)end_of_stack(current);
         |         ^
   kernel/sbm_test.c:654:2: error: use of undeclared identifier 'current_stack_pointer'
     654 |         current_stack_pointer = old_sp;
         |         ^
   3 errors generated.


vim +/current_stack_pointer +648 kernel/sbm_test.c

   645	
   646	static SBM_DEFINE_FUNC(stack_overflow)
   647	{
 > 648		unsigned long old_sp = current_stack_pointer;
   649		int err;
   650	
   651		current_stack_pointer = (unsigned long)end_of_stack(current);
   652		barrier();
   653		err = kaboom();
   654		current_stack_pointer = old_sp;
   655		return err;
   656	}
   657	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 4/5] sbm: SandBox Mode KUnit test suite

[kernel test robot]

Hi Petr,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]
[also build test WARNING on linus/master v6.8-rc4 next-20240215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Petr-Tesarik/sbm-SandBox-Mode-core-data-types-and-functions/20240214-193528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240214113035.2117-5-petrtesarik%40huaweicloud.com
patch subject: [PATCH v1 4/5] sbm: SandBox Mode KUnit test suite
config: mips-randconfig-r121-20240215 (https://download.01.org/0day-ci/archive/20240216/202402160907.r0qgAoRs-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240216/202402160907.r0qgAoRs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402160907.r0qgAoRs-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/sbm_test.c:300:13: sparse: sparse: symbol 'static_bss' was not declared. Should it be static?

vim +/static_bss +300 kernel/sbm_test.c

   293	
   294	/**************************************************************
   295	 * Memory write to kernel BSS.
   296	 *
   297	 * Sandbox mode cannot write to kernel BSS.
   298	 */
   299	
 > 300	struct data static_bss;
   301	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki