[PATCH v7 0/7] mseal system mappings

[PATCH v7 0/7] mseal system mappings

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

This is V7 version, addressing comments from V6, without code logic
change.

--------------------------------------------------

History:
V7:
 - Remove cover letter from the first patch (Liam R. Howlett)
 - Change macro name to VM_SEALED_SYSMAP (Liam R. Howlett)
 - logging and fclose() in selftest (Liam R. Howlett)

V6:
  https://lore.kernel.org/all/20250224174513.3600914-1-jeffxu@google.com/

V5
  https://lore.kernel.org/all/20250212032155.1276806-1-jeffxu@google.com/

V4:
  https://lore.kernel.org/all/20241125202021.3684919-1-jeffxu@google.com/

V3:
  https://lore.kernel.org/all/20241113191602.3541870-1-jeffxu@google.com/

V2:
  https://lore.kernel.org/all/20241014215022.68530-1-jeffxu@google.com/

V1:
  https://lore.kernel.org/all/20241004163155.3493183-1-jeffxu@google.com/

--------------------------------------------------
As discussed during mseal() upstream process [1], mseal() protects
the VMAs of a given virtual memory range against modifications, such
as the read/write (RW) and no-execute (NX) bits. For complete
descriptions of memory sealing, please see mseal.rst [2].

The mseal() is useful to mitigate memory corruption issues where a
corrupted pointer is passed to a memory management system. For
example, such an attacker primitive can break control-flow integrity
guarantees since read-only memory that is supposed to be trusted can
become writable or .text pages can get remapped.

The system mappings are readonly only, memory sealing can protect
them from ever changing to writable or unmmap/remapped as different
attributes.

System mappings such as vdso, vvar, and sigpage (arm), vectors (arm)
are created by the kernel during program initialization, and could
be sealed after creation.

Unlike the aforementioned mappings, the uprobe mapping is not
established during program startup. However, its lifetime is the same
as the process's lifetime [3]. It could be sealed from creation.

The vsyscall on x86-64 uses a special address (0xffffffffff600000),
which is outside the mm managed range. This means mprotect, munmap, and
mremap won't work on the vsyscall. Since sealing doesn't enhance
the vsyscall's security, it is skipped in this patch. If we ever seal
the vsyscall, it is probably only for decorative purpose, i.e. showing
the 'sl' flag in the /proc/pid/smaps. For this patch, it is ignored.

It is important to note that the CHECKPOINT_RESTORE feature (CRIU) may
alter the system mappings during restore operations. UML(User Mode Linux)
and gVisor, rr are also known to change the vdso/vvar mappings.
Consequently, this feature cannot be universally enabled across all
systems. As such, CONFIG_MSEAL_SYSTEM_MAPPINGS is disabled by default.

To support mseal of system mappings, architectures must define
CONFIG_ARCH_HAS_MSEAL_SYSTEM_MAPPINGS and update their special mappings
calls to pass mseal flag. Additionally, architectures must confirm they
do not unmap/remap system mappings during the process lifetime.

In this version, we've improved the handling of system mapping sealing from
previous versions, instead of modifying the _install_special_mapping
function itself, which would affect all architectures, we now call
_install_special_mapping with a sealing flag only within the specific
architecture that requires it. This targeted approach offers two key
advantages: 1) It limits the code change's impact to the necessary
architectures, and 2) It aligns with the software architecture by keeping
the core memory management within the mm layer, while delegating the
decision of sealing system mappings to the individual architecture, which
is particularly relevant since 32-bit architectures never require sealing.

Prior to this patch series, we explored sealing special mappings from
userspace using glibc's dynamic linker. This approach revealed several
issues:
- The PT_LOAD header may report an incorrect length for vdso, (smaller
  than its actual size). The dynamic linker, which relies on PT_LOAD
  information to determine mapping size, would then split and partially
  seal the vdso mapping. Since each architecture has its own vdso/vvar
  code, fixing this in the kernel would require going through each
  archiecture. Our initial goal was to enable sealing readonly mappings,
  e.g. .text, across all architectures, sealing vdso from kernel since
  creation appears to be simpler than sealing vdso at glibc.
- The [vvar] mapping header only contains address information, not length
  information. Similar issues might exist for other special mappings.
- Mappings like uprobe are not covered by the dynamic linker,
  and there is no effective solution for them.

This feature's security enhancements will benefit ChromeOS, Android,
and other high security systems.

Testing:
This feature was tested on ChromeOS and Android for both x86-64 and ARM64.
- Enable sealing and verify vdso/vvar, sigpage, vector are sealed properly,
  i.e. "sl" shown in the smaps for those mappings, and mremap is blocked.
- Passing various automation tests (e.g. pre-checkin) on ChromeOS and
  Android to ensure the sealing doesn't affect the functionality of
  Chromebook and Android phone.

I also tested the feature on Ubuntu on x86-64:
- With config disabled, vdso/vvar is not sealed,
- with config enabled, vdso/vvar is sealed, and booting up Ubuntu is OK,
  normal operations such as browsing the web, open/edit doc are OK.

In addition, Benjamin Berg tested this on UML.

Link: https://lore.kernel.org/all/20240415163527.626541-1-jeffxu@chromium.org/ [1]
Link: Documentation/userspace-api/mseal.rst [2]
Link: https://lore.kernel.org/all/CABi2SkU9BRUnqf70-nksuMCQ+yyiWjo3fM4XkRkL-NrCZxYAyg@mail.gmail.com/ [3]




Jeff Xu (7):
  mseal, system mappings: kernel config and header change
  selftests: x86: test_mremap_vdso: skip if vdso is msealed
  mseal, system mappings: enable x86-64
  mseal, system mappings: enable arm64
  mseal, system mappings: enable uml architecture
  mseal, system mappings: uprobe mapping
  mseal, system mappings: update mseal.rst

 Documentation/userspace-api/mseal.rst         |  7 +++
 arch/arm64/Kconfig                            |  1 +
 arch/arm64/kernel/vdso.c                      | 22 +++++++---
 arch/um/Kconfig                               |  1 +
 arch/x86/Kconfig                              |  1 +
 arch/x86/entry/vdso/vma.c                     | 16 ++++---
 arch/x86/um/vdso/vma.c                        |  6 ++-
 include/linux/mm.h                            | 10 +++++
 init/Kconfig                                  | 18 ++++++++
 kernel/events/uprobes.c                       |  5 ++-
 security/Kconfig                              | 18 ++++++++
 .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
 12 files changed, 132 insertions(+), 16 deletions(-)

-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 1/7] mseal, system mappings: kernel config and header change

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Provide infrastructure to mseal system mappings. Establish
two kernel configs (CONFIG_MSEAL_SYSTEM_MAPPINGS,
ARCH_HAS_MSEAL_SYSTEM_MAPPINGS) and VM_SEALED_SYSMAP
macro for future patches.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 include/linux/mm.h | 10 ++++++++++
 init/Kconfig       | 18 ++++++++++++++++++
 security/Kconfig   | 18 ++++++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7b1068ddcbb7..8b800941678d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4155,4 +4155,14 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
 int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
 int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
 
+
+/*
+ * mseal of userspace process's system mappings.
+ */
+#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
+#define VM_SEALED_SYSMAP	VM_SEALED
+#else
+#define VM_SEALED_SYSMAP	VM_NONE
+#endif
+
 #endif /* _LINUX_MM_H */
diff --git a/init/Kconfig b/init/Kconfig
index d0d021b3fa3b..07435e33f965 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
 config ARCH_HAS_MEMBARRIER_SYNC_CORE
 	bool
 
+config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
+	bool
+	help
+	  Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
+
+	  A 64-bit kernel is required for the memory sealing feature.
+	  No specific hardware features from the CPU are needed.
+
+	  To enable this feature, the architecture needs to update their
+	  special mappings calls to include the sealing flag and confirm
+	  that it doesn't unmap/remap system mappings during the life
+	  time of the process. After the architecture enables this, a
+	  distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
+	  to the feature.
+
+	  For complete descriptions of memory sealing, please see
+	  Documentation/userspace-api/mseal.rst
+
 config HAVE_PERF_EVENTS
 	bool
 	help
diff --git a/security/Kconfig b/security/Kconfig
index f10dbf15c294..15a86a952910 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -51,6 +51,24 @@ config PROC_MEM_NO_FORCE
 
 endchoice
 
+config MSEAL_SYSTEM_MAPPINGS
+	bool "mseal system mappings"
+	depends on 64BIT
+	depends on ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
+	depends on !CHECKPOINT_RESTORE
+	help
+	  Seal system mappings such as vdso, vvar, sigpage, uprobes, etc.
+
+	  A 64-bit kernel is required for the memory sealing feature.
+	  No specific hardware features from the CPU are needed.
+
+	  Note: CHECKPOINT_RESTORE, UML, gVisor, rr are known to relocate or
+	  unmap system mapping, therefore this config can't be enabled
+	  universally.
+
+	  For complete descriptions of memory sealing, please see
+	  Documentation/userspace-api/mseal.rst
+
 config SECURITY
 	bool "Enable different security models"
 	depends on SYSFS
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 2/7] selftests: x86: test_mremap_vdso: skip if vdso is msealed

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Add code to detect if the vdso is memory sealed, skip the test
if it is.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Reviewed-by: Kees Cook <kees@kernel.org>
---
 .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c
index d53959e03593..94bee6e0c813 100644
--- a/tools/testing/selftests/x86/test_mremap_vdso.c
+++ b/tools/testing/selftests/x86/test_mremap_vdso.c
@@ -14,6 +14,7 @@
 #include <errno.h>
 #include <unistd.h>
 #include <string.h>
+#include <stdbool.h>
 
 #include <sys/mman.h>
 #include <sys/auxv.h>
@@ -55,13 +56,55 @@ static int try_to_remap(void *vdso_addr, unsigned long size)
 
 }
 
+#define VDSO_NAME "[vdso]"
+#define VMFLAGS "VmFlags:"
+#define MSEAL_FLAGS "sl"
+#define MAX_LINE_LEN 512
+
+bool vdso_sealed(FILE *maps)
+{
+	char line[MAX_LINE_LEN];
+	bool has_vdso = false;
+
+	while (fgets(line, sizeof(line), maps)) {
+		if (strstr(line, VDSO_NAME))
+			has_vdso = true;
+
+		if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) {
+			if (strstr(line, MSEAL_FLAGS))
+				return true;
+
+			return false;
+		}
+	}
+
+	return false;
+}
+
 int main(int argc, char **argv, char **envp)
 {
 	pid_t child;
+	FILE *maps;
 
 	ksft_print_header();
 	ksft_set_plan(1);
 
+	maps = fopen("/proc/self/smaps", "r");
+	if (!maps) {
+		ksft_test_result_skip(
+			"Could not open /proc/self/smaps, errno=%d\n",
+			 errno);
+
+		return 0;
+	}
+
+	if (vdso_sealed(maps)) {
+		ksft_test_result_skip("vdso is sealed\n");
+		return 0;
+	}
+
+	fclose(maps);
+
 	child = fork();
 	if (child == -1)
 		ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 3/7] mseal, system mappings: enable x86-64

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
covering the vdso, vvar, vvar_vclock.

Production release testing passes on Android and Chrome OS.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 arch/x86/Kconfig          |  1 +
 arch/x86/entry/vdso/vma.c | 16 ++++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 87198d957e2f..8fa17032ca46 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -26,6 +26,7 @@ config X86_64
 	depends on 64BIT
 	# Options that are inherently 64-bit kernel only:
 	select ARCH_HAS_GIGANTIC_PAGE
+	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
 	select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
 	select ARCH_SUPPORTS_PER_VMA_LOCK
 	select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index 39e6efc1a9ca..1b1c009f20a8 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	unsigned long text_start;
+	unsigned long vm_flags;
 	int ret = 0;
 
 	if (mmap_write_lock_killable(mm))
@@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
 	/*
 	 * MAYWRITE to allow gdb to COW and set breakpoints
 	 */
+	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
+	vm_flags |= VM_SEALED_SYSMAP;
 	vma = _install_special_mapping(mm,
 				       text_start,
 				       image->size,
-				       VM_READ|VM_EXEC|
-				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
+				       vm_flags,
 				       &vdso_mapping);
 
 	if (IS_ERR(vma)) {
@@ -276,11 +278,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
 		goto up_fail;
 	}
 
+	vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
+	vm_flags |= VM_SEALED_SYSMAP;
 	vma = _install_special_mapping(mm,
 				       addr,
 				       (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
-				       VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
-				       VM_PFNMAP,
+				       vm_flags,
 				       &vvar_mapping);
 
 	if (IS_ERR(vma)) {
@@ -289,11 +292,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
 		goto up_fail;
 	}
 
+	vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
+	vm_flags |= VM_SEALED_SYSMAP;
 	vma = _install_special_mapping(mm,
 				       addr + (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
 				       VDSO_NR_VCLOCK_PAGES * PAGE_SIZE,
-				       VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
-				       VM_PFNMAP,
+				       vm_flags,
 				       &vvar_vclock_mapping);
 
 	if (IS_ERR(vma)) {
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 4/7] mseal, system mappings: enable arm64

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
the vdso, vvar, and compat-mode vectors and sigpage mappings.

Production release testing passes on Android and Chrome OS.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 arch/arm64/Kconfig       |  1 +
 arch/arm64/kernel/vdso.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index fcdd0ed3eca8..39202aa9a5af 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -38,6 +38,7 @@ config ARM64
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
 	select ARCH_HAS_MEM_ENCRYPT
+	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
 	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_NONLEAF_PMD_YOUNG if ARM64_HAFT
diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index e8ed8e5b713b..12e6ab396018 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -183,6 +183,7 @@ static int __setup_additional_pages(enum vdso_abi abi,
 {
 	unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
 	unsigned long gp_flags = 0;
+	unsigned long vm_flags;
 	void *ret;
 
 	BUILD_BUG_ON(VVAR_NR_PAGES != __VVAR_PAGES);
@@ -197,8 +198,10 @@ static int __setup_additional_pages(enum vdso_abi abi,
 		goto up_fail;
 	}
 
+	vm_flags = VM_READ|VM_MAYREAD|VM_PFNMAP;
+	vm_flags |= VM_SEALED_SYSMAP;
 	ret = _install_special_mapping(mm, vdso_base, VVAR_NR_PAGES * PAGE_SIZE,
-				       VM_READ|VM_MAYREAD|VM_PFNMAP,
+				       vm_flags,
 				       &vvar_map);
 	if (IS_ERR(ret))
 		goto up_fail;
@@ -208,9 +211,10 @@ static int __setup_additional_pages(enum vdso_abi abi,
 
 	vdso_base += VVAR_NR_PAGES * PAGE_SIZE;
 	mm->context.vdso = (void *)vdso_base;
+	vm_flags = VM_READ|VM_EXEC|gp_flags|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
+	vm_flags |= VM_SEALED_SYSMAP;
 	ret = _install_special_mapping(mm, vdso_base, vdso_text_len,
-				       VM_READ|VM_EXEC|gp_flags|
-				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
+				       vm_flags,
 				       vdso_info[abi].cm);
 	if (IS_ERR(ret))
 		goto up_fail;
@@ -326,6 +330,7 @@ arch_initcall(aarch32_alloc_vdso_pages);
 static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 {
 	void *ret;
+	unsigned long vm_flags;
 
 	if (!IS_ENABLED(CONFIG_KUSER_HELPERS))
 		return 0;
@@ -334,9 +339,10 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 	 * Avoid VM_MAYWRITE for compatibility with arch/arm/, where it's
 	 * not safe to CoW the page containing the CPU exception vectors.
 	 */
+	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYEXEC;
+	vm_flags |= VM_SEALED_SYSMAP;
 	ret = _install_special_mapping(mm, AARCH32_VECTORS_BASE, PAGE_SIZE,
-				       VM_READ | VM_EXEC |
-				       VM_MAYREAD | VM_MAYEXEC,
+				       vm_flags,
 				       &aarch32_vdso_maps[AA32_MAP_VECTORS]);
 
 	return PTR_ERR_OR_ZERO(ret);
@@ -345,6 +351,7 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 static int aarch32_sigreturn_setup(struct mm_struct *mm)
 {
 	unsigned long addr;
+	unsigned long vm_flags;
 	void *ret;
 
 	addr = get_unmapped_area(NULL, 0, PAGE_SIZE, 0, 0);
@@ -357,9 +364,10 @@ static int aarch32_sigreturn_setup(struct mm_struct *mm)
 	 * VM_MAYWRITE is required to allow gdb to Copy-on-Write and
 	 * set breakpoints.
 	 */
+	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
+	vm_flags |= VM_SEALED_SYSMAP;
 	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
-				       VM_READ | VM_EXEC | VM_MAYREAD |
-				       VM_MAYWRITE | VM_MAYEXEC,
+				       vm_flags,
 				       &aarch32_vdso_maps[AA32_MAP_SIGPAGE]);
 	if (IS_ERR(ret))
 		goto out;
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 5/7] mseal, system mappings: enable uml architecture

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
the vdso.

Testing passes on UML.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Tested-by: Benjamin Berg <benjamin.berg@intel.com>
---
 arch/um/Kconfig        | 1 +
 arch/x86/um/vdso/vma.c | 6 ++++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 18051b1cfce0..eb2d439a5334 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -10,6 +10,7 @@ config UML
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_HAS_KCOV
+	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
 	select ARCH_HAS_STRNCPY_FROM_USER
 	select ARCH_HAS_STRNLEN_USER
 	select HAVE_ARCH_AUDITSYSCALL
diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
index f238f7b33cdd..fdfba858ffc9 100644
--- a/arch/x86/um/vdso/vma.c
+++ b/arch/x86/um/vdso/vma.c
@@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 {
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
+	unsigned long vm_flags;
 	static struct vm_special_mapping vdso_mapping = {
 		.name = "[vdso]",
 	};
@@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 		return -EINTR;
 
 	vdso_mapping.pages = vdsop;
+	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
+	vm_flags |= VM_SEALED_SYSMAP;
 	vma = _install_special_mapping(mm, um_vdso_addr, PAGE_SIZE,
-		VM_READ|VM_EXEC|
-		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
+		vm_flags,
 		&vdso_mapping);
 
 	mmap_write_unlock(mm);
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 6/7] mseal, system mappings: uprobe mapping

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Provide support to mseal the uprobe mapping.

Unlike other system mappings, the uprobe mapping is not
established during program startup. However, its lifetime is the same
as the process's lifetime. It could be sealed from creation.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 kernel/events/uprobes.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2ca797cbe465..8dcdfa0d306b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1662,6 +1662,7 @@ static const struct vm_special_mapping xol_mapping = {
 static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 {
 	struct vm_area_struct *vma;
+	unsigned long vm_flags;
 	int ret;
 
 	if (mmap_write_lock_killable(mm))
@@ -1682,8 +1683,10 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 		}
 	}
 
+	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
+	vm_flags |= VM_SEALED_SYSMAP;
 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
-				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
+				vm_flags,
 				&xol_mapping);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
-- 
2.48.1.658.g4767266eb4-goog

[PATCH v7 7/7] mseal, system mappings: update mseal.rst

[jeffxu]

From: Jeff Xu <jeffxu@chromium.org>

Update memory sealing documentation to include details about system
mappings.

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
Reviewed-by: Kees Cook <kees@kernel.org>
---
 Documentation/userspace-api/mseal.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/userspace-api/mseal.rst b/Documentation/userspace-api/mseal.rst
index 41102f74c5e2..10147281bf2d 100644
--- a/Documentation/userspace-api/mseal.rst
+++ b/Documentation/userspace-api/mseal.rst
@@ -130,6 +130,13 @@ Use cases
 
 - Chrome browser: protect some security sensitive data structures.
 
+- System mappings:
+  If supported by an architecture (via CONFIG_ARCH_HAS_MSEAL_SYSTEM_MAPPINGS),
+  the CONFIG_MSEAL_SYSTEM_MAPPINGS seals system mappings, e.g. vdso, vvar,
+  uprobes, sigpage, vectors, etc. CHECKPOINT_RESTORE, UML, gVisor, rr are
+  known to relocate or unmap system mapping, therefore this config can't be
+  enabled universally.
+
 When not to use mseal
 =====================
 Applications can apply sealing to any virtual memory region from userspace,
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v7 0/7] mseal system mappings

[Pedro Falcato]

On Mon, Feb 24, 2025 at 10:52 PM <jeffxu@chromium.org> wrote:
>
> From: Jeff Xu <jeffxu@chromium.org>
>
> This is V7 version, addressing comments from V6, without code logic
> change.
>
> --------------------------------------------------
>
> History:
> V7:
>  - Remove cover letter from the first patch (Liam R. Howlett)
>  - Change macro name to VM_SEALED_SYSMAP (Liam R. Howlett)
>  - logging and fclose() in selftest (Liam R. Howlett)

Jeff, please don't send out new versions of the patchset that quickly.
We were having a discussion on v5, you sent v6 today (acceptable) and
now v7 (while changing barely anything of note). It's hard to track
things this way, and you're just flooding a bunch of mailboxes.

Thanks,
Pedro

Re: [PATCH v7 0/7] mseal system mappings

[Jeff Xu]

On Mon, Feb 24, 2025 at 3:03 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> On Mon, Feb 24, 2025 at 10:52 PM <jeffxu@chromium.org> wrote:
> >
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > This is V7 version, addressing comments from V6, without code logic
> > change.
> >
> > --------------------------------------------------
> >
> > History:
> > V7:
> >  - Remove cover letter from the first patch (Liam R. Howlett)
> >  - Change macro name to VM_SEALED_SYSMAP (Liam R. Howlett)
> >  - logging and fclose() in selftest (Liam R. Howlett)
>
> Jeff, please don't send out new versions of the patchset that quickly.
> We were having a discussion on v5, you sent v6 today (acceptable) and
> now v7 (while changing barely anything of note). It's hard to track
> things this way, and you're just flooding a bunch of mailboxes.
>
Ah, I apologize. Sure.

-Jeff


> Thanks,
> Pedro

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Kees Cook]

On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
> 
> Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> covering the vdso, vvar, vvar_vclock.
> 
> Production release testing passes on Android and Chrome OS.
> 
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  arch/x86/Kconfig          |  1 +
>  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 87198d957e2f..8fa17032ca46 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -26,6 +26,7 @@ config X86_64
>  	depends on 64BIT
>  	# Options that are inherently 64-bit kernel only:
>  	select ARCH_HAS_GIGANTIC_PAGE
> +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>  	select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
>  	select ARCH_SUPPORTS_PER_VMA_LOCK
>  	select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> index 39e6efc1a9ca..1b1c009f20a8 100644
> --- a/arch/x86/entry/vdso/vma.c
> +++ b/arch/x86/entry/vdso/vma.c
> @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	unsigned long text_start;
> +	unsigned long vm_flags;
>  	int ret = 0;
>  
>  	if (mmap_write_lock_killable(mm))
> @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  	/*
>  	 * MAYWRITE to allow gdb to COW and set breakpoints
>  	 */
> +	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm,
>  				       text_start,
>  				       image->size,
> -				       VM_READ|VM_EXEC|
> -				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> +				       vm_flags,
>  				       &vdso_mapping);

I think these (in all patches) were supposed to be reworked without the
"vm_flags" variable addition?

-- 
Kees Cook

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 10:52:40PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> Provide infrastructure to mseal system mappings. Establish
> two kernel configs (CONFIG_MSEAL_SYSTEM_MAPPINGS,
> ARCH_HAS_MSEAL_SYSTEM_MAPPINGS) and VM_SEALED_SYSMAP
> macro for future patches.
>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  include/linux/mm.h | 10 ++++++++++
>  init/Kconfig       | 18 ++++++++++++++++++
>  security/Kconfig   | 18 ++++++++++++++++++
>  3 files changed, 46 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b1068ddcbb7..8b800941678d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4155,4 +4155,14 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
>  int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
>  int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>
> +
> +/*
> + * mseal of userspace process's system mappings.
> + */
> +#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> +#define VM_SEALED_SYSMAP	VM_SEALED
> +#else
> +#define VM_SEALED_SYSMAP	VM_NONE
> +#endif

This is much better than the original thanks.

> +
>  #endif /* _LINUX_MM_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index d0d021b3fa3b..07435e33f965 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
>  config ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	bool
>
> +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> +	bool
> +	help
> +	  Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> +
> +	  A 64-bit kernel is required for the memory sealing feature.
> +	  No specific hardware features from the CPU are needed.
> +
> +	  To enable this feature, the architecture needs to update their
> +	  special mappings calls to include the sealing flag and confirm
> +	  that it doesn't unmap/remap system mappings during the life
> +	  time of the process. After the architecture enables this, a
> +	  distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> +	  to the feature.

Architectures also need to be confirmed not to require any form of VDSO
relocation, which as discussed in previous series some arches appear to
need to do. I'd mention that here.

> +
> +	  For complete descriptions of memory sealing, please see
> +	  Documentation/userspace-api/mseal.rst
> +
>  config HAVE_PERF_EVENTS
>  	bool
>  	help
> diff --git a/security/Kconfig b/security/Kconfig
> index f10dbf15c294..15a86a952910 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -51,6 +51,24 @@ config PROC_MEM_NO_FORCE
>
>  endchoice
>
> +config MSEAL_SYSTEM_MAPPINGS
> +	bool "mseal system mappings"
> +	depends on 64BIT
> +	depends on ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> +	depends on !CHECKPOINT_RESTORE
> +	help
> +	  Seal system mappings such as vdso, vvar, sigpage, uprobes, etc.

Let's be specific here, 'etc.' could mean _anything_. Also you aren't
sealing most of this, let's just list what you are _actually_ sealing
here. Which is AFAIK VDSO only?

You can update this later as time goes on if/when you expand this.

> +
> +	  A 64-bit kernel is required for the memory sealing feature.
> +	  No specific hardware features from the CPU are needed.
> +
> +	  Note: CHECKPOINT_RESTORE, UML, gVisor, rr are known to relocate or
> +	  unmap system mapping, therefore this config can't be enabled
> +	  universally.

Thanks for putting this here, appreciate it!

Could we tweak this though? I'd like to make it crystal clear, so I don't
think 'note' sufficies and this sounds a little too vague.

I think 'warning' is more appropriate here since you're breaking things for
people who might be unaware. And we need to say this -breaks- programs:

WARNING: This feature breaks programs which rely on relocating or
         unmapping system mappings.

         Known broken software at the time of writing includes
         CHECKPOINT_RESTORE, UML, gVisor and rr.

I think this is critical.

> +
> +	  For complete descriptions of memory sealing, please see
> +	  Documentation/userspace-api/mseal.rst
> +
>  config SECURITY
>  	bool "Enable different security models"
>  	depends on SYSFS
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH v7 0/7] mseal system mappings

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 03:07:03PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 3:03 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 10:52 PM <jeffxu@chromium.org> wrote:
> > >
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > This is V7 version, addressing comments from V6, without code logic
> > > change.
> > >
> > > --------------------------------------------------
> > >
> > > History:
> > > V7:
> > >  - Remove cover letter from the first patch (Liam R. Howlett)
> > >  - Change macro name to VM_SEALED_SYSMAP (Liam R. Howlett)
> > >  - logging and fclose() in selftest (Liam R. Howlett)
> >
> > Jeff, please don't send out new versions of the patchset that quickly.
> > We were having a discussion on v5, you sent v6 today (acceptable) and
> > now v7 (while changing barely anything of note). It's hard to track
> > things this way, and you're just flooding a bunch of mailboxes.
> >
> Ah, I apologize. Sure.
>
> -Jeff

Thanks, I am behind the eight ball on this in a post-viral fatigue haze, so I'd
especially appreciate relaxing a bit on series pace here haha.

I mean being reasonable I don't want you to feel you're needing to be told 'ok
send vX' now, but I'd simply suggest - wait until things 'settle down' a bit on
comments + everything's addressed and the 'usual suspects' have commented, then
this is a good time to send next version.

I realise I'm maybe not well placed to say this as I've previouisly been famous
for resending WAY too quick haha, but it's something I've worked on myself so I
guess, I relate...

>
>
> > Thanks,
> > Pedro

Thanks!

Re: [PATCH v7 2/7] selftests: x86: test_mremap_vdso: skip if vdso is msealed

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 10:52:41PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> Add code to detect if the vdso is memory sealed, skip the test
> if it is.

I feel this is a little succinct of a commit message, but I guess it gets to the
heart of what you're doing here.

Fundamentally I mean it makes sense, but I'm concerned that x86 has a test
-expliictly checking- whether mremap() of VDSO is possible - are there cases
where x86 might want to do this internal to the kernel?

I guess not since this is essentially a userland self test and probably
asserting you can do this in the way rr, etc. do.

>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> Reviewed-by: Kees Cook <kees@kernel.org>

Anyway, this aside, this looks fine, aside from nit below, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c
> index d53959e03593..94bee6e0c813 100644
> --- a/tools/testing/selftests/x86/test_mremap_vdso.c
> +++ b/tools/testing/selftests/x86/test_mremap_vdso.c
> @@ -14,6 +14,7 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <string.h>
> +#include <stdbool.h>
>
>  #include <sys/mman.h>
>  #include <sys/auxv.h>
> @@ -55,13 +56,55 @@ static int try_to_remap(void *vdso_addr, unsigned long size)
>
>  }
>
> +#define VDSO_NAME "[vdso]"
> +#define VMFLAGS "VmFlags:"
> +#define MSEAL_FLAGS "sl"
> +#define MAX_LINE_LEN 512
> +
> +bool vdso_sealed(FILE *maps)

Should be static?

> +{
> +	char line[MAX_LINE_LEN];
> +	bool has_vdso = false;
> +
> +	while (fgets(line, sizeof(line), maps)) {
> +		if (strstr(line, VDSO_NAME))
> +			has_vdso = true;
> +
> +		if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) {
> +			if (strstr(line, MSEAL_FLAGS))
> +				return true;
> +
> +			return false;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  int main(int argc, char **argv, char **envp)
>  {
>  	pid_t child;
> +	FILE *maps;
>
>  	ksft_print_header();
>  	ksft_set_plan(1);
>
> +	maps = fopen("/proc/self/smaps", "r");
> +	if (!maps) {
> +		ksft_test_result_skip(
> +			"Could not open /proc/self/smaps, errno=%d\n",
> +			 errno);
> +
> +		return 0;
> +	}
> +
> +	if (vdso_sealed(maps)) {
> +		ksft_test_result_skip("vdso is sealed\n");
> +		return 0;
> +	}
> +
> +	fclose(maps);
> +
>  	child = fork();
>  	if (child == -1)
>  		ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> the vdso, vvar, and compat-mode vectors and sigpage mappings.
>
> Production release testing passes on Android and Chrome OS.

This is pretty limited (yes yes I know android is massive etc. but we must
account for all the weird and wonderful arm64 devices out there in context of
upstream :)

Have you looking through all arm64-code relating to vdso, vvar, compat-mode
vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
nervous of that.

At any rate some comment about having checked/confirmed this would be good, arm
concerns me a lot more than x86 on this front.

Thanks however for doing extensive testing android/chrome side! This is of
course, very important for sheer volume (and probably worldwide % of deployed
arm64 devices...)

Just need to dot our i's and cross our t's...

>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  arch/arm64/Kconfig       |  1 +
>  arch/arm64/kernel/vdso.c | 22 +++++++++++++++-------
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index fcdd0ed3eca8..39202aa9a5af 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -38,6 +38,7 @@ config ARM64
>  	select ARCH_HAS_KEEPINITRD
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	select ARCH_HAS_MEM_ENCRYPT
> +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>  	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_NONLEAF_PMD_YOUNG if ARM64_HAFT
> diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
> index e8ed8e5b713b..12e6ab396018 100644
> --- a/arch/arm64/kernel/vdso.c
> +++ b/arch/arm64/kernel/vdso.c
> @@ -183,6 +183,7 @@ static int __setup_additional_pages(enum vdso_abi abi,
>  {
>  	unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
>  	unsigned long gp_flags = 0;
> +	unsigned long vm_flags;
>  	void *ret;
>
>  	BUILD_BUG_ON(VVAR_NR_PAGES != __VVAR_PAGES);
> @@ -197,8 +198,10 @@ static int __setup_additional_pages(enum vdso_abi abi,
>  		goto up_fail;
>  	}
>
> +	vm_flags = VM_READ|VM_MAYREAD|VM_PFNMAP;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	ret = _install_special_mapping(mm, vdso_base, VVAR_NR_PAGES * PAGE_SIZE,
> -				       VM_READ|VM_MAYREAD|VM_PFNMAP,
> +				       vm_flags,
>  				       &vvar_map);
>  	if (IS_ERR(ret))
>  		goto up_fail;
> @@ -208,9 +211,10 @@ static int __setup_additional_pages(enum vdso_abi abi,
>
>  	vdso_base += VVAR_NR_PAGES * PAGE_SIZE;
>  	mm->context.vdso = (void *)vdso_base;
> +	vm_flags = VM_READ|VM_EXEC|gp_flags|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	ret = _install_special_mapping(mm, vdso_base, vdso_text_len,
> -				       VM_READ|VM_EXEC|gp_flags|
> -				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> +				       vm_flags,
>  				       vdso_info[abi].cm);
>  	if (IS_ERR(ret))
>  		goto up_fail;
> @@ -326,6 +330,7 @@ arch_initcall(aarch32_alloc_vdso_pages);
>  static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>  {
>  	void *ret;
> +	unsigned long vm_flags;
>
>  	if (!IS_ENABLED(CONFIG_KUSER_HELPERS))
>  		return 0;
> @@ -334,9 +339,10 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>  	 * Avoid VM_MAYWRITE for compatibility with arch/arm/, where it's
>  	 * not safe to CoW the page containing the CPU exception vectors.
>  	 */
> +	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	ret = _install_special_mapping(mm, AARCH32_VECTORS_BASE, PAGE_SIZE,
> -				       VM_READ | VM_EXEC |
> -				       VM_MAYREAD | VM_MAYEXEC,
> +				       vm_flags,
>  				       &aarch32_vdso_maps[AA32_MAP_VECTORS]);
>
>  	return PTR_ERR_OR_ZERO(ret);
> @@ -345,6 +351,7 @@ static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
>  static int aarch32_sigreturn_setup(struct mm_struct *mm)
>  {
>  	unsigned long addr;
> +	unsigned long vm_flags;
>  	void *ret;
>
>  	addr = get_unmapped_area(NULL, 0, PAGE_SIZE, 0, 0);
> @@ -357,9 +364,10 @@ static int aarch32_sigreturn_setup(struct mm_struct *mm)
>  	 * VM_MAYWRITE is required to allow gdb to Copy-on-Write and
>  	 * set breakpoints.
>  	 */
> +	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	ret = _install_special_mapping(mm, addr, PAGE_SIZE,
> -				       VM_READ | VM_EXEC | VM_MAYREAD |
> -				       VM_MAYWRITE | VM_MAYEXEC,
> +				       vm_flags,
>  				       &aarch32_vdso_maps[AA32_MAP_SIGPAGE]);
>  	if (IS_ERR(ret))
>  		goto out;
> --
> 2.48.1.658.g4767266eb4-goog
>

Patch looks fine for purposes of what you're trying to achieve though, just
need to have some calming reassurances about arch :)

Thanks!

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> the vdso.
>
> Testing passes on UML.

Maybe expand on this by stating that it has been confirmed by Benjamin (I
_believe_) that UML has no need for problematic relocation so this is known to
be good.

>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> Tested-by: Benjamin Berg <benjamin.berg@intel.com>

Anyway I know UML has at any rate been confirmed to be good to go + patch looks
fine, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  arch/um/Kconfig        | 1 +
>  arch/x86/um/vdso/vma.c | 6 ++++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 18051b1cfce0..eb2d439a5334 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -10,6 +10,7 @@ config UML
>  	select ARCH_HAS_FORTIFY_SOURCE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
>  	select ARCH_HAS_KCOV
> +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>  	select ARCH_HAS_STRNCPY_FROM_USER
>  	select ARCH_HAS_STRNLEN_USER
>  	select HAVE_ARCH_AUDITSYSCALL
> diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> index f238f7b33cdd..fdfba858ffc9 100644
> --- a/arch/x86/um/vdso/vma.c
> +++ b/arch/x86/um/vdso/vma.c
> @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>  {
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
> +	unsigned long vm_flags;
>  	static struct vm_special_mapping vdso_mapping = {
>  		.name = "[vdso]",
>  	};
> @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>  		return -EINTR;
>
>  	vdso_mapping.pages = vdsop;
> +	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm, um_vdso_addr, PAGE_SIZE,
> -		VM_READ|VM_EXEC|
> -		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> +		vm_flags,
>  		&vdso_mapping);
>
>  	mmap_write_unlock(mm);
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Mon, Feb 24, 2025 at 10:52:45PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> Provide support to mseal the uprobe mapping.
>
> Unlike other system mappings, the uprobe mapping is not
> established during program startup. However, its lifetime is the same
> as the process's lifetime. It could be sealed from creation.
>

I thought we agreed not to enable this for uprobes for now? What testing
have you done to ensure this is functional?

I mean is this literally _all_ uprobe mappings now being sealed?

I'd really like some more assurances on this one. And what are you
mitigating by sealing these? I get VDSO (kinda) but uprobes?

You really need to provide more justification here.

> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  kernel/events/uprobes.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2ca797cbe465..8dcdfa0d306b 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1662,6 +1662,7 @@ static const struct vm_special_mapping xol_mapping = {
>  static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
>  {
>  	struct vm_area_struct *vma;
> +	unsigned long vm_flags;
>  	int ret;
>
>  	if (mmap_write_lock_killable(mm))
> @@ -1682,8 +1683,10 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
>  		}
>  	}
>
> +	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> +				vm_flags,
>  				&xol_mapping);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Thomas Weißschuh]

Hi Jeff,

On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
> 
> Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> covering the vdso, vvar, vvar_vclock.
> 
> Production release testing passes on Android and Chrome OS.
> 
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  arch/x86/Kconfig          |  1 +
>  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 87198d957e2f..8fa17032ca46 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -26,6 +26,7 @@ config X86_64
>  	depends on 64BIT
>  	# Options that are inherently 64-bit kernel only:
>  	select ARCH_HAS_GIGANTIC_PAGE
> +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>  	select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
>  	select ARCH_SUPPORTS_PER_VMA_LOCK
>  	select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> index 39e6efc1a9ca..1b1c009f20a8 100644
> --- a/arch/x86/entry/vdso/vma.c
> +++ b/arch/x86/entry/vdso/vma.c
> @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
>  	unsigned long text_start;
> +	unsigned long vm_flags;
>  	int ret = 0;
>  
>  	if (mmap_write_lock_killable(mm))
> @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  	/*
>  	 * MAYWRITE to allow gdb to COW and set breakpoints
>  	 */
> +	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm,
>  				       text_start,
>  				       image->size,
> -				       VM_READ|VM_EXEC|
> -				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> +				       vm_flags,
>  				       &vdso_mapping);
>  
>  	if (IS_ERR(vma)) {
> @@ -276,11 +278,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  		goto up_fail;
>  	}
>  
> +	vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm,
>  				       addr,
>  				       (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
> -				       VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
> -				       VM_PFNMAP,
> +				       vm_flags,
>  				       &vvar_mapping);

This hunk (and the vvar mapping in the arm64 patch) will conflict with my
"Generic vDSO datapage" series.
That series is already part of the tip tree (branch timers/vdso) and scheduled
for the next merge window.

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/vdso

The conflict resolution is fairly easy:
Move the new flag logic into lib/vdso/datastore.c

Depending on the expected mainline timing for this series either mention this
somewhere for the conflict resolution by Linus or rebase your series on top of
tip/timers/vdso.

>  	if (IS_ERR(vma)) {
> @@ -289,11 +292,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
>  		goto up_fail;
>  	}
>  
> +	vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
> +	vm_flags |= VM_SEALED_SYSMAP;
>  	vma = _install_special_mapping(mm,
>  				       addr + (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
>  				       VDSO_NR_VCLOCK_PAGES * PAGE_SIZE,
> -				       VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
> -				       VM_PFNMAP,
> +				       vm_flags,
>  				       &vvar_vclock_mapping);
>  
>  	if (IS_ERR(vma)) {
> -- 
> 2.48.1.658.g4767266eb4-goog
> 

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Berg, Benjamin]

Hi,

On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> > 
> > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > the vdso.
> > 
> > Testing passes on UML.
> 
> Maybe expand on this by stating that it has been confirmed by Benjamin (I
> _believe_) that UML has no need for problematic relocation so this is known to
> be good.

I may well be misreading this message, but this sounds to me that this
is a misinterpretation. So, just to clarify in case that is needed.

CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
However, the UML kernel is a normal userspace application itself and
for this application to run, the host kernel must have the feature
disabled.

So, UML supports the feature. But it still *cannot* run on a host
machine that has the feature enabled.

Benjamin

> 
> > 
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > Tested-by: Benjamin Berg <benjamin.berg@intel.com>
> 
> Anyway I know UML has at any rate been confirmed to be good to go +
> patch looks
> fine, so:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 
> > ---
> >  arch/um/Kconfig        | 1 +
> >  arch/x86/um/vdso/vma.c | 6 ++++--
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> > index 18051b1cfce0..eb2d439a5334 100644
> > --- a/arch/um/Kconfig
> > +++ b/arch/um/Kconfig
> > @@ -10,6 +10,7 @@ config UML
> >  	select ARCH_HAS_FORTIFY_SOURCE
> >  	select ARCH_HAS_GCOV_PROFILE_ALL
> >  	select ARCH_HAS_KCOV
> > +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> >  	select ARCH_HAS_STRNCPY_FROM_USER
> >  	select ARCH_HAS_STRNLEN_USER
> >  	select HAVE_ARCH_AUDITSYSCALL
> > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> > index f238f7b33cdd..fdfba858ffc9 100644
> > --- a/arch/x86/um/vdso/vma.c
> > +++ b/arch/x86/um/vdso/vma.c
> > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct
> > linux_binprm *bprm, int uses_interp)
> >  {
> >  	struct vm_area_struct *vma;
> >  	struct mm_struct *mm = current->mm;
> > +	unsigned long vm_flags;
> >  	static struct vm_special_mapping vdso_mapping = {
> >  		.name = "[vdso]",
> >  	};
> > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct
> > linux_binprm *bprm, int uses_interp)
> >  		return -EINTR;
> > 
> >  	vdso_mapping.pages = vdsop;
> > +	vm_flags =
> > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > +	vm_flags |= VM_SEALED_SYSMAP;
> >  	vma = _install_special_mapping(mm, um_vdso_addr,
> > PAGE_SIZE,
> > -		VM_READ|VM_EXEC|
> > -		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > +		vm_flags,
> >  		&vdso_mapping);
> > 
> >  	mmap_write_unlock(mm);
> > --
> > 2.48.1.658.g4767266eb4-goog
> > 

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v7 0/7] mseal system mappings

[Lorenzo Stoakes]

BTW can we please drop the 'mseal, system mappings' prefixes on this
series, it's really weird and makes it really hard for me to actually read
the individual summary lines for each commit. 'mseal:' will do.

I mean really you could argue it's 'mm: mseal: ...' but I'm not quite
_that_ pedantic :)

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> Hi,
>
> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > > the vdso.
> > >
> > > Testing passes on UML.
> >
> > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> > _believe_) that UML has no need for problematic relocation so this is known to
> > be good.
>
> I may well be misreading this message, but this sounds to me that this
> is a misinterpretation. So, just to clarify in case that is needed.
>
> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> However, the UML kernel is a normal userspace application itself and
> for this application to run, the host kernel must have the feature
> disabled.
>
> So, UML supports the feature. But it still *cannot* run on a host
> machine that has the feature enabled.

Sigh ok. Apologies if I misunderstood.

Is there any point having this for the 'guest' system? I mean security wise are
we concerned about sealing of system mappings?

I feel like having this here might just add confusion and churn if it's not
useful.

If this is useless for UML guest, let's just drop this patch.

>
> Benjamin
>
> >
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > Tested-by: Benjamin Berg <benjamin.berg@intel.com>
> >
> > Anyway I know UML has at any rate been confirmed to be good to go +
> > patch looks
> > fine, so:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

OK guess drop this tag please until we can figure this out, sorry Jeff.

> >
> > > ---
> > >  arch/um/Kconfig        | 1 +
> > >  arch/x86/um/vdso/vma.c | 6 ++++--
> > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> > > index 18051b1cfce0..eb2d439a5334 100644
> > > --- a/arch/um/Kconfig
> > > +++ b/arch/um/Kconfig
> > > @@ -10,6 +10,7 @@ config UML
> > >  	select ARCH_HAS_FORTIFY_SOURCE
> > >  	select ARCH_HAS_GCOV_PROFILE_ALL
> > >  	select ARCH_HAS_KCOV
> > > +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > >  	select ARCH_HAS_STRNCPY_FROM_USER
> > >  	select ARCH_HAS_STRNLEN_USER
> > >  	select HAVE_ARCH_AUDITSYSCALL
> > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> > > index f238f7b33cdd..fdfba858ffc9 100644
> > > --- a/arch/x86/um/vdso/vma.c
> > > +++ b/arch/x86/um/vdso/vma.c
> > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct
> > > linux_binprm *bprm, int uses_interp)
> > >  {
> > >  	struct vm_area_struct *vma;
> > >  	struct mm_struct *mm = current->mm;
> > > +	unsigned long vm_flags;
> > >  	static struct vm_special_mapping vdso_mapping = {
> > >  		.name = "[vdso]",
> > >  	};
> > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct
> > > linux_binprm *bprm, int uses_interp)
> > >  		return -EINTR;
> > >
> > >  	vdso_mapping.pages = vdsop;
> > > +	vm_flags =
> > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > > +	vm_flags |= VM_SEALED_SYSMAP;
> > >  	vma = _install_special_mapping(mm, um_vdso_addr,
> > > PAGE_SIZE,
> > > -		VM_READ|VM_EXEC|
> > > -		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > > +		vm_flags,
> > >  		&vdso_mapping);
> > >
> > >  	mmap_write_unlock(mm);
> > > --
> > > 2.48.1.658.g4767266eb4-goog
> > >
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Benjamin Berg]

Hi,

On Tue, 2025-02-25 at 10:37 +0000, Lorenzo Stoakes wrote:
> On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> > Hi,
> > 
> > On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> > > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > 
> > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > > > the vdso.
> > > > 
> > > > Testing passes on UML.
> > > 
> > > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> > > _believe_) that UML has no need for problematic relocation so this is known to
> > > be good.
> > 
> > I may well be misreading this message, but this sounds to me that this
> > is a misinterpretation. So, just to clarify in case that is needed.
> > 
> > CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> > However, the UML kernel is a normal userspace application itself and
> > for this application to run, the host kernel must have the feature
> > disabled.
> > 
> > So, UML supports the feature. But it still *cannot* run on a host
> > machine that has the feature enabled.
> 
> Sigh ok. Apologies if I misunderstood.
> 
> Is there any point having this for the 'guest' system? I mean security wise are
> we concerned about sealing of system mappings?
> 
> I feel like having this here might just add confusion and churn if it's not
> useful.
> 
> If this is useless for UML guest, let's just drop this patch.

I figured it is not a lot of churn and there isn't really any cost to
enabling the feature.

That said, the only possible real-life use case I can see is doing MM
subsystem testing using UML. We certainly do not need the feature to
run our UML based wireless stack and driver tests.

Benjamin

> 
> > 
> > Benjamin
> > 
> > > 
> > > > 
> > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > > Tested-by: Benjamin Berg <benjamin.berg@intel.com>
> > > 
> > > Anyway I know UML has at any rate been confirmed to be good to go +
> > > patch looks
> > > fine, so:
> > > 
> > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 
> OK guess drop this tag please until we can figure this out, sorry Jeff.
> 
> > > 
> > > > ---
> > > >  arch/um/Kconfig        | 1 +
> > > >  arch/x86/um/vdso/vma.c | 6 ++++--
> > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> > > > index 18051b1cfce0..eb2d439a5334 100644
> > > > --- a/arch/um/Kconfig
> > > > +++ b/arch/um/Kconfig
> > > > @@ -10,6 +10,7 @@ config UML
> > > >  	select ARCH_HAS_FORTIFY_SOURCE
> > > >  	select ARCH_HAS_GCOV_PROFILE_ALL
> > > >  	select ARCH_HAS_KCOV
> > > > +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > >  	select ARCH_HAS_STRNCPY_FROM_USER
> > > >  	select ARCH_HAS_STRNLEN_USER
> > > >  	select HAVE_ARCH_AUDITSYSCALL
> > > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> > > > index f238f7b33cdd..fdfba858ffc9 100644
> > > > --- a/arch/x86/um/vdso/vma.c
> > > > +++ b/arch/x86/um/vdso/vma.c
> > > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct
> > > > linux_binprm *bprm, int uses_interp)
> > > >  {
> > > >  	struct vm_area_struct *vma;
> > > >  	struct mm_struct *mm = current->mm;
> > > > +	unsigned long vm_flags;
> > > >  	static struct vm_special_mapping vdso_mapping = {
> > > >  		.name = "[vdso]",
> > > >  	};
> > > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct
> > > > linux_binprm *bprm, int uses_interp)
> > > >  		return -EINTR;
> > > > 
> > > >  	vdso_mapping.pages = vdsop;
> > > > +	vm_flags =
> > > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > > > +	vm_flags |= VM_SEALED_SYSMAP;
> > > >  	vma = _install_special_mapping(mm, um_vdso_addr,
> > > > PAGE_SIZE,
> > > > -		VM_READ|VM_EXEC|
> > > > -		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > > > +		vm_flags,
> > > >  		&vdso_mapping);
> > > > 
> > > >  	mmap_write_unlock(mm);
> > > > --
> > > > 2.48.1.658.g4767266eb4-goog
> > > > 
> > 
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 01:24:49PM +0100, Benjamin Berg wrote:
> Hi,
>
> On Tue, 2025-02-25 at 10:37 +0000, Lorenzo Stoakes wrote:
> > On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> > > Hi,
> > >
> > > On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> > > > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > >
> > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > > > > the vdso.
> > > > >
> > > > > Testing passes on UML.
> > > >
> > > > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> > > > _believe_) that UML has no need for problematic relocation so this is known to
> > > > be good.
> > >
> > > I may well be misreading this message, but this sounds to me that this
> > > is a misinterpretation. So, just to clarify in case that is needed.
> > >
> > > CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> > > However, the UML kernel is a normal userspace application itself and
> > > for this application to run, the host kernel must have the feature
> > > disabled.
> > >
> > > So, UML supports the feature. But it still *cannot* run on a host
> > > machine that has the feature enabled.
> >
> > Sigh ok. Apologies if I misunderstood.
> >
> > Is there any point having this for the 'guest' system? I mean security wise are
> > we concerned about sealing of system mappings?
> >
> > I feel like having this here might just add confusion and churn if it's not
> > useful.
> >
> > If this is useless for UML guest, let's just drop this patch.
>
> I figured it is not a lot of churn and there isn't really any cost to
> enabling the feature.
>
> That said, the only possible real-life use case I can see is doing MM
> subsystem testing using UML. We certainly do not need the feature to
> run our UML based wireless stack and driver tests.

OK ack - my concern is users getting confused about this ironic host
vs. client thing, must disable the security feature in the _actual kernel_
to enable it in the client.

I'm not sure this is really worth it?

I mean I agree this isn't a _huge_ amount added here and I don't want to be
difficult - Jeff, Kees are you really keen on having this? Do you have
specific use cases in mind or was this just a 'because we can':>)

I guess if intent is to slowly add architectures, it's not totally insane
since we kinda know this one is ok so if that's what it is, probably won't
oppose it _too_ badly.

>
> Benjamin
>
> >
> > >
> > > Benjamin
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > > > Tested-by: Benjamin Berg <benjamin.berg@intel.com>
> > > >
> > > > Anyway I know UML has at any rate been confirmed to be good to go +
> > > > patch looks
> > > > fine, so:
> > > >
> > > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > OK guess drop this tag please until we can figure this out, sorry Jeff.

(to be clear this is just temporary while we establish what's up with this
situation! :>)

> >
> > > >
> > > > > ---
> > > > >  arch/um/Kconfig        | 1 +
> > > > >  arch/x86/um/vdso/vma.c | 6 ++++--
> > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> > > > > index 18051b1cfce0..eb2d439a5334 100644
> > > > > --- a/arch/um/Kconfig
> > > > > +++ b/arch/um/Kconfig
> > > > > @@ -10,6 +10,7 @@ config UML
> > > > >  	select ARCH_HAS_FORTIFY_SOURCE
> > > > >  	select ARCH_HAS_GCOV_PROFILE_ALL
> > > > >  	select ARCH_HAS_KCOV
> > > > > +	select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > > >  	select ARCH_HAS_STRNCPY_FROM_USER
> > > > >  	select ARCH_HAS_STRNLEN_USER
> > > > >  	select HAVE_ARCH_AUDITSYSCALL
> > > > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c
> > > > > index f238f7b33cdd..fdfba858ffc9 100644
> > > > > --- a/arch/x86/um/vdso/vma.c
> > > > > +++ b/arch/x86/um/vdso/vma.c
> > > > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct
> > > > > linux_binprm *bprm, int uses_interp)
> > > > >  {
> > > > >  	struct vm_area_struct *vma;
> > > > >  	struct mm_struct *mm = current->mm;
> > > > > +	unsigned long vm_flags;
> > > > >  	static struct vm_special_mapping vdso_mapping = {
> > > > >  		.name = "[vdso]",
> > > > >  	};
> > > > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct
> > > > > linux_binprm *bprm, int uses_interp)
> > > > >  		return -EINTR;
> > > > >
> > > > >  	vdso_mapping.pages = vdsop;
> > > > > +	vm_flags =
> > > > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > > > > +	vm_flags |= VM_SEALED_SYSMAP;
> > > > >  	vma = _install_special_mapping(mm, um_vdso_addr,
> > > > > PAGE_SIZE,
> > > > > -		VM_READ|VM_EXEC|
> > > > > -		VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > > > > +		vm_flags,
> > > > >  		&vdso_mapping);
> > > > >
> > > > >  	mmap_write_unlock(mm);
> > > > > --
> > > > > 2.48.1.658.g4767266eb4-goog
> > > > >
> > >
> > > Intel Deutschland GmbH
> > > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > > Tel: +49 89 99 8853-0, www.intel.de
> > > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> > > Chairperson of the Supervisory Board: Nicole Lau
> > > Registered Office: Munich
> > > Commercial Register: Amtsgericht Muenchen HRB 186928
>

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Johannes Berg]

On Tue, 2025-02-25 at 13:41 +0000, Lorenzo Stoakes wrote:
> > I figured it is not a lot of churn and there isn't really any cost to
> > enabling the feature.
> > 
> > That said, the only possible real-life use case I can see is doing MM
> > subsystem testing using UML. We certainly do not need the feature to
> > run our UML based wireless stack and driver tests.
> 
> OK ack - my concern is users getting confused about this ironic host
> vs. client thing, must disable the security feature in the _actual kernel_
> to enable it in the client.

Well, s/to enable it in the client/to run the client/, I guess.

I'm still a bit disappointed in the whole thing anyway - if this does
get enabled in e.g. ChromeOS (as it looks like), then it'll mean that
gvisor/rr/UML will never run on chromebooks, which ... I mean yeah who's
going to do that, so it's more of a purist disappointment I guess. Can't
run kunit on a chromebook then, for example. This looks much different
for more general purpose distros too.

I also don't really want to reopen a discussion that was probably had
before, but I did wonder now what the security downsides of having an
opt-out, e.g. a new ELF property, for skipping the sealings would be.
Perhaps, depending on the impact, even making that mean "no system
mappings at all", at least for UML I believe they're not needed in the
first place.


> I'm not sure this is really worth it?
> 
> I mean I agree this isn't a _huge_ amount added here and I don't want to be
> difficult - Jeff, Kees are you really keen on having this? Do you have
> specific use cases in mind or was this just a 'because we can':>)

There's always kunit that can run with UML, but I don't see tests being
added for this feature, in fact the only thing here is _disabling_ a
test. Maybe it should come with tests and then it'd be more interesting
;-)

The commit says "Testing passes on UML" but I'm not sure I see what
testing that might have been, per the cover letter Benjamin did that?

> I guess if intent is to slowly add architectures, it's not totally insane
> since we kinda know this one is ok so if that's what it is, probably won't
> oppose it _too_ badly.

I think it still makes _some_ sense to have it for the testing aspect,
but perhaps might then make sense to split it out of the series to avoid
all the confusion and submit it to UML separately later? Or just leave
it since you can always test with qemu.

johannes

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Kees Cook]

On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
>> Hi,
>>
>> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
>> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
>> > > From: Jeff Xu <jeffxu@chromium.org>
>> > >
>> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
>> > > the vdso.
>> > >
>> > > Testing passes on UML.
>> >
>> > Maybe expand on this by stating that it has been confirmed by Benjamin (I
>> > _believe_) that UML has no need for problematic relocation so this is known to
>> > be good.
>>
>> I may well be misreading this message, but this sounds to me that this
>> is a misinterpretation. So, just to clarify in case that is needed.
>>
>> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
>> However, the UML kernel is a normal userspace application itself and
>> for this application to run, the host kernel must have the feature
>> disabled.
>>
>> So, UML supports the feature. But it still *cannot* run on a host
>> machine that has the feature enabled.
>
>Sigh ok. Apologies if I misunderstood.
>
>Is there any point having this for the 'guest' system? I mean security wise are
>we concerned about sealing of system mappings?

UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me.

>
>I feel like having this here might just add confusion and churn if it's not
>useful.
>
>If this is useless for UML guest, let's just drop this patch.

But on the flip side, it's certainly not critical to have UML supported. I guess I just don't see a down side to keeping the patch.

-Kees


-- 
Kees Cook

Re: [PATCH v7 0/7] mseal system mappings

[Lorenzo Stoakes]

Jeff - looking further in this series, I asked for a couple things for this
series which you've not provided:

1. Some assurance based on code that the kernel-side code doesn't rely on
   VDSO/VVAR etc. mapping. I gave up waiting for this and went and checked
   myself, it looks fine for arm64, x86-64. But it might have been nice had
   you done it :) Apologies if you had and I just missed it.

2. Tests - could you please add some tests to assert that mremap() fails
   for VDSO for instance? You've edited an existing test for VDSO in x86 to
   skip the test should this be enabled, but this is not the same as a self
   test. And obviously doesn't cover arm64.

   This should be relatively strightforward right? You already have code
   for finding out whether VDSO is msealed, so just use that to see if you
   skip, then attempt mremap(), mmap() over etc. + assert it fails.

   Ideally these tests would cover all the cases you've changed.

Please do try to ensure you address requests from maintainers to save on
iterations, while I get the desire to shoot out new versions (I've been
guilty of this in the past), it makes life so much easier and will reduce
version count if you try to get everything done in a one go.

Having said the above, we're really not far off this being a viable
series. You just need to address comments here (+ in v6...) + provide some
tests.

Thanks!

On Mon, Feb 24, 2025 at 10:52:39PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> This is V7 version, addressing comments from V6, without code logic
> change.
>
> --------------------------------------------------
>
> History:
> V7:
>  - Remove cover letter from the first patch (Liam R. Howlett)
>  - Change macro name to VM_SEALED_SYSMAP (Liam R. Howlett)
>  - logging and fclose() in selftest (Liam R. Howlett)
>
> V6:
>   https://lore.kernel.org/all/20250224174513.3600914-1-jeffxu@google.com/
>

Nitty, but it's really useful to actually include the history for all of
these.

> V5
>   https://lore.kernel.org/all/20250212032155.1276806-1-jeffxu@google.com/
>
> V4:
>   https://lore.kernel.org/all/20241125202021.3684919-1-jeffxu@google.com/
>
> V3:
>   https://lore.kernel.org/all/20241113191602.3541870-1-jeffxu@google.com/
>
> V2:
>   https://lore.kernel.org/all/20241014215022.68530-1-jeffxu@google.com/
>
> V1:
>   https://lore.kernel.org/all/20241004163155.3493183-1-jeffxu@google.com/
>
> --------------------------------------------------
> As discussed during mseal() upstream process [1], mseal() protects
> the VMAs of a given virtual memory range against modifications, such
> as the read/write (RW) and no-execute (NX) bits. For complete
> descriptions of memory sealing, please see mseal.rst [2].
>
> The mseal() is useful to mitigate memory corruption issues where a
> corrupted pointer is passed to a memory management system. For
> example, such an attacker primitive can break control-flow integrity
> guarantees since read-only memory that is supposed to be trusted can
> become writable or .text pages can get remapped.
>
> The system mappings are readonly only, memory sealing can protect
> them from ever changing to writable or unmmap/remapped as different
> attributes.
>
> System mappings such as vdso, vvar, and sigpage (arm), vectors (arm)
> are created by the kernel during program initialization, and could
> be sealed after creation.
>
> Unlike the aforementioned mappings, the uprobe mapping is not
> established during program startup. However, its lifetime is the same
> as the process's lifetime [3]. It could be sealed from creation.
>
> The vsyscall on x86-64 uses a special address (0xffffffffff600000),
> which is outside the mm managed range. This means mprotect, munmap, and
> mremap won't work on the vsyscall. Since sealing doesn't enhance
> the vsyscall's security, it is skipped in this patch. If we ever seal
> the vsyscall, it is probably only for decorative purpose, i.e. showing
> the 'sl' flag in the /proc/pid/smaps. For this patch, it is ignored.
>
> It is important to note that the CHECKPOINT_RESTORE feature (CRIU) may
> alter the system mappings during restore operations. UML(User Mode Linux)
> and gVisor, rr are also known to change the vdso/vvar mappings.
> Consequently, this feature cannot be universally enabled across all
> systems. As such, CONFIG_MSEAL_SYSTEM_MAPPINGS is disabled by default.
>
> To support mseal of system mappings, architectures must define
> CONFIG_ARCH_HAS_MSEAL_SYSTEM_MAPPINGS and update their special mappings
> calls to pass mseal flag. Additionally, architectures must confirm they
> do not unmap/remap system mappings during the process lifetime.
>
> In this version, we've improved the handling of system mapping sealing from
> previous versions, instead of modifying the _install_special_mapping
> function itself, which would affect all architectures, we now call
> _install_special_mapping with a sealing flag only within the specific
> architecture that requires it. This targeted approach offers two key
> advantages: 1) It limits the code change's impact to the necessary
> architectures, and 2) It aligns with the software architecture by keeping
> the core memory management within the mm layer, while delegating the
> decision of sealing system mappings to the individual architecture, which
> is particularly relevant since 32-bit architectures never require sealing.
>
> Prior to this patch series, we explored sealing special mappings from
> userspace using glibc's dynamic linker. This approach revealed several
> issues:
> - The PT_LOAD header may report an incorrect length for vdso, (smaller
>   than its actual size). The dynamic linker, which relies on PT_LOAD
>   information to determine mapping size, would then split and partially
>   seal the vdso mapping. Since each architecture has its own vdso/vvar
>   code, fixing this in the kernel would require going through each
>   archiecture. Our initial goal was to enable sealing readonly mappings,
>   e.g. .text, across all architectures, sealing vdso from kernel since
>   creation appears to be simpler than sealing vdso at glibc.
> - The [vvar] mapping header only contains address information, not length
>   information. Similar issues might exist for other special mappings.
> - Mappings like uprobe are not covered by the dynamic linker,
>   and there is no effective solution for them.
>
> This feature's security enhancements will benefit ChromeOS, Android,
> and other high security systems.
>
> Testing:
> This feature was tested on ChromeOS and Android for both x86-64 and ARM64.
> - Enable sealing and verify vdso/vvar, sigpage, vector are sealed properly,
>   i.e. "sl" shown in the smaps for those mappings, and mremap is blocked.
> - Passing various automation tests (e.g. pre-checkin) on ChromeOS and
>   Android to ensure the sealing doesn't affect the functionality of
>   Chromebook and Android phone.
>
> I also tested the feature on Ubuntu on x86-64:
> - With config disabled, vdso/vvar is not sealed,
> - with config enabled, vdso/vvar is sealed, and booting up Ubuntu is OK,
>   normal operations such as browsing the web, open/edit doc are OK.
>
> In addition, Benjamin Berg tested this on UML.
>
> Link: https://lore.kernel.org/all/20240415163527.626541-1-jeffxu@chromium.org/ [1]
> Link: Documentation/userspace-api/mseal.rst [2]
> Link: https://lore.kernel.org/all/CABi2SkU9BRUnqf70-nksuMCQ+yyiWjo3fM4XkRkL-NrCZxYAyg@mail.gmail.com/ [3]
>
>
>
>
> Jeff Xu (7):
>   mseal, system mappings: kernel config and header change
>   selftests: x86: test_mremap_vdso: skip if vdso is msealed
>   mseal, system mappings: enable x86-64
>   mseal, system mappings: enable arm64
>   mseal, system mappings: enable uml architecture
>   mseal, system mappings: uprobe mapping
>   mseal, system mappings: update mseal.rst
>
>  Documentation/userspace-api/mseal.rst         |  7 +++
>  arch/arm64/Kconfig                            |  1 +
>  arch/arm64/kernel/vdso.c                      | 22 +++++++---
>  arch/um/Kconfig                               |  1 +
>  arch/x86/Kconfig                              |  1 +
>  arch/x86/entry/vdso/vma.c                     | 16 ++++---
>  arch/x86/um/vdso/vma.c                        |  6 ++-
>  include/linux/mm.h                            | 10 +++++
>  init/Kconfig                                  | 18 ++++++++
>  kernel/events/uprobes.c                       |  5 ++-
>  security/Kconfig                              | 18 ++++++++
>  .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
>  12 files changed, 132 insertions(+), 16 deletions(-)
>
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Liam R. Howlett]

* jeffxu@chromium.org <jeffxu@chromium.org> [250224 17:52]:
> From: Jeff Xu <jeffxu@chromium.org>
> 
> Provide infrastructure to mseal system mappings. Establish
> two kernel configs (CONFIG_MSEAL_SYSTEM_MAPPINGS,
> ARCH_HAS_MSEAL_SYSTEM_MAPPINGS) and VM_SEALED_SYSMAP
> macro for future patches.
> 
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  include/linux/mm.h | 10 ++++++++++
>  init/Kconfig       | 18 ++++++++++++++++++
>  security/Kconfig   | 18 ++++++++++++++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b1068ddcbb7..8b800941678d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4155,4 +4155,14 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
>  int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
>  int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>  
> +
> +/*
> + * mseal of userspace process's system mappings.
> + */
> +#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> +#define VM_SEALED_SYSMAP	VM_SEALED
> +#else
> +#define VM_SEALED_SYSMAP	VM_NONE
> +#endif
> +
>  #endif /* _LINUX_MM_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index d0d021b3fa3b..07435e33f965 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
>  config ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	bool
>  
> +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS

I think we said ARCH_SUPPORTS_ for software features in v5 of the series
[1].  Can we also make this change please?

...

Thanks,
Liam

[1]. https://lore.kernel.org/all/202502131142.F5EE115C3A@keescook/

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote:
>
>
> On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> >> Hi,
> >>
> >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> >> > > From: Jeff Xu <jeffxu@chromium.org>
> >> > >
> >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> >> > > the vdso.
> >> > >
> >> > > Testing passes on UML.
> >> >
> >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> >> > _believe_) that UML has no need for problematic relocation so this is known to
> >> > be good.
> >>
> >> I may well be misreading this message, but this sounds to me that this
> >> is a misinterpretation. So, just to clarify in case that is needed.
> >>
> >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> >> However, the UML kernel is a normal userspace application itself and
> >> for this application to run, the host kernel must have the feature
> >> disabled.
> >>
> >> So, UML supports the feature. But it still *cannot* run on a host
> >> machine that has the feature enabled.
> >
> >Sigh ok. Apologies if I misunderstood.
> >
> >Is there any point having this for the 'guest' system? I mean security wise are
> >we concerned about sealing of system mappings?
>
> UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me.
>

'Having sealing working' you mean system sealing? Because mseal works fine
(presumably in UML, not tried myself!)

System msealing lacks any test in this series (I did ask for them...), certainly
no kunit tests, so this seems a bit theoretical? Unless you're talking about the
theoretical interaction of kunit tests and VDSO sealing?

I mean can't we just introduce this at the time if we believe this'd be useful?

Adding stuff for entirely theoretical reasons is generally frowned upon and
isn't that the (at least one) definition of churn?

Can you give a really specific reason why this needs to be added?

For x86-64, arm64 the reason is obvious (albeit one can argue how much security
benefit msealing the VDSO truly gives).

For UML it just isn't, at all.

> >
> >I feel like having this here might just add confusion and churn if it's not
> >useful.
> >
> >If this is useless for UML guest, let's just drop this patch.
>
> But on the flip side, it's certainly not critical to have UML supported. I guess I just don't see a down side to keeping the patch.

Right, presumably you disagree that it might be confusing that to test mseal
_system_ sealing (not mseal itself) by enabling mseal system sealing in UML but
having to disable it in the host?

Because that seems really confusing to me (and why I explicitly cited it as a
reason I'm confused here, even the exchange above is confused).

If there benefit is basically theoretical/why not/nil, and the downside is
confusion + churn (albeit small churn), why not just wait until somebody writes
kunit tests?

If I saw a list of supported arches including UML, but also saw a note about it
not working in UML I'd definitely be confused.

Generally I'm not a fan of adding features mid-way through a series, the
revisions are meant to be refinements of the original, not an evolving thing.

So in general I'd prefer this to be added if + when we need it for something.

>
> -Kees
>
>
> --
> Kees Cook

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 10:22:56AM -0500, Liam R. Howlett wrote:
> * jeffxu@chromium.org <jeffxu@chromium.org> [250224 17:52]:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide infrastructure to mseal system mappings. Establish
> > two kernel configs (CONFIG_MSEAL_SYSTEM_MAPPINGS,
> > ARCH_HAS_MSEAL_SYSTEM_MAPPINGS) and VM_SEALED_SYSMAP
> > macro for future patches.
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  include/linux/mm.h | 10 ++++++++++
> >  init/Kconfig       | 18 ++++++++++++++++++
> >  security/Kconfig   | 18 ++++++++++++++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7b1068ddcbb7..8b800941678d 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4155,4 +4155,14 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
> >  int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
> >  int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >
> > +
> > +/*
> > + * mseal of userspace process's system mappings.
> > + */
> > +#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> > +#define VM_SEALED_SYSMAP	VM_SEALED
> > +#else
> > +#define VM_SEALED_SYSMAP	VM_NONE
> > +#endif
> > +
> >  #endif /* _LINUX_MM_H */
> > diff --git a/init/Kconfig b/init/Kconfig
> > index d0d021b3fa3b..07435e33f965 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
> >  config ARCH_HAS_MEMBARRIER_SYNC_CORE
> >  	bool
> >
> > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>
> I think we said ARCH_SUPPORTS_ for software features in v5 of the series
> [1].  Can we also make this change please?

Jeff - I'm going to risk sounding like a typical, grumpy, kernel maintainer
here (sorry! :P) but please, please do ensure that you address all prior
feedback.

It may be worth even, when things settle down here, putting together a list
of things you intend to address in the next round and double-check you've
covered everything off.

Because obviously when this happens once it's probably an oversight, but it
seems you've failed to address at least 4 separate things requested of you
here. It's obivously s going to make this a faaar more drawn out process
for al linvolved.

Again, to be clear, as to allay LWN articles about 'Stoakes urging caution'
or what not and related drama - my intent in saying this is purely that _I
want this series to land_. So I say it merely to help expedite this! :)

Cheers, Lorenzo

>
> ...
>
> Thanks,
> Liam
>
> [1]. https://lore.kernel.org/all/202502131142.F5EE115C3A@keescook/

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Kees Cook]

On Tue, Feb 25, 2025 at 03:31:06PM +0000, Lorenzo Stoakes wrote:
> On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote:
> >
> >
> > On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> > >> Hi,
> > >>
> > >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> > >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > >> > > From: Jeff Xu <jeffxu@chromium.org>
> > >> > >
> > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > >> > > the vdso.
> > >> > >
> > >> > > Testing passes on UML.
> > >> >
> > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> > >> > _believe_) that UML has no need for problematic relocation so this is known to
> > >> > be good.
> > >>
> > >> I may well be misreading this message, but this sounds to me that this
> > >> is a misinterpretation. So, just to clarify in case that is needed.
> > >>
> > >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> > >> However, the UML kernel is a normal userspace application itself and
> > >> for this application to run, the host kernel must have the feature
> > >> disabled.
> > >>
> > >> So, UML supports the feature. But it still *cannot* run on a host
> > >> machine that has the feature enabled.
> > >
> > >Sigh ok. Apologies if I misunderstood.
> > >
> > >Is there any point having this for the 'guest' system? I mean security wise are
> > >we concerned about sealing of system mappings?
> >
> > UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me.
> >
> 
> 'Having sealing working' you mean system sealing? Because mseal works fine
> (presumably in UML, not tried myself!)

Sorry, yes, I mean "system mapping msealing".

> 
> System msealing lacks any test in this series (I did ask for them...), certainly
> no kunit tests, so this seems a bit theoretical? Unless you're talking about the
> theoretical interaction of kunit tests and VDSO sealing?

Right, I meant theoretical interaction, but it would be useful for
future KUnit tests of system mapping msealing too.

> I mean can't we just introduce this at the time if we believe this'd be useful?

Perhaps adding it as part of adding some KUnit tests that exercise the
system mapping msealing would be the most sensible.

> Generally I'm not a fan of adding features mid-way through a series, the
> revisions are meant to be refinements of the original, not an evolving thing.
> 
> So in general I'd prefer this to be added if + when we need it for something.

Yup, makes sense. And it may be that KUnit tests need to exercise more
than what UML can support, so even the KUnit idea may be invalid.

Jeff, let's leave off UML for this initial "minimum viable feature"
series, unless there is a strong reason to keep it.

-- 
Kees Cook

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Jeff Xu]

On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> >
> > Production release testing passes on Android and Chrome OS.
>
> This is pretty limited (yes yes I know android is massive etc. but we must
> account for all the weird and wonderful arm64 devices out there in context of
> upstream :)
>
> Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> nervous of that.
>
Can you please point out where PPC munmap/mremap the vdso ?

Previously, when you mentioned that, I thought you meant user space in
PPC, I didn't realize that you meant that kernel code in PPC.  I
tried, but didn't find anything, hence asking.

Thanks.
-Jeff

Re: [PATCH v7 2/7] selftests: x86: test_mremap_vdso: skip if vdso is msealed

[Jeff Xu]

On Mon, Feb 24, 2025 at 10:15 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Mon, Feb 24, 2025 at 10:52:41PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Add code to detect if the vdso is memory sealed, skip the test
> > if it is.
>
> I feel this is a little succinct of a commit message, but I guess it gets to the
> heart of what you're doing here.
>
> Fundamentally I mean it makes sense, but I'm concerned that x86 has a test
> -expliictly checking- whether mremap() of VDSO is possible - are there cases
> where x86 might want to do this internal to the kernel?
>
> I guess not since this is essentially a userland self test and probably
> asserting you can do this in the way rr, etc. do.
>
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > Reviewed-by: Kees Cook <kees@kernel.org>
>
> Anyway, this aside, this looks fine, aside from nit below, so:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> > ---
> >  .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c
> > index d53959e03593..94bee6e0c813 100644
> > --- a/tools/testing/selftests/x86/test_mremap_vdso.c
> > +++ b/tools/testing/selftests/x86/test_mremap_vdso.c
> > @@ -14,6 +14,7 @@
> >  #include <errno.h>
> >  #include <unistd.h>
> >  #include <string.h>
> > +#include <stdbool.h>
> >
> >  #include <sys/mman.h>
> >  #include <sys/auxv.h>
> > @@ -55,13 +56,55 @@ static int try_to_remap(void *vdso_addr, unsigned long size)
> >
> >  }
> >
> > +#define VDSO_NAME "[vdso]"
> > +#define VMFLAGS "VmFlags:"
> > +#define MSEAL_FLAGS "sl"
> > +#define MAX_LINE_LEN 512
> > +
> > +bool vdso_sealed(FILE *maps)
>
> Should be static?
>
sure.

> > +{
> > +     char line[MAX_LINE_LEN];
> > +     bool has_vdso = false;
> > +
> > +     while (fgets(line, sizeof(line), maps)) {
> > +             if (strstr(line, VDSO_NAME))
> > +                     has_vdso = true;
> > +
> > +             if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) {
> > +                     if (strstr(line, MSEAL_FLAGS))
> > +                             return true;
> > +
> > +                     return false;
> > +             }
> > +     }
> > +
> > +     return false;
> > +}
> > +
> >  int main(int argc, char **argv, char **envp)
> >  {
> >       pid_t child;
> > +     FILE *maps;
> >
> >       ksft_print_header();
> >       ksft_set_plan(1);
> >
> > +     maps = fopen("/proc/self/smaps", "r");
> > +     if (!maps) {
> > +             ksft_test_result_skip(
> > +                     "Could not open /proc/self/smaps, errno=%d\n",
> > +                      errno);
> > +
> > +             return 0;
> > +     }
> > +
> > +     if (vdso_sealed(maps)) {
> > +             ksft_test_result_skip("vdso is sealed\n");
> > +             return 0;
> > +     }
> > +
> > +     fclose(maps);
> > +
> >       child = fork();
> >       if (child == -1)
> >               ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);
> > --
> > 2.48.1.658.g4767266eb4-goog
> >

Re: [PATCH v7 5/7] mseal, system mappings: enable uml architecture

[Jeff Xu]

On Tue, Feb 25, 2025 at 10:38 AM Kees Cook <kees@kernel.org> wrote:
>
> On Tue, Feb 25, 2025 at 03:31:06PM +0000, Lorenzo Stoakes wrote:
> > On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote:
> > >
> > >
> > > On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> > > >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote:
> > > >> Hi,
> > > >>
> > > >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote:
> > > >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote:
> > > >> > > From: Jeff Xu <jeffxu@chromium.org>
> > > >> > >
> > > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering
> > > >> > > the vdso.
> > > >> > >
> > > >> > > Testing passes on UML.
> > > >> >
> > > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I
> > > >> > _believe_) that UML has no need for problematic relocation so this is known to
> > > >> > be good.
> > > >>
> > > >> I may well be misreading this message, but this sounds to me that this
> > > >> is a misinterpretation. So, just to clarify in case that is needed.
> > > >>
> > > >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel.
> > > >> However, the UML kernel is a normal userspace application itself and
> > > >> for this application to run, the host kernel must have the feature
> > > >> disabled.
> > > >>
> > > >> So, UML supports the feature. But it still *cannot* run on a host
> > > >> machine that has the feature enabled.
> > > >
> > > >Sigh ok. Apologies if I misunderstood.
> > > >
> > > >Is there any point having this for the 'guest' system? I mean security wise are
> > > >we concerned about sealing of system mappings?
> > >
> > > UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me.
> > >
> >
> > 'Having sealing working' you mean system sealing? Because mseal works fine
> > (presumably in UML, not tried myself!)
>
> Sorry, yes, I mean "system mapping msealing".
>
> >
> > System msealing lacks any test in this series (I did ask for them...), certainly
> > no kunit tests, so this seems a bit theoretical? Unless you're talking about the
> > theoretical interaction of kunit tests and VDSO sealing?
>
> Right, I meant theoretical interaction, but it would be useful for
> future KUnit tests of system mapping msealing too.
>
> > I mean can't we just introduce this at the time if we believe this'd be useful?
>
> Perhaps adding it as part of adding some KUnit tests that exercise the
> system mapping msealing would be the most sensible.
>
> > Generally I'm not a fan of adding features mid-way through a series, the
> > revisions are meant to be refinements of the original, not an evolving thing.
> >
> > So in general I'd prefer this to be added if + when we need it for something.
>
> Yup, makes sense. And it may be that KUnit tests need to exercise more
> than what UML can support, so even the KUnit idea may be invalid.
>
> Jeff, let's leave off UML for this initial "minimum viable feature"
> series, unless there is a strong reason to keep it.
>
Sure.
It will be removed unless someone raises a strong reason to keep it.
UML can be added when future KUnit tests need it.

Thanks
-Jeff


> --
> Kees Cook

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Jeff Xu]

On Tue, Feb 25, 2025 at 7:23 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * jeffxu@chromium.org <jeffxu@chromium.org> [250224 17:52]:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide infrastructure to mseal system mappings. Establish
> > two kernel configs (CONFIG_MSEAL_SYSTEM_MAPPINGS,
> > ARCH_HAS_MSEAL_SYSTEM_MAPPINGS) and VM_SEALED_SYSMAP
> > macro for future patches.
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  include/linux/mm.h | 10 ++++++++++
> >  init/Kconfig       | 18 ++++++++++++++++++
> >  security/Kconfig   | 18 ++++++++++++++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7b1068ddcbb7..8b800941678d 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4155,4 +4155,14 @@ int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *st
> >  int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status);
> >  int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >
> > +
> > +/*
> > + * mseal of userspace process's system mappings.
> > + */
> > +#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> > +#define VM_SEALED_SYSMAP     VM_SEALED
> > +#else
> > +#define VM_SEALED_SYSMAP     VM_NONE
> > +#endif
> > +
> >  #endif /* _LINUX_MM_H */
> > diff --git a/init/Kconfig b/init/Kconfig
> > index d0d021b3fa3b..07435e33f965 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
> >  config ARCH_HAS_MEMBARRIER_SYNC_CORE
> >       bool
> >
> > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
>
> I think we said ARCH_SUPPORTS_ for software features in v5 of the series
> [1].  Can we also make this change please?
>
Sure.



> ...
>
> Thanks,
> Liam
>
> [1]. https://lore.kernel.org/all/202502131142.F5EE115C3A@keescook/

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Jeff Xu]

On Mon, Feb 24, 2025 at 10:24 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Mon, Feb 24, 2025 at 10:52:45PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide support to mseal the uprobe mapping.
> >
> > Unlike other system mappings, the uprobe mapping is not
> > established during program startup. However, its lifetime is the same
> > as the process's lifetime. It could be sealed from creation.
> >
>
> I thought we agreed not to enable this for now? What testing
> have you done to ensure this is functional?
>
I honestly don't know much about uprobe. I don't recall that I agree
to ignore that though.

As indicated in the cover letter, it is my understanding that uprobe's
mapping's lifetime are the same as process's lifetime, thus sealable.
[1]
Oleg Nesterov, also cc, seems OK with mseal it in the early version of
this patch [2]

Are there any potential downsides of doing this? If yes, I can remove it.

I'm also looking at Oleg to give more guidance on this :-), or if
there are some functional tests that I need to do for uprobe.


[1] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/
[2] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/

> I mean is this literally _all_ uprobe mappings now being sealed?
>
> I'd really like some more assurances on this one. And what are you
> mitigating by sealing these? I get VDSO (kinda) but uprobes?
>
> You really need to provide more justification here.

Sure. In our threat model, we need to seal all r-x, r--, and --x
mappings to prevent them from becoming writable. This applies to all
mappings, regardless of whether they're created by the kernel or
dynamic linker.


> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  kernel/events/uprobes.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 2ca797cbe465..8dcdfa0d306b 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1662,6 +1662,7 @@ static const struct vm_special_mapping xol_mapping = {
> >  static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> >  {
> >       struct vm_area_struct *vma;
> > +     unsigned long vm_flags;
> >       int ret;
> >
> >       if (mmap_write_lock_killable(mm))
> > @@ -1682,8 +1683,10 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> >               }
> >       }
> >
> > +     vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> > +     vm_flags |= VM_SEALED_SYSMAP;
> >       vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > -                             VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > +                             vm_flags,
> >                               &xol_mapping);
> >       if (IS_ERR(vma)) {
> >               ret = PTR_ERR(vma);
> > --
> > 2.48.1.658.g4767266eb4-goog
> >

(no subject)

[Jeff Xu]

On Tue, Feb 25, 2025 at 7:18 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> Jeff - looking further in this series, I asked for a couple things for this
> series which you've not provided:
>
> 1. Some assurance based on code that the kernel-side code doesn't rely on
>    VDSO/VVAR etc. mapping. I gave up waiting for this and went and checked
>    myself, it looks fine for arm64, x86-64. But it might have been nice had
>    you done it :) Apologies if you had and I just missed it.
>
Thanks for checking this.
Do ppc in kernel code unmap/remap  vdso ?

I am aware that userspace can remap/unmap special mappings, but I
don't know if the kernel will remap/unmap a special mapping. Could you
please point out the code ?


> 2. Tests - could you please add some tests to assert that mremap() fails
>    for VDSO for instance? You've edited an existing test for VDSO in x86 to
>    skip the test should this be enabled, but this is not the same as a self
>    test. And obviously doesn't cover arm64.
>
>    This should be relatively strightforward right? You already have code
>    for finding out whether VDSO is msealed, so just use that to see if you
>    skip, then attempt mremap(), mmap() over etc. + assert it fails.
>
>    Ideally these tests would cover all the cases you've changed.
>
It is not as easy.

The config is disabled by default. And I don't know a way to detect
KCONFIG  from selftest itself. Without this, I can't reasonably
determine the test result.

Re: [PATCH v7 0/7] mseal system mappings

[Jeff Xu]

On Tue, Feb 25, 2025 at 2:32 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> BTW can we please drop the 'mseal, system mappings' prefixes on this
> series, it's really weird and makes it really hard for me to actually read
> the individual summary lines for each commit. 'mseal:' will do.
>

I am not sure.
I had comments about adding mseal, system mappings, as prefixes, and I
think it is reasonable.

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Jeff Xu]

On Mon, Feb 24, 2025 at 5:03 PM Kees Cook <kees@kernel.org> wrote:
>
> On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> > covering the vdso, vvar, vvar_vclock.
> >
> > Production release testing passes on Android and Chrome OS.
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  arch/x86/Kconfig          |  1 +
> >  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 87198d957e2f..8fa17032ca46 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -26,6 +26,7 @@ config X86_64
> >       depends on 64BIT
> >       # Options that are inherently 64-bit kernel only:
> >       select ARCH_HAS_GIGANTIC_PAGE
> > +     select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> >       select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> >       select ARCH_SUPPORTS_PER_VMA_LOCK
> >       select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> > index 39e6efc1a9ca..1b1c009f20a8 100644
> > --- a/arch/x86/entry/vdso/vma.c
> > +++ b/arch/x86/entry/vdso/vma.c
> > @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> >       struct mm_struct *mm = current->mm;
> >       struct vm_area_struct *vma;
> >       unsigned long text_start;
> > +     unsigned long vm_flags;
> >       int ret = 0;
> >
> >       if (mmap_write_lock_killable(mm))
> > @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> >       /*
> >        * MAYWRITE to allow gdb to COW and set breakpoints
> >        */
> > +     vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > +     vm_flags |= VM_SEALED_SYSMAP;
> >       vma = _install_special_mapping(mm,
> >                                      text_start,
> >                                      image->size,
> > -                                    VM_READ|VM_EXEC|
> > -                                    VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > +                                    vm_flags,
> >                                      &vdso_mapping);
>
> I think these (in all patches) were supposed to be reworked without the
> "vm_flags" variable addition?
>
OK.

> --
> Kees Cook

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Jeff Xu]

On Tue, Feb 25, 2025 at 12:08 AM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> Hi Jeff,
>
> On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> > covering the vdso, vvar, vvar_vclock.
> >
> > Production release testing passes on Android and Chrome OS.
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  arch/x86/Kconfig          |  1 +
> >  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 87198d957e2f..8fa17032ca46 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -26,6 +26,7 @@ config X86_64
> >       depends on 64BIT
> >       # Options that are inherently 64-bit kernel only:
> >       select ARCH_HAS_GIGANTIC_PAGE
> > +     select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> >       select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> >       select ARCH_SUPPORTS_PER_VMA_LOCK
> >       select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> > index 39e6efc1a9ca..1b1c009f20a8 100644
> > --- a/arch/x86/entry/vdso/vma.c
> > +++ b/arch/x86/entry/vdso/vma.c
> > @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> >       struct mm_struct *mm = current->mm;
> >       struct vm_area_struct *vma;
> >       unsigned long text_start;
> > +     unsigned long vm_flags;
> >       int ret = 0;
> >
> >       if (mmap_write_lock_killable(mm))
> > @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> >       /*
> >        * MAYWRITE to allow gdb to COW and set breakpoints
> >        */
> > +     vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > +     vm_flags |= VM_SEALED_SYSMAP;
> >       vma = _install_special_mapping(mm,
> >                                      text_start,
> >                                      image->size,
> > -                                    VM_READ|VM_EXEC|
> > -                                    VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > +                                    vm_flags,
> >                                      &vdso_mapping);
> >
> >       if (IS_ERR(vma)) {
> > @@ -276,11 +278,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> >               goto up_fail;
> >       }
> >
> > +     vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
> > +     vm_flags |= VM_SEALED_SYSMAP;
> >       vma = _install_special_mapping(mm,
> >                                      addr,
> >                                      (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
> > -                                    VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
> > -                                    VM_PFNMAP,
> > +                                    vm_flags,
> >                                      &vvar_mapping);
>
> This hunk (and the vvar mapping in the arm64 patch) will conflict with my
> "Generic vDSO datapage" series.
> That series is already part of the tip tree (branch timers/vdso) and scheduled
> for the next merge window.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/vdso
>
> The conflict resolution is fairly easy:
> Move the new flag logic into lib/vdso/datastore.c
>
Thank you for bringing this to my attention.

In your change,  it seems lib/vdso/datastore.c implements a
vdso_install_vvar_mapping(), then all the architectures call this
function.

So merging conflict won't be as straightforward.  Maybe a better
approach is that I continue resolving all the comments, based on the
latest main. Then wait for your change to be merged and submit another
version.

Thanks
-Jeff

-Jeff

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Jeff Xu]

On Mon, Feb 24, 2025 at 10:05 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
> > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > +     bool
> > +     help
> > +       Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> > +
> > +       A 64-bit kernel is required for the memory sealing feature.
> > +       No specific hardware features from the CPU are needed.
> > +
> > +       To enable this feature, the architecture needs to update their
> > +       special mappings calls to include the sealing flag and confirm
> > +       that it doesn't unmap/remap system mappings during the life
> > +       time of the process. After the architecture enables this, a
> > +       distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> > +       to the feature.
>
> Architectures also need to be confirmed not to require any form of VDSO
> relocation, which as discussed in previous series some arches appear to
> need to do. I'd mention that here.
>
This might need clarification, the system mapping includes vdso, right
? Why the focus on vdso ?

The sentence  "... it doesn't unmap/remap system mappings during the
lifetime of the process."  already cover what you want here, I think.


> > +
> > +       For complete descriptions of memory sealing, please see
> > +       Documentation/userspace-api/mseal.rst
> > +
> >  config HAVE_PERF_EVENTS
> >       bool
> >       help
> > diff --git a/security/Kconfig b/security/Kconfig
> > index f10dbf15c294..15a86a952910 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -51,6 +51,24 @@ config PROC_MEM_NO_FORCE
> >
> >  endchoice
> >
> > +config MSEAL_SYSTEM_MAPPINGS
> > +     bool "mseal system mappings"
> > +     depends on 64BIT
> > +     depends on ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > +     depends on !CHECKPOINT_RESTORE
> > +     help
> > +       Seal system mappings such as vdso, vvar, sigpage, uprobes, etc.
>
> Let's be specific here, 'etc.' could mean _anything_. Also you aren't
> sealing most of this, let's just list what you are _actually_ sealing
> here. Which is AFAIK VDSO only?
>
I will remove "etc" and list all the mappings.

Those mappings are:
vdso, vvar, vvar_vclock, vectors (arm compact-mode) and sigpage (arm
compact-mode), uprobe.

We seal all system mappings that x86-64 and arm64 have.

> You can update this later as time goes on if/when you expand this.
>
> > +
> > +       A 64-bit kernel is required for the memory sealing feature.
> > +       No specific hardware features from the CPU are needed.
> > +
> > +       Note: CHECKPOINT_RESTORE, UML, gVisor, rr are known to relocate or
> > +       unmap system mapping, therefore this config can't be enabled
> > +       universally.
>
> Thanks for putting this here, appreciate it!
>
> Could we tweak this though? I'd like to make it crystal clear, so I don't
> think 'note' sufficies and this sounds a little too vague.
>
> I think 'warning' is more appropriate here since you're breaking things for
> people who might be unaware. And we need to say this -breaks- programs:
>
> WARNING: This feature breaks programs which rely on relocating or
>          unmapping system mappings.
>
>          Known broken software at the time of writing includes
>          CHECKPOINT_RESTORE, UML, gVisor and rr.
>
> I think this is critical.
>
Sure.

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > >
> > > Production release testing passes on Android and Chrome OS.
> >
> > This is pretty limited (yes yes I know android is massive etc. but we must
> > account for all the weird and wonderful arm64 devices out there in context of
> > upstream :)
> >
> > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > nervous of that.
> >
> Can you please point out where PPC munmap/mremap the vdso ?
>
> Previously, when you mentioned that, I thought you meant user space in
> PPC, I didn't realize that you meant that kernel code in PPC.  I
> tried, but didn't find anything, hence asking.

Jeff, please stick to replying to review. 'Have you looking through all
arm64-code'.

I ended up doing this myself yesterday and found no issues, as with x86-64.

I said I'm _pretty sure_ PPC does this. Liam mentioned something about
it. We can discuss it, and I can find specifics if + when you try to add
this to PPC.

Please try to respect my time...

>
> Thanks.
> -Jeff

Re: your mail

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 04:12:40PM -0800, Jeff Xu wrote:
> On Tue, Feb 25, 2025 at 7:18 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > Jeff - looking further in this series, I asked for a couple things for this
> > series which you've not provided:
> >
> > 1. Some assurance based on code that the kernel-side code doesn't rely on
> >    VDSO/VVAR etc. mapping. I gave up waiting for this and went and checked
> >    myself, it looks fine for arm64, x86-64. But it might have been nice had
> >    you done it :) Apologies if you had and I just missed it.
> >
> Thanks for checking this.
> Do ppc in kernel code unmap/remap  vdso ?
>
> I am aware that userspace can remap/unmap special mappings, but I
> don't know if the kernel will remap/unmap a special mapping. Could you
> please point out the code ?

Again as discussed in other thread, let's leave this until as/when you try
to attack PPC. I am not 100% this is the case, I may be mistaken sure, but
gathered it _might_ be.

>
>
> > 2. Tests - could you please add some tests to assert that mremap() fails
> >    for VDSO for instance? You've edited an existing test for VDSO in x86 to
> >    skip the test should this be enabled, but this is not the same as a self
> >    test. And obviously doesn't cover arm64.
> >
> >    This should be relatively strightforward right? You already have code
> >    for finding out whether VDSO is msealed, so just use that to see if you
> >    skip, then attempt mremap(), mmap() over etc. + assert it fails.
> >
> >    Ideally these tests would cover all the cases you've changed.
> >
> It is not as easy.
>
> The config is disabled by default. And I don't know a way to detect
> KCONFIG  from selftest itself. Without this, I can't reasonably
> determine the test result.

Please in future let's try to get this kind of response at the point of the
request being made :) makes life much easier.

So I think it is easy actually. As I say here (perhaps you missed it?) you
literally already have code you added to the x86 test to prevent test
failure.

So you essentially look for [vdso] or whatever, then you look up to see if
it is sealed, ensure an mremap() fails.

Of course this doesn't check to see if it _should_ be enabled or not. I'm
being nice by not _insisting_ we export a way for you to determine whether
this config option is enabled or not for the sake of a test (since I don't
want to hold up this series). But that'd be nice! Maybe in a
/sys/kernel/security endpoint...

...but I think we can potentially add this later on so we don't hold things
up here/add yet more to the series. I suspect you and Kees alike would
prioritise getting _this series_ in at this point :)

You could, if you wanted to, check to see if /proc/config.gz exists and
zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on
that, but you know kinda hacky.

But anyway, FWIW I think it'd be useful to assert that mremap() or munmap()
fails here for system mappings for the sake of it. I guess this is, in
effect, only checking mseal-ing works right? But it at least asserts the
existence of the thing, and that it behaves.

Later on, when you add some way of observing that it's enabled or not, you
can extend the test to check this.

Thanks!

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 04:06:37PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:24 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 10:52:45PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Provide support to mseal the uprobe mapping.
> > >
> > > Unlike other system mappings, the uprobe mapping is not
> > > established during program startup. However, its lifetime is the same
> > > as the process's lifetime. It could be sealed from creation.
> > >
> >
> > I thought we agreed not to enable this for now? What testing
> > have you done to ensure this is functional?
> >
> I honestly don't know much about uprobe. I don't recall that I agree
> to ignore that though.

OK sorry I realise you have done this from an early version of the series,
my mistake! Apologies.

I'm concerned you don't feel you know much about uprobe, but I guess you
defer to Oleg's views here?

If he's confirmed this is ok, then fine.

>
> As indicated in the cover letter, it is my understanding that uprobe's
> mapping's lifetime are the same as process's lifetime, thus sealable.

> [1]
> Oleg Nesterov, also cc, seems OK with mseal it in the early version of
> this patch [2]
>
> Are there any potential downsides of doing this? If yes, I can remove it.
>
> I'm also looking at Oleg to give more guidance on this :-), or if
> there are some functional tests that I need to do for uprobe.

Yeah, apologies, my mistake I forgot that this was from early, I thought it
was scope creep... but I double-checked and yeah, no haha.

>
>
> [1] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/
> [2] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/
>
> > I mean is this literally _all_ uprobe mappings now being sealed?
> >
> > I'd really like some more assurances on this one. And what are you
> > mitigating by sealing these? I get VDSO (kinda) but uprobes?
> >
> > You really need to provide more justification here.
>
> Sure. In our threat model, we need to seal all r-x, r--, and --x
> mappings to prevent them from becoming writable. This applies to all
> mappings, regardless of whether they're created by the kernel or
> dynamic linker.

All mappings? :P I mean I guess you mean somehow, all 'system' mappings
right?

I guess you mean that somehow some malicious user could manipulate these
mappings from a sandbox or such using a series of exploits that are maybe
more achievable that arbitrary code execution (rop with syscalls or sth? I
am not a security person - obviously! :)

And then un-sandboxed code could innocently touch and bang.

I mean that to me makes sense and cool, we're good. Something like this in
the doc, just a brief sentence like this for idiots (or perhaps you might
say, idiots when it comes to security :) like me would be great, thanks!

>
>
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > ---
> > >  kernel/events/uprobes.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index 2ca797cbe465..8dcdfa0d306b 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -1662,6 +1662,7 @@ static const struct vm_special_mapping xol_mapping = {
> > >  static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> > >  {
> > >       struct vm_area_struct *vma;
> > > +     unsigned long vm_flags;
> > >       int ret;
> > >
> > >       if (mmap_write_lock_killable(mm))
> > > @@ -1682,8 +1683,10 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> > >               }
> > >       }
> > >
> > > +     vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> > > +     vm_flags |= VM_SEALED_SYSMAP;
> > >       vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > > -                             VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > > +                             vm_flags,
> > >                               &xol_mapping);
> > >       if (IS_ERR(vma)) {
> > >               ret = PTR_ERR(vma);
> > > --
> > > 2.48.1.658.g4767266eb4-goog
> > >

Re: [PATCH v7 2/7] selftests: x86: test_mremap_vdso: skip if vdso is msealed

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 02:37:46PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:15 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 10:52:41PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Add code to detect if the vdso is memory sealed, skip the test
> > > if it is.
> >
> > I feel this is a little succinct of a commit message, but I guess it gets to the
> > heart of what you're doing here.
> >
> > Fundamentally I mean it makes sense, but I'm concerned that x86 has a test
> > -expliictly checking- whether mremap() of VDSO is possible - are there cases
> > where x86 might want to do this internal to the kernel?
> >
> > I guess not since this is essentially a userland self test and probably
> > asserting you can do this in the way rr, etc. do.
> >
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > Reviewed-by: Kees Cook <kees@kernel.org>
> >
> > Anyway, this aside, this looks fine, aside from nit below, so:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > > ---
> > >  .../testing/selftests/x86/test_mremap_vdso.c  | 43 +++++++++++++++++++
> > >  1 file changed, 43 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c
> > > index d53959e03593..94bee6e0c813 100644
> > > --- a/tools/testing/selftests/x86/test_mremap_vdso.c
> > > +++ b/tools/testing/selftests/x86/test_mremap_vdso.c
> > > @@ -14,6 +14,7 @@
> > >  #include <errno.h>
> > >  #include <unistd.h>
> > >  #include <string.h>
> > > +#include <stdbool.h>
> > >
> > >  #include <sys/mman.h>
> > >  #include <sys/auxv.h>
> > > @@ -55,13 +56,55 @@ static int try_to_remap(void *vdso_addr, unsigned long size)
> > >
> > >  }
> > >
> > > +#define VDSO_NAME "[vdso]"
> > > +#define VMFLAGS "VmFlags:"
> > > +#define MSEAL_FLAGS "sl"
> > > +#define MAX_LINE_LEN 512
> > > +
> > > +bool vdso_sealed(FILE *maps)
> >
> > Should be static?
> >
> sure.

Thanks! :)

>
> > > +{
> > > +     char line[MAX_LINE_LEN];
> > > +     bool has_vdso = false;
> > > +
> > > +     while (fgets(line, sizeof(line), maps)) {
> > > +             if (strstr(line, VDSO_NAME))
> > > +                     has_vdso = true;
> > > +
> > > +             if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) {
> > > +                     if (strstr(line, MSEAL_FLAGS))
> > > +                             return true;
> > > +
> > > +                     return false;
> > > +             }
> > > +     }
> > > +
> > > +     return false;
> > > +}
> > > +
> > >  int main(int argc, char **argv, char **envp)
> > >  {
> > >       pid_t child;
> > > +     FILE *maps;
> > >
> > >       ksft_print_header();
> > >       ksft_set_plan(1);
> > >
> > > +     maps = fopen("/proc/self/smaps", "r");
> > > +     if (!maps) {
> > > +             ksft_test_result_skip(
> > > +                     "Could not open /proc/self/smaps, errno=%d\n",
> > > +                      errno);
> > > +
> > > +             return 0;
> > > +     }
> > > +
> > > +     if (vdso_sealed(maps)) {
> > > +             ksft_test_result_skip("vdso is sealed\n");
> > > +             return 0;
> > > +     }
> > > +
> > > +     fclose(maps);
> > > +
> > >       child = fork();
> > >       if (child == -1)
> > >               ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);
> > > --
> > > 2.48.1.658.g4767266eb4-goog
> > >

Re: [PATCH v7 0/7] mseal system mappings

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 04:17:01PM -0800, Jeff Xu wrote:
> On Tue, Feb 25, 2025 at 2:32 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > BTW can we please drop the 'mseal, system mappings' prefixes on this
> > series, it's really weird and makes it really hard for me to actually read
> > the individual summary lines for each commit. 'mseal:' will do.
> >
>
> I am not sure.
> I had comments about adding mseal, system mappings, as prefixes, and I
> think it is reasonable.

No it's really horrible (sorry I know it's hyperbolic but it really really looks
horrid to me) , I've never seen prefixes like that before in mm in my life and I
don't think it adds anything.

I also find it MIGHTY confusing.

Please remove this :) you can git log the relevant files to see the conventions
people use. Typically xxx: has something really short and sweet for the 'xxx'.

I realise this is subjective, but it's a small thing and would be helpful for
parsing your series at a glance.

Thanks!

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Lorenzo Stoakes]

On Tue, Feb 25, 2025 at 05:33:24PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:05 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > +     bool
> > > +     help
> > > +       Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> > > +
> > > +       A 64-bit kernel is required for the memory sealing feature.
> > > +       No specific hardware features from the CPU are needed.
> > > +
> > > +       To enable this feature, the architecture needs to update their
> > > +       special mappings calls to include the sealing flag and confirm
> > > +       that it doesn't unmap/remap system mappings during the life
> > > +       time of the process. After the architecture enables this, a
> > > +       distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> > > +       to the feature.
> >
> > Architectures also need to be confirmed not to require any form of VDSO
> > relocation, which as discussed in previous series some arches appear to
> > need to do. I'd mention that here.
> >
> This might need clarification, the system mapping includes vdso, right
> ? Why the focus on vdso ?

My mistake, I thought scope was more limited than this when I first
looked. Please disregard the focus on VDSO here... :)

>
> The sentence  "... it doesn't unmap/remap system mappings during the
> lifetime of the process."  already cover what you want here, I think.
>

Right, I guess it just doesn't quite _emphasise_ it enough for me. Something
like the below would really help bring that out:

	The existing of this flag for an architecture implies that it does not
	require the remapping of these system mappings during process lifetime,
	so sealing these mappings is safe from a kernel perspective.

>
> > > +
> > > +       For complete descriptions of memory sealing, please see
> > > +       Documentation/userspace-api/mseal.rst
> > > +
> > >  config HAVE_PERF_EVENTS
> > >       bool
> > >       help
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index f10dbf15c294..15a86a952910 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -51,6 +51,24 @@ config PROC_MEM_NO_FORCE
> > >
> > >  endchoice
> > >
> > > +config MSEAL_SYSTEM_MAPPINGS
> > > +     bool "mseal system mappings"
> > > +     depends on 64BIT
> > > +     depends on ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > +     depends on !CHECKPOINT_RESTORE
> > > +     help
> > > +       Seal system mappings such as vdso, vvar, sigpage, uprobes, etc.
> >
> > Let's be specific here, 'etc.' could mean _anything_. Also you aren't
> > sealing most of this, let's just list what you are _actually_ sealing
> > here. Which is AFAIK VDSO only?
> >
> I will remove "etc" and list all the mappings.
>
> Those mappings are:
> vdso, vvar, vvar_vclock, vectors (arm compact-mode) and sigpage (arm
> compact-mode), uprobe.
>
> We seal all system mappings that x86-64 and arm64 have.

Perfect!

>
> > You can update this later as time goes on if/when you expand this.
> >
> > > +
> > > +       A 64-bit kernel is required for the memory sealing feature.
> > > +       No specific hardware features from the CPU are needed.
> > > +
> > > +       Note: CHECKPOINT_RESTORE, UML, gVisor, rr are known to relocate or
> > > +       unmap system mapping, therefore this config can't be enabled
> > > +       universally.
> >
> > Thanks for putting this here, appreciate it!
> >
> > Could we tweak this though? I'd like to make it crystal clear, so I don't
> > think 'note' sufficies and this sounds a little too vague.
> >
> > I think 'warning' is more appropriate here since you're breaking things for
> > people who might be unaware. And we need to say this -breaks- programs:
> >
> > WARNING: This feature breaks programs which rely on relocating or
> >          unmapping system mappings.
> >
> >          Known broken software at the time of writing includes
> >          CHECKPOINT_RESTORE, UML, gVisor and rr.
> >
> > I think this is critical.
> >
> Sure.

Perfect, thank you! Much appreciated.

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Thomas Weißschuh]

On Tue, Feb 25, 2025 at 04:48:47PM -0800, Jeff Xu wrote:
> On Tue, Feb 25, 2025 at 12:08 AM Thomas Weißschuh
> <thomas.weissschuh@linutronix.de> wrote:
> > On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> > > covering the vdso, vvar, vvar_vclock.
> > >
> > > Production release testing passes on Android and Chrome OS.
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > ---
> > >  arch/x86/Kconfig          |  1 +
> > >  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
> > >  2 files changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 87198d957e2f..8fa17032ca46 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -26,6 +26,7 @@ config X86_64
> > >       depends on 64BIT
> > >       # Options that are inherently 64-bit kernel only:
> > >       select ARCH_HAS_GIGANTIC_PAGE
> > > +     select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > >       select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> > >       select ARCH_SUPPORTS_PER_VMA_LOCK
> > >       select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > > diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> > > index 39e6efc1a9ca..1b1c009f20a8 100644
> > > --- a/arch/x86/entry/vdso/vma.c
> > > +++ b/arch/x86/entry/vdso/vma.c
> > > @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > >       struct mm_struct *mm = current->mm;
> > >       struct vm_area_struct *vma;
> > >       unsigned long text_start;
> > > +     unsigned long vm_flags;
> > >       int ret = 0;
> > >
> > >       if (mmap_write_lock_killable(mm))
> > > @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > >       /*
> > >        * MAYWRITE to allow gdb to COW and set breakpoints
> > >        */
> > > +     vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > > +     vm_flags |= VM_SEALED_SYSMAP;
> > >       vma = _install_special_mapping(mm,
> > >                                      text_start,
> > >                                      image->size,
> > > -                                    VM_READ|VM_EXEC|
> > > -                                    VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > > +                                    vm_flags,
> > >                                      &vdso_mapping);
> > >
> > >       if (IS_ERR(vma)) {
> > > @@ -276,11 +278,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > >               goto up_fail;
> > >       }
> > >
> > > +     vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
> > > +     vm_flags |= VM_SEALED_SYSMAP;
> > >       vma = _install_special_mapping(mm,
> > >                                      addr,
> > >                                      (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
> > > -                                    VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
> > > -                                    VM_PFNMAP,
> > > +                                    vm_flags,
> > >                                      &vvar_mapping);
> >
> > This hunk (and the vvar mapping in the arm64 patch) will conflict with my
> > "Generic vDSO datapage" series.
> > That series is already part of the tip tree (branch timers/vdso) and scheduled
> > for the next merge window.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/vdso
> >
> > The conflict resolution is fairly easy:
> > Move the new flag logic into lib/vdso/datastore.c
> >
> Thank you for bringing this to my attention.
> 
> In your change,  it seems lib/vdso/datastore.c implements a
> vdso_install_vvar_mapping(), then all the architectures call this
> function.

Correct.

At least all the architectures using the generic vDSO infrastructure,
which are the ones you care about.
Sparc for example has its own implementation.

> So merging conflict won't be as straightforward.

Wouldn't it be enough to unconditionally use VM_SEALED_SYSMAP in
vdso_install_vvar_mapping()?
The symbol is a noop on architectures or configurations where the new
functionality is not available or enabled.

> Maybe a better
> approach is that I continue resolving all the comments, based on the
> latest main. Then wait for your change to be merged and submit another
> version.

That would work, too. As you prefer.

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Oleg Nesterov]

On 02/24, jeffxu@chromium.org wrote:
>
> Unlike other system mappings, the uprobe mapping is not
> established during program startup. However, its lifetime is the same
> as the process's lifetime. It could be sealed from creation.

Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
of config options.

ACK,

but can't we do

	#ifdef CONFIG_64BIT
	/* VM is sealed, in vm_flags */
	#define VM_SEALED	_BITUL(63)
+	#else
+	#define VM_SEALED	0
	#endif

and then simply

	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
-				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
+				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,

?

But I am fine either way, feel free to ignore.

Oleg.

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Oleg Nesterov]

On 02/26, Oleg Nesterov wrote:
>
> On 02/24, jeffxu@chromium.org wrote:
> >
> > Unlike other system mappings, the uprobe mapping is not
> > established during program startup. However, its lifetime is the same
> > as the process's lifetime. It could be sealed from creation.
>
> Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
> of config options.
>
> ACK,
>
> but can't we do
>
> 	#ifdef CONFIG_64BIT
> 	/* VM is sealed, in vm_flags */
> 	#define VM_SEALED	_BITUL(63)
> +	#else
> +	#define VM_SEALED	0
> 	#endif
>
> and then simply
>
> 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> +				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
>
> ?
>
> But I am fine either way, feel free to ignore.

Yes, but either way, why your patch adds "unsigned long vm_flags" ?
OK, perhaps it makes sense for readability, but

	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
	vm_flags |= VM_SEALED_SYSMAP;

looks a bit strange, why not

	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;

?

Oleg.

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote:
> On 02/24, jeffxu@chromium.org wrote:
> >
> > Unlike other system mappings, the uprobe mapping is not
> > established during program startup. However, its lifetime is the same
> > as the process's lifetime. It could be sealed from creation.
>
> Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
> of config options.

If you think this ought to be the case generally, then perhaps we should
drop this patch from the commit and just do this separately as a
permanent-on thing, if you are sure this is fine + want it?

An aside - we _definitely_ cannot allow this -system mapping stuff- to be
enabled without a config option, this is emphatic, for the reason that it
breaks userspace and is only known-good on some arches.

A config flag that checks arch gives a big warning saying 'hey this breaks
userspace' means users use it knowing this to be the case.
>
>
> ACK,
>
> but can't we do
>
> 	#ifdef CONFIG_64BIT
> 	/* VM is sealed, in vm_flags */
> 	#define VM_SEALED	_BITUL(63)
> +	#else
> +	#define VM_SEALED	0
> 	#endif

This has been raised a few times. Jeff objects to this (for reasons I don't
agree with, honestly) but it's been implemented in this way from the start
(in order to catch the case of 32-bit systems trying to use mseal but it
not happening).

Anyway I agree, but I'm not going to push this at least in this series.

>
> and then simply
>
> 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> +				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
>
> ?

Nah you'd have to do:


> 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO
#ifdef CONFIG_64BIT
				VM_SEALED
#endif
				,

Or something similar :)

Beautiful no?

>
> But I am fine either way, feel free to ignore.
>
> Oleg.
>

Ideally We should hear what the security use case is here. I mean I'm less
bothered with it behind a flag, but it seems odd that an attacker would
break out of a sandbox into a process currently being debugged... seems
unlikely.

I'm not sure under what other circumstances this would be a problem. Jeff,
Kees?

Anyway as I said before, I don't overly object to this as-is, as long as
you are ok with it Oleg and can absolutely confirm this will never break
anything, you don't need to remap, unmap (until process teardown), adjust
the VMA in any way etc.?

A quick glance at uprobe code suggests it's fine.

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > From: Jeff Xu <jeffxu@chromium.org>
> > > >
> > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > >
> > > > Production release testing passes on Android and Chrome OS.
> > >
> > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > account for all the weird and wonderful arm64 devices out there in context of
> > > upstream :)
> > >
> > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > nervous of that.
> > >
> > Can you please point out where PPC munmap/mremap the vdso ?
> >
> > Previously, when you mentioned that, I thought you meant user space in
> > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > tried, but didn't find anything, hence asking.
> 
> Jeff, please stick to replying to review. 'Have you looking through all
> arm64-code'.
> 
> I ended up doing this myself yesterday and found no issues, as with x86-64.
> 
> I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> it. We can discuss it, and I can find specifics if + when you try to add
> this to PPC.
> 

PPC allows the vma to be munmapped then detects and falls back to the
slower method, iirc.

They were against the removal of the fallback; other archs also have
this infrastructure.  Really, if we fixed the fallback to work for
all platforms then it would probably also remove the possibility of a
remap over the VDSO being a problem (if it is today, which still isn't
clear?).

Thanks,
Liam

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Jeff Xu]

On Wed, Feb 26, 2025 at 9:12 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> > On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > >
> > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > > >
> > > > > Production release testing passes on Android and Chrome OS.
> > > >
> > > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > > account for all the weird and wonderful arm64 devices out there in context of
> > > > upstream :)
> > > >
> > > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > > nervous of that.
> > > >
> > > Can you please point out where PPC munmap/mremap the vdso ?
> > >
> > > Previously, when you mentioned that, I thought you meant user space in
> > > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > > tried, but didn't find anything, hence asking.
> >
> > Jeff, please stick to replying to review. 'Have you looking through all
> > arm64-code'.
> >
> > I ended up doing this myself yesterday and found no issues, as with x86-64.
> >
> > I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> > it. We can discuss it, and I can find specifics if + when you try to add
> > this to PPC.
> >
>
> PPC allows the vma to be munmapped then detects and falls back to the
> slower method, iirc.
>
Is this code in the kernel or userspace?

If PPC doesn't want to create vdso for all its userspace apps, we
could instead "don't create" vdso during the execve call.


> They were against the removal of the fallback; other archs also have
> this infrastructure.  Really, if we fixed the fallback to work for
> all platforms then it would probably also remove the possibility of a
> remap over the VDSO being a problem (if it is today, which still isn't
> clear?).
>
Any past thread/communication about this that I can read ?

Thanks
-Jeff


> Thanks,
> Liam

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Lorenzo Stoakes]

On Wed, Feb 26, 2025 at 09:17:10AM -0800, Jeff Xu wrote:
> On Wed, Feb 26, 2025 at 9:12 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> > > On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > > > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > >
> > > > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > > >
> > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > > > >
> > > > > > Production release testing passes on Android and Chrome OS.
> > > > >
> > > > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > > > account for all the weird and wonderful arm64 devices out there in context of
> > > > > upstream :)
> > > > >
> > > > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > > > nervous of that.
> > > > >
> > > > Can you please point out where PPC munmap/mremap the vdso ?
> > > >
> > > > Previously, when you mentioned that, I thought you meant user space in
> > > > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > > > tried, but didn't find anything, hence asking.
> > >
> > > Jeff, please stick to replying to review. 'Have you looking through all
> > > arm64-code'.
> > >
> > > I ended up doing this myself yesterday and found no issues, as with x86-64.
> > >
> > > I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> > > it. We can discuss it, and I can find specifics if + when you try to add
> > > this to PPC.
> > >
> >
> > PPC allows the vma to be munmapped then detects and falls back to the
> > slower method, iirc.
> >
> Is this code in the kernel or userspace?
>
> If PPC doesn't want to create vdso for all its userspace apps, we
> could instead "don't create" vdso during the execve call.
>
>
> > They were against the removal of the fallback; other archs also have
> > this infrastructure.  Really, if we fixed the fallback to work for
> > all platforms then it would probably also remove the possibility of a
> > remap over the VDSO being a problem (if it is today, which still isn't
> > clear?).
> >
> Any past thread/communication about this that I can read ?

Jeff, I'm sure you don't intend to, but I find it quite disrespectful that you
ignored my feedback here (and elsewhere, regarding you ignoring 4 sets of
feedback).

This?

https://elixir.bootlin.com/linux/v6.13.4/source/arch/powerpc/kernel/vdso.c#L236

Was [0] a relevant discussion?

[0]: https://lore.kernel.org/all/lhe2mky6ahlk2jzvvfjyongqiseelyx2uy7sbyuso6jcy3b2dq@7ju6cea62jgk/

>
> Thanks
> -Jeff
>
>
> > Thanks,
> > Liam

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Oleg Nesterov]

On 02/26, Lorenzo Stoakes wrote:
>
> On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote:
> > On 02/24, jeffxu@chromium.org wrote:
> > >
> > > Unlike other system mappings, the uprobe mapping is not
> > > established during program startup. However, its lifetime is the same
> > > as the process's lifetime. It could be sealed from creation.
> >
> > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
> > of config options.
>
> If you think this ought to be the case generally, then perhaps we should
> drop this patch from the commit and just do this separately as a
> permanent-on thing, if you are sure this is fine + want it?

See below...

> An aside - we _definitely_ cannot allow this -system mapping stuff- to be
> enabled without a config option,

This is clear.

But as for uprobes in particular I do think that VM_SEALED is always fine.

Do we really want it? I dunno. If a task unmaps its "[uprobes]" vma it
will crash when it hits the uprobes bp next time. Unless the probed insn
can be emulated and it is not ret-probe. Do we really care? Again, I don't
know.

Should this change come as a separate patch? I don't understand why it should
but I am fine either way.

In short. please do what you think is right,  VM_SEALED can't hurt uprobes ;)

> > 	#ifdef CONFIG_64BIT
> > 	/* VM is sealed, in vm_flags */
> > 	#define VM_SEALED	_BITUL(63)
> > +	#else
> > +	#define VM_SEALED	0
> > 	#endif
>
> This has been raised a few times. Jeff objects to this

OK,

> > and then simply
> >
> > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > +				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
> >
> > ?
>
> Nah you'd have to do:
>
>
> > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> 				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO
> #ifdef CONFIG_64BIT
> 				VM_SEALED
> #endif
> 				,

Why??? With the proposed change above VM_SEALED == 0 if !CONFIG_64BIT.

Oleg.

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Wed, Feb 26, 2025 at 07:01:36PM +0100, Oleg Nesterov wrote:
> On 02/26, Lorenzo Stoakes wrote:
> >
> > On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote:
> > > On 02/24, jeffxu@chromium.org wrote:
> > > >
> > > > Unlike other system mappings, the uprobe mapping is not
> > > > established during program startup. However, its lifetime is the same
> > > > as the process's lifetime. It could be sealed from creation.
> > >
> > > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
> > > of config options.
> >
> > If you think this ought to be the case generally, then perhaps we should
> > drop this patch from the commit and just do this separately as a
> > permanent-on thing, if you are sure this is fine + want it?
>
> See below...
>
> > An aside - we _definitely_ cannot allow this -system mapping stuff- to be
> > enabled without a config option,
>
> This is clear.
>
> But as for uprobes in particular I do think that VM_SEALED is always fine.
>
> Do we really want it? I dunno. If a task unmaps its "[uprobes]" vma it
> will crash when it hits the uprobes bp next time. Unless the probed insn
> can be emulated and it is not ret-probe. Do we really care? Again, I don't
> know.
>
> Should this change come as a separate patch? I don't understand why it should
> but I am fine either way.
>
> In short. please do what you think is right,  VM_SEALED can't hurt uprobes ;)
>
> > > 	#ifdef CONFIG_64BIT
> > > 	/* VM is sealed, in vm_flags */
> > > 	#define VM_SEALED	_BITUL(63)
> > > +	#else
> > > +	#define VM_SEALED	0
> > > 	#endif
> >
> > This has been raised a few times. Jeff objects to this
>
> OK,
>
> > > and then simply
> > >
> > > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > > -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > > +				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
> > >
> > > ?
> >
> > Nah you'd have to do:
> >
> >
> > > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > 				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO
> > #ifdef CONFIG_64BIT
> > 				VM_SEALED
> > #endif
> > 				,
>
> Why??? With the proposed change above VM_SEALED == 0 if !CONFIG_64BIT.
>

Like I said, Jeff opposes the change. I disagree with him, and agree with you,
because this is very silly.

But I don't want to hold up this series with that discussion (this is for his
sake...)

> Oleg.
>

Jeff - perhaps drop this and let's return to it in a follow up so this series
isn't held up?

Thanks.

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Lorenzo Stoakes]

On Wed, Feb 26, 2025 at 05:43:22PM +0000, Lorenzo Stoakes wrote:
> On Wed, Feb 26, 2025 at 09:17:10AM -0800, Jeff Xu wrote:
> > On Wed, Feb 26, 2025 at 9:12 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > >
> > > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> > > > On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > > > > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > > > >
> > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > > > > >
> > > > > > > Production release testing passes on Android and Chrome OS.
> > > > > >
> > > > > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > > > > account for all the weird and wonderful arm64 devices out there in context of
> > > > > > upstream :)
> > > > > >
> > > > > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > > > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > > > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > > > > nervous of that.
> > > > > >
> > > > > Can you please point out where PPC munmap/mremap the vdso ?
> > > > >
> > > > > Previously, when you mentioned that, I thought you meant user space in
> > > > > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > > > > tried, but didn't find anything, hence asking.
> > > >
> > > > Jeff, please stick to replying to review. 'Have you looking through all
> > > > arm64-code'.
> > > >
> > > > I ended up doing this myself yesterday and found no issues, as with x86-64.
> > > >
> > > > I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> > > > it. We can discuss it, and I can find specifics if + when you try to add
> > > > this to PPC.
> > > >
> > >
> > > PPC allows the vma to be munmapped then detects and falls back to the
> > > slower method, iirc.
> > >
> > Is this code in the kernel or userspace?
> >
> > If PPC doesn't want to create vdso for all its userspace apps, we
> > could instead "don't create" vdso during the execve call.
> >
> >
> > > They were against the removal of the fallback; other archs also have
> > > this infrastructure.  Really, if we fixed the fallback to work for
> > > all platforms then it would probably also remove the possibility of a
> > > remap over the VDSO being a problem (if it is today, which still isn't
> > > clear?).
> > >
> > Any past thread/communication about this that I can read ?
>
> Jeff, I'm sure you don't intend to, but I find it quite disrespectful that you
> ignored my feedback here (and elsewhere, regarding you ignoring 4 sets of
> feedback).

Apologies, I meant to reword this to sound less harsh but somebody phoned
me and I hit send...

What I mean to say is I think you _do_ ack what I've said here, but you
think it's not useful to reply because there's not really a converastion to
be had.

Whereas I'm saying it'd be useful to ack :)

Sorry I did not mean for this to sound quite so 'full on'.

>
> This?
>
> https://elixir.bootlin.com/linux/v6.13.4/source/arch/powerpc/kernel/vdso.c#L236
>
> Was [0] a relevant discussion?
>
> [0]: https://lore.kernel.org/all/lhe2mky6ahlk2jzvvfjyongqiseelyx2uy7sbyuso6jcy3b2dq@7ju6cea62jgk/

I did in the end go and check this, so hopefully this is useful at least.

But again, I really think we should hold off on PPC stuff until we come to
it and focus on getting this series into mergeable state.

Am doing my best to try to get you there ASAP as there's been a lot of
delays here.

>
> >
> > Thanks
> > -Jeff
> >
> >
> > > Thanks,
> > > Liam

Cheers, Lorenzo

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 13:06]:
> On Wed, Feb 26, 2025 at 07:01:36PM +0100, Oleg Nesterov wrote:
> > On 02/26, Lorenzo Stoakes wrote:
> > >
> > > On Wed, Feb 26, 2025 at 05:26:04PM +0100, Oleg Nesterov wrote:
> > > > On 02/24, jeffxu@chromium.org wrote:
> > > > >
> > > > > Unlike other system mappings, the uprobe mapping is not
> > > > > established during program startup. However, its lifetime is the same
> > > > > as the process's lifetime. It could be sealed from creation.
> > > >
> > > > Agreed, VM_SEALED should be always for the "[uprobes]" vma, regardless
> > > > of config options.
> > >
> > > If you think this ought to be the case generally, then perhaps we should
> > > drop this patch from the commit and just do this separately as a
> > > permanent-on thing, if you are sure this is fine + want it?
> >
> > See below...
> >
> > > An aside - we _definitely_ cannot allow this -system mapping stuff- to be
> > > enabled without a config option,
> >
> > This is clear.
> >
> > But as for uprobes in particular I do think that VM_SEALED is always fine.
> >
> > Do we really want it? I dunno. If a task unmaps its "[uprobes]" vma it
> > will crash when it hits the uprobes bp next time. Unless the probed insn
> > can be emulated and it is not ret-probe. Do we really care? Again, I don't
> > know.
> >
> > Should this change come as a separate patch? I don't understand why it should
> > but I am fine either way.
> >
> > In short. please do what you think is right,  VM_SEALED can't hurt uprobes ;)
> >
> > > > 	#ifdef CONFIG_64BIT
> > > > 	/* VM is sealed, in vm_flags */
> > > > 	#define VM_SEALED	_BITUL(63)
> > > > +	#else
> > > > +	#define VM_SEALED	0

nit, we have VM_NONE for this (it's also 0, so no real difference)

> > > > 	#endif
> > >
> > > This has been raised a few times. Jeff objects to this
> >
> > OK,
> >
> > > > and then simply
> > > >
> > > > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > > > -				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > > > +				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED,
> > > >
> > > > ?
> > >
> > > Nah you'd have to do:
> > >
> > >
> > > > 	vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > > 				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO
> > > #ifdef CONFIG_64BIT
> > > 				VM_SEALED
> > > #endif
> > > 				,
> >
> > Why??? With the proposed change above VM_SEALED == 0 if !CONFIG_64BIT.
> >
> 
> Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> because this is very silly.

Discussion here [1].

> 
> But I don't want to hold up this series with that discussion (this is for his
> sake...)
> 
> > Oleg.
> >
> 
> Jeff - perhaps drop this and let's return to it in a follow up so this series
> isn't held up?
> 

...

Thanks,
Liam

[1]. https://lore.kernel.org/all/CABi2SkVKhjShryG0K-NSjjBvGs0UOVsY-6MQVOuQCkfuph5K8Q@mail.gmail.com/

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Oleg Nesterov]

On 02/26, Lorenzo Stoakes wrote:
>
> Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> because this is very silly.
>
> But I don't want to hold up this series with that discussion (this is for his
> sake...)

Neither me, so lets go with VM_SEALED_SYSMAP.

My only objection is that

	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
	vm_flags |= VM_SEALED_SYSMAP;

looks unnecessarily confusing to me,

	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;

or just

	vma = _install_special_mapping(...,
				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP,
				...

looks more readable. But this is cosmetic/subjective, so I won't argue/insist.

> Jeff - perhaps drop this and let's return to it in a follow up so this series
> isn't held up?

Up to you and Jeff.

But this patch looks "natural" to me in this series.

Oleg.

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Wed, Feb 26, 2025 at 07:20:50PM +0100, Oleg Nesterov wrote:
> On 02/26, Lorenzo Stoakes wrote:
> >
> > Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> > because this is very silly.
> >
> > But I don't want to hold up this series with that discussion (this is for his
> > sake...)
>
> Neither me, so lets go with VM_SEALED_SYSMAP.
>
> My only objection is that
>
> 	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> 	vm_flags |= VM_SEALED_SYSMAP;
>
> looks unnecessarily confusing to me,
>
> 	vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;
>
> or just
>
> 	vma = _install_special_mapping(...,
> 				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP,
> 				...
>
> looks more readable. But this is cosmetic/subjective, so I won't argue/insist.

Agreed. This would be good.

>
> > Jeff - perhaps drop this and let's return to it in a follow up so this series
> > isn't held up?
>
> Up to you and Jeff.
>
> But this patch looks "natural" to me in this series.

OK, I mean in that case I'm ok with it as-is, as you confirms there's no
issue, I've looked at the code and there's no issue.

It was only if we wanted to try the VM_SEALED thing, i.e. _always_ seal
then it'd do better outside of the series as there'd be a discussion about
maybe changing this CONFIG_64BIT thing yada yada.

>
> Oleg.
>

Jeff - in that case, do NOT drop this one :P but do please look at the
above style nit.

Let's keep things moving... :)

Re: [PATCH v7 3/7] mseal, system mappings: enable x86-64

[Jeff Xu]

Hi Thomas

On Tue, Feb 25, 2025 at 11:35 PM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> On Tue, Feb 25, 2025 at 04:48:47PM -0800, Jeff Xu wrote:
> > On Tue, Feb 25, 2025 at 12:08 AM Thomas Weißschuh
> > <thomas.weissschuh@linutronix.de> wrote:
> > > On Mon, Feb 24, 2025 at 10:52:42PM +0000, jeffxu@chromium.org wrote:
> > > > From: Jeff Xu <jeffxu@chromium.org>
> > > >
> > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on x86-64,
> > > > covering the vdso, vvar, vvar_vclock.
> > > >
> > > > Production release testing passes on Android and Chrome OS.
> > > >
> > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > > ---
> > > >  arch/x86/Kconfig          |  1 +
> > > >  arch/x86/entry/vdso/vma.c | 16 ++++++++++------
> > > >  2 files changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > > index 87198d957e2f..8fa17032ca46 100644
> > > > --- a/arch/x86/Kconfig
> > > > +++ b/arch/x86/Kconfig
> > > > @@ -26,6 +26,7 @@ config X86_64
> > > >       depends on 64BIT
> > > >       # Options that are inherently 64-bit kernel only:
> > > >       select ARCH_HAS_GIGANTIC_PAGE
> > > > +     select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > >       select ARCH_SUPPORTS_INT128 if CC_HAS_INT128
> > > >       select ARCH_SUPPORTS_PER_VMA_LOCK
> > > >       select ARCH_SUPPORTS_HUGE_PFNMAP if TRANSPARENT_HUGEPAGE
> > > > diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
> > > > index 39e6efc1a9ca..1b1c009f20a8 100644
> > > > --- a/arch/x86/entry/vdso/vma.c
> > > > +++ b/arch/x86/entry/vdso/vma.c
> > > > @@ -247,6 +247,7 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > > >       struct mm_struct *mm = current->mm;
> > > >       struct vm_area_struct *vma;
> > > >       unsigned long text_start;
> > > > +     unsigned long vm_flags;
> > > >       int ret = 0;
> > > >
> > > >       if (mmap_write_lock_killable(mm))
> > > > @@ -264,11 +265,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > > >       /*
> > > >        * MAYWRITE to allow gdb to COW and set breakpoints
> > > >        */
> > > > +     vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
> > > > +     vm_flags |= VM_SEALED_SYSMAP;
> > > >       vma = _install_special_mapping(mm,
> > > >                                      text_start,
> > > >                                      image->size,
> > > > -                                    VM_READ|VM_EXEC|
> > > > -                                    VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
> > > > +                                    vm_flags,
> > > >                                      &vdso_mapping);
> > > >
> > > >       if (IS_ERR(vma)) {
> > > > @@ -276,11 +278,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
> > > >               goto up_fail;
> > > >       }
> > > >
> > > > +     vm_flags = VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|VM_PFNMAP;
> > > > +     vm_flags |= VM_SEALED_SYSMAP;
> > > >       vma = _install_special_mapping(mm,
> > > >                                      addr,
> > > >                                      (__VVAR_PAGES - VDSO_NR_VCLOCK_PAGES) * PAGE_SIZE,
> > > > -                                    VM_READ|VM_MAYREAD|VM_IO|VM_DONTDUMP|
> > > > -                                    VM_PFNMAP,
> > > > +                                    vm_flags,
> > > >                                      &vvar_mapping);
> > >
> > > This hunk (and the vvar mapping in the arm64 patch) will conflict with my
> > > "Generic vDSO datapage" series.
> > > That series is already part of the tip tree (branch timers/vdso) and scheduled
> > > for the next merge window.
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=timers/vdso
> > >
> > > The conflict resolution is fairly easy:
> > > Move the new flag logic into lib/vdso/datastore.c
> > >
> > Thank you for bringing this to my attention.
> >
> > In your change,  it seems lib/vdso/datastore.c implements a
> > vdso_install_vvar_mapping(), then all the architectures call this
> > function.
>
> Correct.
>
> At least all the architectures using the generic vDSO infrastructure,
> which are the ones you care about.
> Sparc for example has its own implementation.
>
> > So merging conflict won't be as straightforward.
>
> Wouldn't it be enough to unconditionally use VM_SEALED_SYSMAP in
> vdso_install_vvar_mapping()?
> The symbol is a noop on architectures or configurations where the new
> functionality is not available or enabled.
>
Yes. That will work.

> > Maybe a better
> > approach is that I continue resolving all the comments, based on the
> > latest main. Then wait for your change to be merged and submit another
> > version.
>
> That would work, too. As you prefer.
Great !

Thanks
-Jeff

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Jeff Xu]

Hi Oleg

On Wed, Feb 26, 2025 at 10:21 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 02/26, Lorenzo Stoakes wrote:
> >
> > Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> > because this is very silly.
> >
> > But I don't want to hold up this series with that discussion (this is for his
> > sake...)
>
> Neither me, so lets go with VM_SEALED_SYSMAP.
>
> My only objection is that
>
>         vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
>         vm_flags |= VM_SEALED_SYSMAP;
>
> looks unnecessarily confusing to me,
>
>         vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;
>
> or just
>
>         vma = _install_special_mapping(...,
>                                 VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP,
>
Sure
                                 ...
>
> looks more readable. But this is cosmetic/subjective, so I won't argue/insist.
>
> > Jeff - perhaps drop this and let's return to it in a follow up so this series
> > isn't held up?
>
> Up to you and Jeff.
>
> But this patch looks "natural" to me in this series.
>
Thanks for sharing your insights on uprobe! Your expertise is invaluable.

-Jeff

> Oleg.
>

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Jeff Xu]

On Wed, Feb 26, 2025 at 10:25 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Feb 26, 2025 at 07:20:50PM +0100, Oleg Nesterov wrote:
> > On 02/26, Lorenzo Stoakes wrote:
> > >
> > > Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> > > because this is very silly.
> > >
> > > But I don't want to hold up this series with that discussion (this is for his
> > > sake...)
> >
> > Neither me, so lets go with VM_SEALED_SYSMAP.
> >
> > My only objection is that
> >
> >       vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> >       vm_flags |= VM_SEALED_SYSMAP;
> >
> > looks unnecessarily confusing to me,
> >
> >       vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;
> >
> > or just
> >
> >       vma = _install_special_mapping(...,
> >                               VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP,
> >                               ...
> >
> > looks more readable. But this is cosmetic/subjective, so I won't argue/insist.
>
> Agreed. This would be good.
>
> >
> > > Jeff - perhaps drop this and let's return to it in a follow up so this series
> > > isn't held up?
> >
> > Up to you and Jeff.
> >
> > But this patch looks "natural" to me in this series.
>
> OK, I mean in that case I'm ok with it as-is, as you confirms there's no
> issue, I've looked at the code and there's no issue.
>
> It was only if we wanted to try the VM_SEALED thing, i.e. _always_ seal
> then it'd do better outside of the series as there'd be a discussion about
> maybe changing this CONFIG_64BIT thing yada yada.
>
> >
> > Oleg.
> >
>
> Jeff - in that case, do NOT drop this one :P but do please look at the
> above style nit.
>
Ok.


> Let's keep things moving... :)

Re: [PATCH v7 0/7] mseal system mappings

[Jeff Xu]

On Tue, Feb 25, 2025 at 10:01 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Feb 25, 2025 at 04:17:01PM -0800, Jeff Xu wrote:
> > On Tue, Feb 25, 2025 at 2:32 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > BTW can we please drop the 'mseal, system mappings' prefixes on this
> > > series, it's really weird and makes it really hard for me to actually read
> > > the individual summary lines for each commit. 'mseal:' will do.
> > >
> >
> > I am not sure.
> > I had comments about adding mseal, system mappings, as prefixes, and I
> > think it is reasonable.
>
> No it's really horrible (sorry I know it's hyperbolic but it really really looks
> horrid to me) , I've never seen prefixes like that before in mm in my life and I
> don't think it adds anything.
>
> I also find it MIGHTY confusing.
>
> Please remove this :) you can git log the relevant files to see the conventions
> people use. Typically xxx: has something really short and sweet for the 'xxx'.
>
> I realise this is subjective, but it's a small thing and would be helpful for
> parsing your series at a glance.
>
I need a prefix to group the patches, especially when the first patch
is just a config change. "mseal" won't work because this patch isn't
about core business logic of mseal.

How about "mseal sysmap:"?

-Jeff

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Jeff Xu]

On Tue, Feb 25, 2025 at 10:04 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Feb 25, 2025 at 05:33:24PM -0800, Jeff Xu wrote:
> > On Mon, Feb 24, 2025 at 10:05 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > > +     bool
> > > > +     help
> > > > +       Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> > > > +
> > > > +       A 64-bit kernel is required for the memory sealing feature.
> > > > +       No specific hardware features from the CPU are needed.
> > > > +
> > > > +       To enable this feature, the architecture needs to update their
> > > > +       special mappings calls to include the sealing flag and confirm
> > > > +       that it doesn't unmap/remap system mappings during the life
> > > > +       time of the process. After the architecture enables this, a
> > > > +       distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> > > > +       to the feature.
> > >
> > > Architectures also need to be confirmed not to require any form of VDSO
> > > relocation, which as discussed in previous series some arches appear to
> > > need to do. I'd mention that here.
> > >
> > This might need clarification, the system mapping includes vdso, right
> > ? Why the focus on vdso ?
>
> My mistake, I thought scope was more limited than this when I first
> looked. Please disregard the focus on VDSO here... :)
>
> >
> > The sentence  "... it doesn't unmap/remap system mappings during the
> > lifetime of the process."  already cover what you want here, I think.
> >
>
> Right, I guess it just doesn't quite _emphasise_ it enough for me. Something
> like the below would really help bring that out:
>
>         The existing of this flag for an architecture implies that it does not
>         require the remapping of these system mappings during process lifetime,
>         so sealing these mappings is safe from a kernel perspective.
>
I'm not sure I get the difference, but I can add it,  is below OK ?

To enable this feature, the architecture needs to update their
special mappings calls to include the sealing flag and confirm
that it doesn't unmap/remap system mappings during the life
time of the process. The existence of this flag for an architecture
implies that it does not require the remapping of these system
mappings during process lifetime, so sealing these mappings is
safe from a kernel perspective. After the architecture enables this,
a distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to
manage access to the feature.

Thanks
-Jeff

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Jeff Xu]

On Wed, Feb 26, 2025 at 9:43 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Feb 26, 2025 at 09:17:10AM -0800, Jeff Xu wrote:
> > On Wed, Feb 26, 2025 at 9:12 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > >
> > > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> > > > On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > > > > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > > > >
> > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > > > > >
> > > > > > > Production release testing passes on Android and Chrome OS.
> > > > > >
> > > > > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > > > > account for all the weird and wonderful arm64 devices out there in context of
> > > > > > upstream :)
> > > > > >
> > > > > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > > > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > > > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > > > > nervous of that.
> > > > > >
> > > > > Can you please point out where PPC munmap/mremap the vdso ?
> > > > >
> > > > > Previously, when you mentioned that, I thought you meant user space in
> > > > > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > > > > tried, but didn't find anything, hence asking.
> > > >
> > > > Jeff, please stick to replying to review. 'Have you looking through all
> > > > arm64-code'.
> > > >
I checked the kernel code and couldn't find any instances of kernel
unmap/remap system mapping in any architecture. But I could be wrong,
so I've also included developers from different architectures since
V1, and hoping to get some insight.

> > > > I ended up doing this myself yesterday and found no issues, as with x86-64.
> > > >
Thank you for double checking.

> > > > I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> > > > it. We can discuss it, and I can find specifics if + when you try to add
> > > > this to PPC.
> > > >
> > >
> > > PPC allows the vma to be munmapped then detects and falls back to the
> > > slower method, iirc.
> > >
> > Is this code in the kernel or userspace?
> >
> > If PPC doesn't want to create vdso for all its userspace apps, we
> > could instead "don't create" vdso during the execve call.
> >
> >
> > > They were against the removal of the fallback; other archs also have
> > > this infrastructure.  Really, if we fixed the fallback to work for
> > > all platforms then it would probably also remove the possibility of a
> > > remap over the VDSO being a problem (if it is today, which still isn't
> > > clear?).
> > >
> > Any past thread/communication about this that I can read ?
>
> Jeff, I'm sure you don't intend to, but I find it quite disrespectful that you
> ignored my feedback here (and elsewhere, regarding you ignoring 4 sets of
> feedback).
>
I'm just interested in the details :),  If we know why PPC needs to
unmap/remap vdso, then there are additional data points to consider,
when we develop pre-process level control for this feature. But sure,
we can postpone this.

> This?
>
> https://elixir.bootlin.com/linux/v6.13.4/source/arch/powerpc/kernel/vdso.c#L236
>
OK, you meant the failed case ? i.e. when install_special_mappings
failed ? That is a case that I haven't considered. It looks like error
handling, and I was expecting the install_special_mappings to never
fail, maybe I'm wrong here for PPC.

> Was [0] a relevant discussion?
>
Sorry, I'm kind of lost.  This link doesn't give a reason why PPC
needs to be unmap. If it is due to CRIU or  other user space apps,
that is not an architecture dependency, maybe a distribution
dependency.

Anyway, we can postpone this discussion for PPC, I don't mean to make
you spend more time responding to me. Please feel free to ignore this
one.

Thanks.
-Jeff


> [0]: https://lore.kernel.org/all/lhe2mky6ahlk2jzvvfjyongqiseelyx2uy7sbyuso6jcy3b2dq@7ju6cea62jgk/
>
> >
> > Thanks
> > -Jeff
> >
> >
> > > Thanks,
> > > Liam

Re: your mail

[Jeff Xu]

Hi Lorenzo,

On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes
>
> > > 2. Tests - could you please add some tests to assert that mremap() fails
> > >    for VDSO for instance? You've edited an existing test for VDSO in x86 to
> > >    skip the test should this be enabled, but this is not the same as a self
> > >    test. And obviously doesn't cover arm64.
> > >
> > >    This should be relatively strightforward right? You already have code
> > >    for finding out whether VDSO is msealed, so just use that to see if you
> > >    skip, then attempt mremap(), mmap() over etc. + assert it fails.
> > >
> > >    Ideally these tests would cover all the cases you've changed.
> > >
> > It is not as easy.
> >
> > The config is disabled by default. And I don't know a way to detect
> > KCONFIG  from selftest itself. Without this, I can't reasonably
> > determine the test result.
>
> Please in future let's try to get this kind of response at the point of the
> request being made :) makes life much easier.
>
There might be miscommunication ?
This version is the first time you ask about adding a self test.

IIRC, you had comments about providing the details of what tests I did, in V4.
As a follow-up, I added a test-info section in the cover letter of V5

Though I have thought about adding a selftest since the beginning,
there are two problems:
1> This config is by default disabled,
2> No good pattern to check KCONFIG from selftest.

>
> So I think it is easy actually. As I say here (perhaps you missed it?) you
> literally already have code you added to the x86 test to prevent test
> failure.
>
> So you essentially look for [vdso] or whatever, then you look up to see if
> it is sealed, ensure an mremap() fails.
>
This suggestion doesn't test the core change of this series, which is
to enable mseal for vdso.

When the vdso is marked with "sl", mremap() will fail, that's part of
the mseal() business logic and belongs in mseal_test. The mseal_test
already covers the mseal, and this series doesn't change mseal.

As for the "sl", as I responded in the "refactor mseal_test" [1] , it
will be part of the verifying step:

[1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@mail.gmail.com/

> Of course this doesn't check to see if it _should_ be enabled or not. I'm
> being nice by not _insisting_ we export a way for you to determine whether
> this config option is enabled or not for the sake of a test (since I don't
> want to hold up this series).
>
> But that'd be nice! Maybe in a
> /sys/kernel/security endpoint...
>
>
> ...but I think we can potentially add this later on so we don't hold things
> up here/add yet more to the series. I suspect you and Kees alike would
> prioritise getting _this series_ in at this point :)
>
> You could, if you wanted to, check to see if /proc/config.gz exists and
> zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on
> that, but you know kinda hacky.

Ya, it is hacky. None of the existing selftest uses this pattern, and
I'm not sure /proc/config.gz is enabled in the default kernel config.

One option is to have ChromeOS or Android to maintain an out-of-tree
test, since the configuration will be enabled there.

Option two is to create a new path:
tools/testing/selftests/sealsysmap. Then, add
CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to
this path. This seems to be the preferred way by selftest, but we need
a new dir for that.

Option three is to add a self-test when we have a process-level opt-in
solution. This would allow the test to deterministically know whether
the vdso should be sealed or not.

>
> But anyway, FWIW I think it'd be useful to assert that mremap() or munmap()
> fails here for system mappings for the sake of it. I guess this is, in
> effect, only checking mseal-ing works right? But it at least asserts the
> existence of the thing, and that it behaves.
>
> Later on, when you add some way of observing that it's enabled or not, you
> can extend the test to check this.

I think it is best that we don't add a test that doesn't actually
check the code change. Do you think one of the above three options
works ? maybe the second option (with a new path) ?

In any case, I think the risk is low, and the code changes are quite
simple, and fully tested.

Thanks.
-Jeff.

Re: your mail

[Lorenzo Stoakes]

On Thu, Feb 27, 2025 at 04:55:06PM -0800, Jeff Xu wrote:
> Hi Lorenzo,
>
> On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes
> >
> > > > 2. Tests - could you please add some tests to assert that mremap() fails
> > > >    for VDSO for instance? You've edited an existing test for VDSO in x86 to
> > > >    skip the test should this be enabled, but this is not the same as a self
> > > >    test. And obviously doesn't cover arm64.
> > > >
> > > >    This should be relatively strightforward right? You already have code
> > > >    for finding out whether VDSO is msealed, so just use that to see if you
> > > >    skip, then attempt mremap(), mmap() over etc. + assert it fails.
> > > >
> > > >    Ideally these tests would cover all the cases you've changed.
> > > >
> > > It is not as easy.
> > >
> > > The config is disabled by default. And I don't know a way to detect
> > > KCONFIG  from selftest itself. Without this, I can't reasonably
> > > determine the test result.
> >
> > Please in future let's try to get this kind of response at the point of the
> > request being made :) makes life much easier.
> >
> There might be miscommunication ?
> This version is the first time you ask about adding a self test.

Yeah I thought I'd been clear but this might _very well_ have been me not
having been, so apologies if so.

I mean 'make sure it's tested' is an overloaded term right? So fact you've
tested on android, chromeos, etc. implies 'tested', but also self
tests/kunit/whatever.

>
> IIRC, you had comments about providing the details of what tests I did, in V4.
> As a follow-up, I added a test-info section in the cover letter of V5

Thanks. Appreciate that! And this really does point towards a
miscommunication on my part, will try to be super explicit in future.

>
> Though I have thought about adding a selftest since the beginning,
> there are two problems:
> 1> This config is by default disabled,
> 2> No good pattern to check KCONFIG from selftest.

Yeah agreed, it's a TOTAL pain.

I wish we had a better way of doing this. Maybe a self-volunteering
exercise (*goes to write on writeboard :P*)

>
> >
> > So I think it is easy actually. As I say here (perhaps you missed it?) you
> > literally already have code you added to the x86 test to prevent test
> > failure.
> >
> > So you essentially look for [vdso] or whatever, then you look up to see if
> > it is sealed, ensure an mremap() fails.
> >
> This suggestion doesn't test the core change of this series, which is
> to enable mseal for vdso.

Right, and thinking about it, what does this test? Just that mseal works
right?

It's sort of implicit that, if a VMA is sealed, the seal should work (or
rather, tested in mseal tests themselves, rather than mseal system
settings).

>
> When the vdso is marked with "sl", mremap() will fail, that's part of
> the mseal() business logic and belongs in mseal_test. The mseal_test
> already covers the mseal, and this series doesn't change mseal.
>
> As for the "sl", as I responded in the "refactor mseal_test" [1] , it
> will be part of the verifying step:
>
> [1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@mail.gmail.com/

OK cool thanks. I will look at this when I can, I'm just snowed under
pre-LSF and have been sick so backlog, backlog as discussed off-list. But
it's not been forgotten (whiteboard etc. etc.)

>
> > Of course this doesn't check to see if it _should_ be enabled or not. I'm
> > being nice by not _insisting_ we export a way for you to determine whether
> > this config option is enabled or not for the sake of a test (since I don't
> > want to hold up this series).
> >
> > But that'd be nice! Maybe in a
> > /sys/kernel/security endpoint...
> >
> >
> > ...but I think we can potentially add this later on so we don't hold things
> > up here/add yet more to the series. I suspect you and Kees alike would
> > prioritise getting _this series_ in at this point :)
> >
> > You could, if you wanted to, check to see if /proc/config.gz exists and
> > zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on
> > that, but you know kinda hacky.
>
> Ya, it is hacky. None of the existing selftest uses this pattern, and
> I'm not sure /proc/config.gz is enabled in the default kernel config.

Yeah and I'm not sure I even like my hacky suggestion here, I mean let's be
honest, it sucks.

>
> One option is to have ChromeOS or Android to maintain an out-of-tree
> test, since the configuration will be enabled there.

Nah haha, though of course you can do what you want. Really want something
upstream.

>
> Option two is to create a new path:
> tools/testing/selftests/sealsysmap. Then, add
> CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to
> this path. This seems to be the preferred way by selftest, but we need
> a new dir for that.

OK I like option 2 let's do this.

But let's call it mseal_system_mappings (yes I"m being nitty again :) I
really want to try to _explicitly_ say 'mseal' because we have other forms
of sealing.

Not your fault, but we overload terms like _crazy_ in mm and need to be
cautious as not to confuse vs. e.g. memfd seals.


>
> Option three is to add a self-test when we have a process-level opt-in
> solution. This would allow the test to deterministically know whether
> the vdso should be sealed or not.

Yeah one for future.

>
> >
> > But anyway, FWIW I think it'd be useful to assert that mremap() or munmap()
> > fails here for system mappings for the sake of it. I guess this is, in
> > effect, only checking mseal-ing works right? But it at least asserts the
> > existence of the thing, and that it behaves.
> >
> > Later on, when you add some way of observing that it's enabled or not, you
> > can extend the test to check this.
>
> I think it is best that we don't add a test that doesn't actually
> check the code change. Do you think one of the above three options
> works ? maybe the second option (with a new path) ?

Yeah I actually agree on reflection. And yes agreed option 2 is great,
thanks!

>
> In any case, I think the risk is low, and the code changes are quite
> simple, and fully tested.

Yeah indeed, but I'd really like _something_ if possible. If option 2 is
relatively quick let's get that sorted out!

>
> Thanks.
> -Jeff.

Cheers, Lorenzo

Re: [PATCH v7 4/7] mseal, system mappings: enable arm64

[Lorenzo Stoakes]

On Thu, Feb 27, 2025 at 04:48:23PM -0800, Jeff Xu wrote:
> On Wed, Feb 26, 2025 at 9:43 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 09:17:10AM -0800, Jeff Xu wrote:
> > > On Wed, Feb 26, 2025 at 9:12 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > > >
> > > > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [250226 00:26]:
> > > > > On Tue, Feb 25, 2025 at 02:26:50PM -0800, Jeff Xu wrote:
> > > > > > On Mon, Feb 24, 2025 at 10:20 PM Lorenzo Stoakes
> > > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 24, 2025 at 10:52:43PM +0000, jeffxu@chromium.org wrote:
> > > > > > > > From: Jeff Xu <jeffxu@chromium.org>
> > > > > > > >
> > > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on arm64, covering
> > > > > > > > the vdso, vvar, and compat-mode vectors and sigpage mappings.
> > > > > > > >
> > > > > > > > Production release testing passes on Android and Chrome OS.
> > > > > > >
> > > > > > > This is pretty limited (yes yes I know android is massive etc. but we must
> > > > > > > account for all the weird and wonderful arm64 devices out there in context of
> > > > > > > upstream :)
> > > > > > >
> > > > > > > Have you looking through all arm64-code relating to vdso, vvar, compat-mode
> > > > > > > vectors, sigpage mapping and ensured nothing kernel-side relies upon relocation?
> > > > > > > Some arches actually seem to want to do this. Pretty sure PPC does... so a bit
> > > > > > > nervous of that.
> > > > > > >
> > > > > > Can you please point out where PPC munmap/mremap the vdso ?
> > > > > >
> > > > > > Previously, when you mentioned that, I thought you meant user space in
> > > > > > PPC, I didn't realize that you meant that kernel code in PPC.  I
> > > > > > tried, but didn't find anything, hence asking.
> > > > >
> > > > > Jeff, please stick to replying to review. 'Have you looking through all
> > > > > arm64-code'.
> > > > >
> I checked the kernel code and couldn't find any instances of kernel
> unmap/remap system mapping in any architecture. But I could be wrong,
> so I've also included developers from different architectures since
> V1, and hoping to get some insight.

Thanks, yeah me also. Perhaps you said somewhere but I missed, apologies if
so. Might be worth adding explicitly to commit messages, though I think you
allude to it actually.

>
> > > > > I ended up doing this myself yesterday and found no issues, as with x86-64.
> > > > >
> Thank you for double checking.

No problem, in the went was fairly quick.

>
> > > > > I said I'm _pretty sure_ PPC does this. Liam mentioned something about
> > > > > it. We can discuss it, and I can find specifics if + when you try to add
> > > > > this to PPC.
> > > > >
> > > >
> > > > PPC allows the vma to be munmapped then detects and falls back to the
> > > > slower method, iirc.
> > > >
> > > Is this code in the kernel or userspace?
> > >
> > > If PPC doesn't want to create vdso for all its userspace apps, we
> > > could instead "don't create" vdso during the execve call.
> > >
> > >
> > > > They were against the removal of the fallback; other archs also have
> > > > this infrastructure.  Really, if we fixed the fallback to work for
> > > > all platforms then it would probably also remove the possibility of a
> > > > remap over the VDSO being a problem (if it is today, which still isn't
> > > > clear?).
> > > >
> > > Any past thread/communication about this that I can read ?
> >
> > Jeff, I'm sure you don't intend to, but I find it quite disrespectful that you
> > ignored my feedback here (and elsewhere, regarding you ignoring 4 sets of
> > feedback).
> >
> I'm just interested in the details :),  If we know why PPC needs to
> unmap/remap vdso, then there are additional data points to consider,
> when we develop pre-process level control for this feature. But sure,
> we can postpone this.
>
> > This?
> >
> > https://elixir.bootlin.com/linux/v6.13.4/source/arch/powerpc/kernel/vdso.c#L236
> >
> OK, you meant the failed case ? i.e. when install_special_mappings
> failed ? That is a case that I haven't considered. It looks like error
> handling, and I was expecting the install_special_mappings to never
> fail, maybe I'm wrong here for PPC.
>
> > Was [0] a relevant discussion?
> >
> Sorry, I'm kind of lost.  This link doesn't give a reason why PPC
> needs to be unmap. If it is due to CRIU or  other user space apps,
> that is not an architecture dependency, maybe a distribution
> dependency.

Yeah I actually think the web might be somewhat tangled here, I don't know
but...

>
> Anyway, we can postpone this discussion for PPC, I don't mean to make
> you spend more time responding to me. Please feel free to ignore this
> one.

...agreed, let's postpone this to there, at least hopefully these links provide
some basis for further research! :)

>
> Thanks.
> -Jeff
>
>
> > [0]: https://lore.kernel.org/all/lhe2mky6ahlk2jzvvfjyongqiseelyx2uy7sbyuso6jcy3b2dq@7ju6cea62jgk/
> >
> > >
> > > Thanks
> > > -Jeff
> > >
> > >
> > > > Thanks,
> > > > Liam

Cheers, Lorenzo

Re: [PATCH v7 1/7] mseal, system mappings: kernel config and header change

[Lorenzo Stoakes]

On Thu, Feb 27, 2025 at 04:04:03PM -0800, Jeff Xu wrote:
> On Tue, Feb 25, 2025 at 10:04 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 05:33:24PM -0800, Jeff Xu wrote:
> > > On Mon, Feb 24, 2025 at 10:05 PM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> > > > > +     bool
> > > > > +     help
> > > > > +       Control MSEAL_SYSTEM_MAPPINGS access based on architecture.
> > > > > +
> > > > > +       A 64-bit kernel is required for the memory sealing feature.
> > > > > +       No specific hardware features from the CPU are needed.
> > > > > +
> > > > > +       To enable this feature, the architecture needs to update their
> > > > > +       special mappings calls to include the sealing flag and confirm
> > > > > +       that it doesn't unmap/remap system mappings during the life
> > > > > +       time of the process. After the architecture enables this, a
> > > > > +       distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to manage access
> > > > > +       to the feature.
> > > >
> > > > Architectures also need to be confirmed not to require any form of VDSO
> > > > relocation, which as discussed in previous series some arches appear to
> > > > need to do. I'd mention that here.
> > > >
> > > This might need clarification, the system mapping includes vdso, right
> > > ? Why the focus on vdso ?
> >
> > My mistake, I thought scope was more limited than this when I first
> > looked. Please disregard the focus on VDSO here... :)
> >
> > >
> > > The sentence  "... it doesn't unmap/remap system mappings during the
> > > lifetime of the process."  already cover what you want here, I think.
> > >
> >
> > Right, I guess it just doesn't quite _emphasise_ it enough for me. Something
> > like the below would really help bring that out:
> >
> >         The existing of this flag for an architecture implies that it does not
> >         require the remapping of these system mappings during process lifetime,
> >         so sealing these mappings is safe from a kernel perspective.
> >
> I'm not sure I get the difference, but I can add it,  is below OK ?
>
> To enable this feature, the architecture needs to update their
> special mappings calls to include the sealing flag and confirm
> that it doesn't unmap/remap system mappings during the life
> time of the process. The existence of this flag for an architecture
> implies that it does not require the remapping of these system
> mappings during process lifetime, so sealing these mappings is
> safe from a kernel perspective. After the architecture enables this,
> a distribution can set CONFIG_MSEAL_SYSTEM_MAPPING to
> manage access to the feature.

Sounds great, cheers!

>
> Thanks
> -Jeff

Re: [PATCH v7 0/7] mseal system mappings

[Lorenzo Stoakes]

On Thu, Feb 27, 2025 at 03:43:07PM -0800, Jeff Xu wrote:
> On Tue, Feb 25, 2025 at 10:01 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 04:17:01PM -0800, Jeff Xu wrote:
> > > On Tue, Feb 25, 2025 at 2:32 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > BTW can we please drop the 'mseal, system mappings' prefixes on this
> > > > series, it's really weird and makes it really hard for me to actually read
> > > > the individual summary lines for each commit. 'mseal:' will do.
> > > >
> > >
> > > I am not sure.
> > > I had comments about adding mseal, system mappings, as prefixes, and I
> > > think it is reasonable.
> >
> > No it's really horrible (sorry I know it's hyperbolic but it really really looks
> > horrid to me) , I've never seen prefixes like that before in mm in my life and I
> > don't think it adds anything.
> >
> > I also find it MIGHTY confusing.
> >
> > Please remove this :) you can git log the relevant files to see the conventions
> > people use. Typically xxx: has something really short and sweet for the 'xxx'.
> >
> > I realise this is subjective, but it's a small thing and would be helpful for
> > parsing your series at a glance.
> >
> I need a prefix to group the patches, especially when the first patch
> is just a config change. "mseal" won't work because this patch isn't
> about core business logic of mseal.
>
> How about "mseal sysmap:"?

OK let's go with that as a compromise. This isn't a huge big deal.

>
> -Jeff

Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes]

On Thu, Feb 27, 2025 at 03:38:47PM -0800, Jeff Xu wrote:
> On Wed, Feb 26, 2025 at 10:25 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 07:20:50PM +0100, Oleg Nesterov wrote:
> > > On 02/26, Lorenzo Stoakes wrote:
> > > >
> > > > Like I said, Jeff opposes the change. I disagree with him, and agree with you,
> > > > because this is very silly.
> > > >
> > > > But I don't want to hold up this series with that discussion (this is for his
> > > > sake...)
> > >
> > > Neither me, so lets go with VM_SEALED_SYSMAP.
> > >
> > > My only objection is that
> > >
> > >       vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> > >       vm_flags |= VM_SEALED_SYSMAP;
> > >
> > > looks unnecessarily confusing to me,
> > >
> > >       vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP;
> > >
> > > or just
> > >
> > >       vma = _install_special_mapping(...,
> > >                               VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_SEALED_SYSMAP,
> > >                               ...
> > >
> > > looks more readable. But this is cosmetic/subjective, so I won't argue/insist.
> >
> > Agreed. This would be good.
> >
> > >
> > > > Jeff - perhaps drop this and let's return to it in a follow up so this series
> > > > isn't held up?
> > >
> > > Up to you and Jeff.
> > >
> > > But this patch looks "natural" to me in this series.
> >
> > OK, I mean in that case I'm ok with it as-is, as you confirms there's no
> > issue, I've looked at the code and there's no issue.
> >
> > It was only if we wanted to try the VM_SEALED thing, i.e. _always_ seal
> > then it'd do better outside of the series as there'd be a discussion about
> > maybe changing this CONFIG_64BIT thing yada yada.
> >
> > >
> > > Oleg.
> > >
> >
> > Jeff - in that case, do NOT drop this one :P but do please look at the
> > above style nit.
> >
> Ok.

Thanks :)

>
>
> > Let's keep things moving... :)

Re: your mail

[Jeff Xu]

On Fri, Feb 28, 2025 at 1:35 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Feb 27, 2025 at 04:55:06PM -0800, Jeff Xu wrote:
> > Hi Lorenzo,
> >
> > On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes
> > >
> > > > > 2. Tests - could you please add some tests to assert that mremap() fails
> > > > >    for VDSO for instance? You've edited an existing test for VDSO in x86 to
> > > > >    skip the test should this be enabled, but this is not the same as a self
> > > > >    test. And obviously doesn't cover arm64.
> > > > >
> > > > >    This should be relatively strightforward right? You already have code
> > > > >    for finding out whether VDSO is msealed, so just use that to see if you
> > > > >    skip, then attempt mremap(), mmap() over etc. + assert it fails.
> > > > >
> > > > >    Ideally these tests would cover all the cases you've changed.
> > > > >
> > > > It is not as easy.
> > > >
> > > > The config is disabled by default. And I don't know a way to detect
> > > > KCONFIG  from selftest itself. Without this, I can't reasonably
> > > > determine the test result.
> > >
> > > Please in future let's try to get this kind of response at the point of the
> > > request being made :) makes life much easier.
> > >
> > There might be miscommunication ?
> > This version is the first time you ask about adding a self test.
>
> Yeah I thought I'd been clear but this might _very well_ have been me not
> having been, so apologies if so.
>
> I mean 'make sure it's tested' is an overloaded term right? So fact you've
> tested on android, chromeos, etc. implies 'tested', but also self
> tests/kunit/whatever.
>
> >
> > IIRC, you had comments about providing the details of what tests I did, in V4.
> > As a follow-up, I added a test-info section in the cover letter of V5
>
> Thanks. Appreciate that! And this really does point towards a
> miscommunication on my part, will try to be super explicit in future.
>
> >
> > Though I have thought about adding a selftest since the beginning,
> > there are two problems:
> > 1> This config is by default disabled,
> > 2> No good pattern to check KCONFIG from selftest.
>
> Yeah agreed, it's a TOTAL pain.
>
> I wish we had a better way of doing this. Maybe a self-volunteering
> exercise (*goes to write on writeboard :P*)
>
> >
> > >
> > > So I think it is easy actually. As I say here (perhaps you missed it?) you
> > > literally already have code you added to the x86 test to prevent test
> > > failure.
> > >
> > > So you essentially look for [vdso] or whatever, then you look up to see if
> > > it is sealed, ensure an mremap() fails.
> > >
> > This suggestion doesn't test the core change of this series, which is
> > to enable mseal for vdso.
>
> Right, and thinking about it, what does this test? Just that mseal works
> right?
>
> It's sort of implicit that, if a VMA is sealed, the seal should work (or
> rather, tested in mseal tests themselves, rather than mseal system
> settings).
>
> >
> > When the vdso is marked with "sl", mremap() will fail, that's part of
> > the mseal() business logic and belongs in mseal_test. The mseal_test
> > already covers the mseal, and this series doesn't change mseal.
> >
> > As for the "sl", as I responded in the "refactor mseal_test" [1] , it
> > will be part of the verifying step:
> >
> > [1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@mail.gmail.com/
>
> OK cool thanks. I will look at this when I can, I'm just snowed under
> pre-LSF and have been sick so backlog, backlog as discussed off-list. But
> it's not been forgotten (whiteboard etc. etc.)
>
Ya, no worry about that review, please take time to recover, I will wait.
And appreciate your time and take priority for reviewing this series.

> >
> > > Of course this doesn't check to see if it _should_ be enabled or not. I'm
> > > being nice by not _insisting_ we export a way for you to determine whether
> > > this config option is enabled or not for the sake of a test (since I don't
> > > want to hold up this series).
> > >
> > > But that'd be nice! Maybe in a
> > > /sys/kernel/security endpoint...
> > >
> > >
> > > ...but I think we can potentially add this later on so we don't hold things
> > > up here/add yet more to the series. I suspect you and Kees alike would
> > > prioritise getting _this series_ in at this point :)
> > >
> > > You could, if you wanted to, check to see if /proc/config.gz exists and
> > > zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on
> > > that, but you know kinda hacky.
> >
> > Ya, it is hacky. None of the existing selftest uses this pattern, and
> > I'm not sure /proc/config.gz is enabled in the default kernel config.
>
> Yeah and I'm not sure I even like my hacky suggestion here, I mean let's be
> honest, it sucks.
>
> >
> > One option is to have ChromeOS or Android to maintain an out-of-tree
> > test, since the configuration will be enabled there.
>
> Nah haha, though of course you can do what you want. Really want something
> upstream.
>
> >
> > Option two is to create a new path:
> > tools/testing/selftests/sealsysmap. Then, add
> > CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to
> > this path. This seems to be the preferred way by selftest, but we need
> > a new dir for that.
>
> OK I like option 2 let's do this.
>
> But let's call it mseal_system_mappings (yes I"m being nitty again :) I
> really want to try to _explicitly_ say 'mseal' because we have other forms
> of sealing.
>
Sure.

If long path names aren't a problem, I will use mseal_system_mappings,
otherwise mseal_sysmap.

> Not your fault, but we overload terms like _crazy_ in mm and need to be
> cautious as not to confuse vs. e.g. memfd seals.
>
>
> >
> > Option three is to add a self-test when we have a process-level opt-in
> > solution. This would allow the test to deterministically know whether
> > the vdso should be sealed or not.
>
> Yeah one for future.
>
> >
> > >
> > > But anyway, FWIW I think it'd be useful to assert that mremap() or munmap()
> > > fails here for system mappings for the sake of it. I guess this is, in
> > > effect, only checking mseal-ing works right? But it at least asserts the
> > > existence of the thing, and that it behaves.
> > >
> > > Later on, when you add some way of observing that it's enabled or not, you
> > > can extend the test to check this.
> >
> > I think it is best that we don't add a test that doesn't actually
> > check the code change. Do you think one of the above three options
> > works ? maybe the second option (with a new path) ?
>
> Yeah I actually agree on reflection. And yes agreed option 2 is great,
> thanks!
>
> >
> > In any case, I think the risk is low, and the code changes are quite
> > simple, and fully tested.
>
> Yeah indeed, but I'd really like _something_ if possible. If option 2 is
> relatively quick let's get that sorted out!
>
Great ! I will work on option 2.

Thanks
-Jeff

Re: your mail

[Lorenzo Stoakes]

On Fri, Feb 28, 2025 at 09:24:11AM -0800, Jeff Xu wrote:

[snip]

> > >
> > > When the vdso is marked with "sl", mremap() will fail, that's part of
> > > the mseal() business logic and belongs in mseal_test. The mseal_test
> > > already covers the mseal, and this series doesn't change mseal.
> > >
> > > As for the "sl", as I responded in the "refactor mseal_test" [1] , it
> > > will be part of the verifying step:
> > >
> > > [1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@mail.gmail.com/
> >
> > OK cool thanks. I will look at this when I can, I'm just snowed under
> > pre-LSF and have been sick so backlog, backlog as discussed off-list. But
> > it's not been forgotten (whiteboard etc. etc.)
> >
> Ya, no worry about that review, please take time to recover, I will wait.
> And appreciate your time and take priority for reviewing this series.

Thanks :)

[snip]

> > >
> > > Option two is to create a new path:
> > > tools/testing/selftests/sealsysmap. Then, add
> > > CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to
> > > this path. This seems to be the preferred way by selftest, but we need
> > > a new dir for that.
> >
> > OK I like option 2 let's do this.
> >
> > But let's call it mseal_system_mappings (yes I"m being nitty again :) I
> > really want to try to _explicitly_ say 'mseal' because we have other forms
> > of sealing.
> >
> Sure.

Thanks!

>
> If long path names aren't a problem, I will use mseal_system_mappings,
> otherwise mseal_sysmap.

I think long form is fine.

[snip]

> > >
> > > In any case, I think the risk is low, and the code changes are quite
> > > simple, and fully tested.
> >
> > Yeah indeed, but I'd really like _something_ if possible. If option 2 is
> > relatively quick let's get that sorted out!
> >
> Great ! I will work on option 2.

Perfect cheers!

>
> Thanks
> -Jeff