* [PATCH RFT v13 3/8] selftests: Provide helper header for shadow stack testing
From: Mark Brown @ 2024-12-03 18:43 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241203-clone3-shadow-stack-v13-0-93b89a81a5ed@kernel.org>
While almost all users of shadow stacks should be relying on the dynamic
linker and libc to enable the feature there are several low level test
programs where it is useful to enable without any libc support, allowing
testing without full system enablement. This low level testing is helpful
during bringup of the support itself, and also in enabling coverage by
automated testing without needing all system components in the target root
filesystems to have enablement.
Provide a header with helpers for this purpose, intended for use only by
test programs directly exercising shadow stack interfaces.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/tools/testing/selftests/ksft_shstk.h b/tools/testing/selftests/ksft_shstk.h
new file mode 100644
index 0000000000000000000000000000000000000000..869ecea2bf3ea3d30cead9819d2b3a75f5397754
--- /dev/null
+++ b/tools/testing/selftests/ksft_shstk.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Helpers for shadow stack enablement, this is intended to only be
+ * used by low level test programs directly exercising interfaces for
+ * working with shadow stacks.
+ *
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#ifndef __KSFT_SHSTK_H
+#define __KSFT_SHSTK_H
+
+#include <asm/mman.h>
+
+/* This is currently only defined for x86 */
+#ifndef SHADOW_STACK_SET_TOKEN
+#define SHADOW_STACK_SET_TOKEN (1ULL << 0)
+#endif
+
+static bool shadow_stack_enabled;
+
+#ifdef __x86_64__
+#define ARCH_SHSTK_ENABLE 0x5001
+#define ARCH_SHSTK_SHSTK (1ULL << 0)
+
+#define ARCH_PRCTL(arg1, arg2) \
+({ \
+ long _ret; \
+ register long _num asm("eax") = __NR_arch_prctl; \
+ register long _arg1 asm("rdi") = (long)(arg1); \
+ register long _arg2 asm("rsi") = (long)(arg2); \
+ \
+ asm volatile ( \
+ "syscall\n" \
+ : "=a"(_ret) \
+ : "r"(_arg1), "r"(_arg2), \
+ "0"(_num) \
+ : "rcx", "r11", "memory", "cc" \
+ ); \
+ _ret; \
+})
+
+#define ENABLE_SHADOW_STACK
+static inline __attribute__((always_inline)) void enable_shadow_stack(void)
+{
+ int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifdef __aarch64__
+#define PR_SET_SHADOW_STACK_STATUS 75
+# define PR_SHADOW_STACK_ENABLE (1UL << 0)
+
+#define my_syscall2(num, arg1, arg2) \
+({ \
+ register long _num __asm__ ("x8") = (num); \
+ register long _arg1 __asm__ ("x0") = (long)(arg1); \
+ register long _arg2 __asm__ ("x1") = (long)(arg2); \
+ register long _arg3 __asm__ ("x2") = 0; \
+ register long _arg4 __asm__ ("x3") = 0; \
+ register long _arg5 __asm__ ("x4") = 0; \
+ \
+ __asm__ volatile ( \
+ "svc #0\n" \
+ : "=r"(_arg1) \
+ : "r"(_arg1), "r"(_arg2), \
+ "r"(_arg3), "r"(_arg4), \
+ "r"(_arg5), "r"(_num) \
+ : "memory", "cc" \
+ ); \
+ _arg1; \
+})
+
+#define ENABLE_SHADOW_STACK
+static inline __attribute__((always_inline)) void enable_shadow_stack(void)
+{
+ int ret;
+
+ ret = my_syscall2(__NR_prctl, PR_SET_SHADOW_STACK_STATUS,
+ PR_SHADOW_STACK_ENABLE);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifndef __NR_map_shadow_stack
+#define __NR_map_shadow_stack 453
+#endif
+
+#ifndef ENABLE_SHADOW_STACK
+static inline void enable_shadow_stack(void) { }
+#endif
+
+#endif
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v13 2/8] Documentation: userspace-api: Add shadow stack API documentation
From: Mark Brown @ 2024-12-03 18:43 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20241203-clone3-shadow-stack-v13-0-93b89a81a5ed@kernel.org>
There are a number of architectures with shadow stack features which we are
presenting to userspace with as consistent an API as we can (though there
are some architecture specifics). Especially given that there are some
important considerations for userspace code interacting directly with the
feature let's provide some documentation covering the common aspects.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 42 ++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index 274cc7546efc2a042d2dc00aa67c71c52372179a..c39709bfba2c5682d0d1a22444db17c17bcf01ce 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -59,6 +59,7 @@ Everything else
ELF
netlink/index
+ shadow_stack
sysfs-platform_profile
vduse
futex2
diff --git a/Documentation/userspace-api/shadow_stack.rst b/Documentation/userspace-api/shadow_stack.rst
new file mode 100644
index 0000000000000000000000000000000000000000..9d0d4e79cfa7c47d3208dd53071a3d0b86c18575
--- /dev/null
+++ b/Documentation/userspace-api/shadow_stack.rst
@@ -0,0 +1,42 @@
+=============
+Shadow Stacks
+=============
+
+Introduction
+============
+
+Several architectures have features which provide backward edge
+control flow protection through a hardware maintained stack, only
+writeable by userspace through very limited operations. This feature
+is referred to as shadow stacks on Linux, on x86 it is part of Intel
+Control Enforcement Technology (CET), on arm64 it is Guarded Control
+Stacks feature (FEAT_GCS) and for RISC-V it is the Zicfiss extension.
+It is expected that this feature will normally be managed by the
+system dynamic linker and libc in ways broadly transparent to
+application code, this document covers interfaces and considerations.
+
+
+Enabling
+========
+
+Shadow stacks default to disabled when a userspace process is
+executed, they can be enabled for the current thread with a syscall:
+
+ - For x86 the ARCH_SHSTK_ENABLE arch_prctl()
+ - For other architectures the PR_SET_SHADOW_STACK_ENABLE prctl()
+
+It is expected that this will normally be done by the dynamic linker.
+Any new threads created by a thread with shadow stacks enabled will
+themselves have shadow stacks enabled.
+
+
+Enablement considerations
+=========================
+
+- Returning from the function that enables shadow stacks without first
+ disabling them will cause a shadow stack exception. This includes
+ any syscall wrapper or other library functions, the syscall will need
+ to be inlined.
+- A lock feature allows userspace to prevent disabling of shadow stacks.
+- Those that change the stack context like longjmp() or use of ucontext
+ changes on signal return will need support from libc.
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v13 1/8] arm64/gcs: Return a success value from gcs_alloc_thread_stack()
From: Mark Brown @ 2024-12-03 18:43 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook
In-Reply-To: <20241203-clone3-shadow-stack-v13-0-93b89a81a5ed@kernel.org>
Currently as a result of templating from x86 code gcs_alloc_thread_stack()
returns a pointer as an unsigned int however on arm64 we don't actually use
this pointer value as anything other than a pass/fail flag. Simplify the
interface to just return an int with 0 on success and a negative error code
on failure.
Acked-by: Deepak Gupta <debug@rivosinc.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/include/asm/gcs.h | 8 ++++----
arch/arm64/kernel/process.c | 8 ++++----
arch/arm64/mm/gcs.c | 8 ++++----
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index f50660603ecf5dc09a92740062df3a089b02b219..d8923b5f03b776252aca76ce316ef57399d71fa9 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -64,8 +64,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
void gcs_set_el0_mode(struct task_struct *task);
void gcs_free(struct task_struct *task);
void gcs_preserve_current_state(void);
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args);
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args);
static inline int gcs_check_locked(struct task_struct *task,
unsigned long new_val)
@@ -91,8 +91,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
static inline void gcs_set_el0_mode(struct task_struct *task) { }
static inline void gcs_free(struct task_struct *task) { }
static inline void gcs_preserve_current_state(void) { }
-static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+static inline int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
return -ENOTSUPP;
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 2968a33bb3bc16208ff672590fd9a9a8d0b26b19..c217ab67e82baa212d008b62b876acf8b2b492d6 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -297,7 +297,7 @@ static void flush_gcs(void)
static int copy_thread_gcs(struct task_struct *p,
const struct kernel_clone_args *args)
{
- unsigned long gcs;
+ int ret;
if (!system_supports_gcs())
return 0;
@@ -305,9 +305,9 @@ static int copy_thread_gcs(struct task_struct *p,
p->thread.gcs_base = 0;
p->thread.gcs_size = 0;
- gcs = gcs_alloc_thread_stack(p, args);
- if (IS_ERR_VALUE(gcs))
- return PTR_ERR((void *)gcs);
+ ret = gcs_alloc_thread_stack(p, args);
+ if (ret != 0)
+ return ret;
p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 5c46ec527b1cdaa8f52cff445d70ba0f8509d086..1f633a482558b59aac5427963d42b37fce08c8a6 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -38,8 +38,8 @@ static unsigned long gcs_size(unsigned long size)
return max(PAGE_SIZE, size);
}
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
unsigned long addr, size;
@@ -59,13 +59,13 @@ unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
size = gcs_size(size);
addr = alloc_gcs(0, size);
if (IS_ERR_VALUE(addr))
- return addr;
+ return PTR_ERR((void *)addr);
tsk->thread.gcs_base = addr;
tsk->thread.gcs_size = size;
tsk->thread.gcspr_el0 = addr + size - sizeof(u64);
- return addr;
+ return 0;
}
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
--
2.39.5
^ permalink raw reply related
* [PATCH RFT v13 0/8] fork: Support shadow stacks in clone3()
From: Mark Brown @ 2024-12-03 18:43 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Yury Khrustalev, Wilco Dijkstra, linux-kselftest, linux-api,
Mark Brown, Kees Cook, Kees Cook, Shuah Khan
The kernel has recently added support for shadow stacks, currently
x86 only using their CET feature but both arm64 and RISC-V have
equivalent features (GCS and Zicfiss respectively), I am actively
working on GCS[1]. With shadow stacks the hardware maintains an
additional stack containing only the return addresses for branch
instructions which is not generally writeable by userspace and ensures
that any returns are to the recorded addresses. This provides some
protection against ROP attacks and making it easier to collect call
stacks. These shadow stacks are allocated in the address space of the
userspace process.
Our API for shadow stacks does not currently offer userspace any
flexiblity for managing the allocation of shadow stacks for newly
created threads, instead the kernel allocates a new shadow stack with
the same size as the normal stack whenever a thread is created with the
feature enabled. The stacks allocated in this way are freed by the
kernel when the thread exits or shadow stacks are disabled for the
thread. This lack of flexibility and control isn't ideal, in the vast
majority of cases the shadow stack will be over allocated and the
implicit allocation and deallocation is not consistent with other
interfaces. As far as I can tell the interface is done in this manner
mainly because the shadow stack patches were in development since before
clone3() was implemented.
Since clone3() is readily extensible let's add support for specifying a
shadow stack when creating a new thread or process, keeping the current
implicit allocation behaviour if one is not specified either with
clone3() or through the use of clone(). The user must provide a shadow
stack pointer, this must point to memory mapped for use as a shadow
stackby map_shadow_stack() with an architecture specified shadow stack
token at the top of the stack.
Please note that the x86 portions of this code are build tested only, I
don't appear to have a system that can run CET available to me.
[1] https://lore.kernel.org/linux-arm-kernel/20241001-arm64-gcs-v13-0-222b78d87eee@kernel.org/T/#mc58f97f27461749ccf400ebabf6f9f937116a86b
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v13:
- Rebase onto v6.13-rc1.
- Link to v12: https://lore.kernel.org/r/20241031-clone3-shadow-stack-v12-0-7183eb8bee17@kernel.org
Changes in v12:
- Add the regular prctl() to the userspace API document since arm64
support is queued in -next.
- Link to v11: https://lore.kernel.org/r/20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org
Changes in v11:
- Rebase onto arm64 for-next/gcs, which is based on v6.12-rc1, and
integrate arm64 support.
- Rework the interface to specify a shadow stack pointer rather than a
base and size like we do for the regular stack.
- Link to v10: https://lore.kernel.org/r/20240821-clone3-shadow-stack-v10-0-06e8797b9445@kernel.org
Changes in v10:
- Integrate fixes & improvements for the x86 implementation from Rick
Edgecombe.
- Require that the shadow stack be VM_WRITE.
- Require that the shadow stack base and size be sizeof(void *) aligned.
- Clean up trailing newline.
- Link to v9: https://lore.kernel.org/r/20240819-clone3-shadow-stack-v9-0-962d74f99464@kernel.org
Changes in v9:
- Pull token validation earlier and report problems with an error return
to parent rather than signal delivery to the child.
- Verify that the top of the supplied shadow stack is VM_SHADOW_STACK.
- Rework token validation to only do the page mapping once.
- Drop no longer needed support for testing for signals in selftest.
- Fix typo in comments.
- Link to v8: https://lore.kernel.org/r/20240808-clone3-shadow-stack-v8-0-0acf37caf14c@kernel.org
Changes in v8:
- Fix token verification with user specified shadow stack.
- Don't track user managed shadow stacks for child processes.
- Link to v7: https://lore.kernel.org/r/20240731-clone3-shadow-stack-v7-0-a9532eebfb1d@kernel.org
Changes in v7:
- Rebase onto v6.11-rc1.
- Typo fixes.
- Link to v6: https://lore.kernel.org/r/20240623-clone3-shadow-stack-v6-0-9ee7783b1fb9@kernel.org
Changes in v6:
- Rebase onto v6.10-rc3.
- Ensure we don't try to free the parent shadow stack in error paths of
x86 arch code.
- Spelling fixes in userspace API document.
- Additional cleanups and improvements to the clone3() tests to support
the shadow stack tests.
- Link to v5: https://lore.kernel.org/r/20240203-clone3-shadow-stack-v5-0-322c69598e4b@kernel.org
Changes in v5:
- Rebase onto v6.8-rc2.
- Rework ABI to have the user allocate the shadow stack memory with
map_shadow_stack() and a token.
- Force inlining of the x86 shadow stack enablement.
- Move shadow stack enablement out into a shared header for reuse by
other tests.
- Link to v4: https://lore.kernel.org/r/20231128-clone3-shadow-stack-v4-0-8b28ffe4f676@kernel.org
Changes in v4:
- Formatting changes.
- Use a define for minimum shadow stack size and move some basic
validation to fork.c.
- Link to v3: https://lore.kernel.org/r/20231120-clone3-shadow-stack-v3-0-a7b8ed3e2acc@kernel.org
Changes in v3:
- Rebase onto v6.7-rc2.
- Remove stale shadow_stack in internal kargs.
- If a shadow stack is specified unconditionally use it regardless of
CLONE_ parameters.
- Force enable shadow stacks in the selftest.
- Update changelogs for RISC-V feature rename.
- Link to v2: https://lore.kernel.org/r/20231114-clone3-shadow-stack-v2-0-b613f8681155@kernel.org
Changes in v2:
- Rebase onto v6.7-rc1.
- Remove ability to provide preallocated shadow stack, just specify the
desired size.
- Link to v1: https://lore.kernel.org/r/20231023-clone3-shadow-stack-v1-0-d867d0b5d4d0@kernel.org
---
Mark Brown (8):
arm64/gcs: Return a success value from gcs_alloc_thread_stack()
Documentation: userspace-api: Add shadow stack API documentation
selftests: Provide helper header for shadow stack testing
fork: Add shadow stack support to clone3()
selftests/clone3: Remove redundant flushes of output streams
selftests/clone3: Factor more of main loop into test_clone3()
selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
selftests/clone3: Test shadow stack support
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 42 ++++
arch/arm64/include/asm/gcs.h | 8 +-
arch/arm64/kernel/process.c | 8 +-
arch/arm64/mm/gcs.c | 62 +++++-
arch/x86/include/asm/shstk.h | 11 +-
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 57 +++++-
include/asm-generic/cacheflush.h | 11 ++
include/linux/sched/task.h | 17 ++
include/uapi/linux/sched.h | 10 +-
kernel/fork.c | 96 +++++++--
tools/testing/selftests/clone3/clone3.c | 226 ++++++++++++++++++----
tools/testing/selftests/clone3/clone3_selftests.h | 65 ++++++-
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++
15 files changed, 633 insertions(+), 81 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20231019-clone3-shadow-stack-15d40d2bf536
Best regards,
--
Mark Brown <broonie@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa_utils.c
From: Andrew Lunn @ 2024-12-03 1:51 UTC (permalink / raw)
To: D. Wythe; +Cc: John Ousterhout, netdev, linux-api
In-Reply-To: <f79a70fd-35a4-4d0d-b239-daa4ab652880@linux.alibaba.com>
On Mon, Dec 02, 2024 at 11:51:48AM +0800, D. Wythe wrote:
> > +/**
> > + * homa_setsockopt() - Implements the getsockopt system call for Homa sockets.
> > + * @sk: Socket on which the system call was invoked.
> > + * @level: Level at which the operation should be handled; will always
> > + * be IPPROTO_HOMA.
> > + * @optname: Identifies a particular setsockopt operation.
> > + * @optval: Address in user space of information about the option.
> > + * @optlen: Number of bytes of data at @optval.
> > + * Return: 0 on success, otherwise a negative errno.
> > + */
> > +int homa_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval,
> > + unsigned int optlen)
> > +{
> > + struct homa_sock *hsk = homa_sk(sk);
> > + struct homa_set_buf_args args;
> > + int ret;
> > +
> > + if (level != IPPROTO_HOMA || optname != SO_HOMA_SET_BUF ||
> > + optlen != sizeof(struct homa_set_buf_args))
> > + return -EINVAL;
>
> SO_HOMA_SET_BUF is a bit odd here, maybe HOMA_RCVBUF ? which also can be
> implemented in getsockopt.
Please trim the text when replying to just the needed context. With
pages and pages of useless quoted text it is easy to overlook your
comments.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 12/12] net: homa: create Makefile and Kconfig
From: John Ousterhout @ 2024-12-02 23:27 UTC (permalink / raw)
To: D. Wythe; +Cc: netdev, linux-api
In-Reply-To: <f81a78ac-b32a-44bf-9375-8ac380bbce74@linux.alibaba.com>
On Mon, Nov 25, 2024 at 8:27 PM D. Wythe <alibuda@linux.alibaba.com> wrote:
>
> On 11/12/24 7:40 AM, John Ousterhout wrote:
>
> A small formatting issue, perhaps you're using spaces?
Yep; I fixed the formatting in Kconfig based on input from Randy
Dunlap earlier in this message train.
> > +menuconfig HOMA
> > + tristate "The Homa transport protocol"
> > + depends on INET
> > + depends on IPV6
>
> Can HOMA run in an environment without IPv6（IPv4 only)? If so, depends is not suitable here. Perhaps
> what you need is to implement different branches in the code using
>
> #if IS_ENABLED(CONFIG_IPV6)
No, Homa really can't run in environments without IPv6: internally,
Homa stores all addresses as IPv6 addresses, converting to/from IPv4
at the interfaces with other kernel functions. Hopefully that is not a
problem? And given that, "depends on IPV6" is OK, right?
-John-
^ permalink raw reply
* Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Christian Brauner @ 2024-12-02 14:31 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Oleg Nesterov, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard
In-Reply-To: <b8f4664c-b8f0-46ca-b9a3-8d73e398b5ca@lucifer.local>
On Wed, Oct 30, 2024 at 04:37:37PM +0000, Lorenzo Stoakes wrote:
> On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
> > I guess I'll try to adapt that and respin a v7 when I get a chance.
>
> Hm looking at this draft patch, it seems like a total rework of pidfd's
> across the board right (now all pidfd's will need to be converted to
> pid_fd)? Correct me if I'm wrong.
>
> If only for the signal case, it seems like overkill to define a whole
> pid_fd and to use this CLASS() wrapper just for this one instance.
>
> If the intent is to convert _all_ pidfd's to use this type, it feels really
> out of scope for this series and I think we'd probably instead want to go
> off and do that as a separate series and put this on hold until that is
> done.
>
> If instead you mean that we ought to do something like this just for the
> signal case, it feels like it'd be quite a bit of extra abstraction just
> used in this one case but nowhere else, I think if you did an abstraction
> like this it would _have_ to be across the board right?
>
> I agree that the issue is with this one signal case that pins only the fd
> (rather than this pid) where this 'pinning' doesn't _necessary_ mess around
> with reference counts.
>
> So we definitely must address this, but the issue you had with the first
> approach was that I think (correct me if I'm wrong) I was passing a pointer
> to a struct fd which is not permitted right?
>
> Could we pass the struct fd by value to avoid this? I think we'd have to
> unfortunately special-case this and probably duplicate some code which is a
> pity as I liked the idea of abstracting everything to one place, but we can
> obviously do that.
>
> So I guess to TL;DR it, the options are:
>
> 1. Implement pid_fd everywhere, in which case I will leave off on
> this series and I guess, if I have time I could look at trying to
> implement that or perhaps you'd prefer to?
>
> 2. We are good for the sake of this series to special-case a pidfd_to_pid()
> implementation (used only by the pidfd_send_signal() syscall)
>
> 3. Something else, or I am misunderstanding your point :)
>
> Let me know how you want me to proceed on this as we're at v6 already and I
> want to be _really_ sure I'm doing what you want here.
I don't think we get away with abstracting it in one place without this
ending up a pretty janky api. I need to go back and think about calling
conventions for all this stuff. For now I think I'm fine with something
like the below that abstracts the api to handle mm/ cleanly and then a
special-case for pidfd_send_signal():
diff --git a/kernel/pid.c b/kernel/pid.c
index 6131543e7c09..c1857c44d1a3 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -564,15 +564,29 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
*/
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags)
{
- unsigned int f_flags;
+ unsigned int f_flags = 0;
struct pid *pid;
struct task_struct *task;
+ enum pid_type type;
- pid = pidfd_get_pid(pidfd, &f_flags);
- if (IS_ERR(pid))
- return ERR_CAST(pid);
+ switch (pidfd) {
+ case PIDFD_SELF_THREAD:
+ type = PIDTYPE_PID;
+ pid = get_task_pid(current, type);
+ break;
+ case PIDFD_SELF_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ pid = get_task_pid(current, type);
+ break;
+ default:
+ pid = pidfd_get_pid(pidfd, &f_flags);
+ if (IS_ERR(pid))
+ return ERR_CAST(pid);
+ type = PIDTYPE_TGID;
+ break;
+ }
- task = get_pid_task(pid, PIDTYPE_TGID);
+ task = get_pid_task(pid, type);
put_pid(pid);
if (!task)
return ERR_PTR(-ESRCH);
That would get you support for PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP for process_madvise() and process_mrelease().
And for pidfd_send_signal() we could just open code this for now:
diff --git a/kernel/signal.c b/kernel/signal.c
index 989b1cc9116a..a2e4e3a5ee42 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3990,6 +3990,45 @@ static struct pid *pidfd_to_pid(const struct file *file)
(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
PIDFD_SIGNAL_PROCESS_GROUP)
+static int do_pidfd_send_signal(struct pid *pid, int sig, enum pid_type type,
+ siginfo_t __user *info, unsigned int flags)
+{
+ kernel_siginfo_t kinfo;
+
+ switch (flags) {
+ case PIDFD_SIGNAL_THREAD:
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ }
+
+ if (info) {
+ int ret = copy_siginfo_from_user_any(&kinfo, info);
+ if (unlikely(ret))
+ return ret;
+
+ if (unlikely(sig != kinfo.si_signo))
+ return -EINVAL;
+
+ /* Only allow sending arbitrary signals to yourself. */
+ if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
+ (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
+ return -EPERM;
+ } else {
+ prepare_kill_siginfo(sig, &kinfo, type);
+ }
+
+ if (type == PIDTYPE_PGID)
+ return kill_pgrp_info(sig, &kinfo, pid);
+
+ return kill_pid_info_type(sig, &kinfo, pid, type);
+}
+
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -4009,7 +4048,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
{
int ret;
struct pid *pid;
- kernel_siginfo_t kinfo;
enum pid_type type;
/* Enforce flags be set to 0 until we add an extension. */
@@ -4021,56 +4059,39 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
return -EINVAL;
CLASS(fd, f)(pidfd);
- if (fd_empty(f))
- return -EBADF;
- /* Is this a pidfd? */
- pid = pidfd_to_pid(fd_file(f));
- if (IS_ERR(pid))
- return PTR_ERR(pid);
+ switch (pidfd) {
+ case PIDFD_SELF_THREAD:
+ pid = get_task_pid(current, PIDTYPE_PID);
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SELF_THREAD_GROUP:
+ pid = get_task_pid(current, PIDTYPE_TGID);
+ type = PIDTYPE_TGID;
+ break;
+ default:
+ if (fd_empty(f))
+ return -EBADF;
- if (!access_pidfd_pidns(pid))
- return -EINVAL;
+ /* Is this a pidfd? */
+ pid = pidfd_to_pid(fd_file(f));
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
- switch (flags) {
- case 0:
+ if (!access_pidfd_pidns(pid))
+ return -EINVAL;
/* Infer scope from the type of pidfd. */
if (fd_file(f)->f_flags & PIDFD_THREAD)
type = PIDTYPE_PID;
else
type = PIDTYPE_TGID;
break;
- case PIDFD_SIGNAL_THREAD:
- type = PIDTYPE_PID;
- break;
- case PIDFD_SIGNAL_THREAD_GROUP:
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_PROCESS_GROUP:
- type = PIDTYPE_PGID;
- break;
}
- if (info) {
- ret = copy_siginfo_from_user_any(&kinfo, info);
- if (unlikely(ret))
- return ret;
-
- if (unlikely(sig != kinfo.si_signo))
- return -EINVAL;
-
- /* Only allow sending arbitrary signals to yourself. */
- if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
- (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
- return -EPERM;
- } else {
- prepare_kill_siginfo(sig, &kinfo, type);
- }
-
- if (type == PIDTYPE_PGID)
- return kill_pgrp_info(sig, &kinfo, pid);
- else
- return kill_pid_info_type(sig, &kinfo, pid, type);
+ ret = do_pidfd_send_signal(pid, sig, type, info, flags);
+ if (fd_empty(f))
+ put_pid(pid);
+ return ret;
}
static int
^ permalink raw reply related
* Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-12-02 10:52 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard
In-Reply-To: <55764300-1b53-4d14-99cc-e735d3704713@lucifer.local>
On Fri, Nov 08, 2024 at 02:28:14PM +0000, Lorenzo Stoakes wrote:
> On Wed, Oct 30, 2024 at 04:37:37PM +0000, Lorenzo Stoakes wrote:
> > On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
> > > I guess I'll try to adapt that and respin a v7 when I get a chance.
> >
> > Hm looking at this draft patch, it seems like a total rework of pidfd's
> > across the board right (now all pidfd's will need to be converted to
> > pid_fd)? Correct me if I'm wrong.
> >
> > If only for the signal case, it seems like overkill to define a whole
> > pid_fd and to use this CLASS() wrapper just for this one instance.
> >
> > If the intent is to convert _all_ pidfd's to use this type, it feels really
> > out of scope for this series and I think we'd probably instead want to go
> > off and do that as a separate series and put this on hold until that is
> > done.
> >
> > If instead you mean that we ought to do something like this just for the
> > signal case, it feels like it'd be quite a bit of extra abstraction just
> > used in this one case but nowhere else, I think if you did an abstraction
> > like this it would _have_ to be across the board right?
> >
> > I agree that the issue is with this one signal case that pins only the fd
> > (rather than this pid) where this 'pinning' doesn't _necessary_ mess around
> > with reference counts.
> >
> > So we definitely must address this, but the issue you had with the first
> > approach was that I think (correct me if I'm wrong) I was passing a pointer
> > to a struct fd which is not permitted right?
> >
> > Could we pass the struct fd by value to avoid this? I think we'd have to
> > unfortunately special-case this and probably duplicate some code which is a
> > pity as I liked the idea of abstracting everything to one place, but we can
> > obviously do that.
> >
> > So I guess to TL;DR it, the options are:
> >
> > 1. Implement pid_fd everywhere, in which case I will leave off on
> > this series and I guess, if I have time I could look at trying to
> > implement that or perhaps you'd prefer to?
> >
> > 2. We are good for the sake of this series to special-case a pidfd_to_pid()
> > implementation (used only by the pidfd_send_signal() syscall)
> >
> > 3. Something else, or I am misunderstanding your point :)
> >
> > Let me know how you want me to proceed on this as we're at v6 already and I
> > want to be _really_ sure I'm doing what you want here.
> >
> > Thanks!
>
> Hi Christian,
>
> Just a gentle nudge on this - as I need some guidance in order to know how
> to move the series forwards.
>
> Obviously no rush if your workload is high at the moment as this is pretty
> low priority, but just in case you missed it :)
>
> Thanks, Lorenzo
Hi Christian,
Just a ping on this now we're past the merge window and it's been over a
month.
It'd be good to at least get a polite ack to indicate you're aware even if
you don't have the time to respond right now.
If you'd prefer this series not to go ahead just let me know, but
unfortunately I really require your input to know how to move forward
otherwise I risk doing work that you might then reject.
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa_utils.c
From: D. Wythe @ 2024-12-02 3:51 UTC (permalink / raw)
To: John Ousterhout, netdev, linux-api
In-Reply-To: <20241111234006.5942-12-ouster@cs.stanford.edu>
On 11/12/24 7:40 AM, John Ousterhout wrote:
> homa_plumbing.c contains functions that connect Homa to the rest of
> the Linux kernel, such as dispatch tables used by Linux and the
> top-level functions that Linux invokes from those dispatch tables.
>
> homa_utils.c contains a few odds and ends, such as code to initialize
> and destroy struct homa's.
>
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
> ---
> net/homa/homa_plumbing.c | 965 +++++++++++++++++++++++++++++++++++++++
> net/homa/homa_utils.c | 150 ++++++
> 2 files changed, 1115 insertions(+)
> create mode 100644 net/homa/homa_plumbing.c
> create mode 100644 net/homa/homa_utils.c
>
> diff --git a/net/homa/homa_plumbing.c b/net/homa/homa_plumbing.c
> new file mode 100644
> index 000000000000..afd3a9cc97ba
> --- /dev/null
> +++ b/net/homa/homa_plumbing.c
> @@ -0,0 +1,965 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +
> +/* This file consists mostly of "glue" that hooks Homa into the rest of
> + * the Linux kernel. The guts of the protocol are in other files.
> + */
> +
> +#include "homa_impl.h"
> +#include "homa_peer.h"
> +#include "homa_pool.h"
> +
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_AUTHOR("John Ousterhout");
> +MODULE_DESCRIPTION("Homa transport protocol");
> +MODULE_VERSION("0.01");
> +
> +/* Not yet sure what these variables are for */
> +static long sysctl_homa_mem[3] __read_mostly;
> +static int sysctl_homa_rmem_min __read_mostly;
> +static int sysctl_homa_wmem_min __read_mostly;
> +
> +/* Global data for Homa. Never reference homa_data directory. Always use
> + * the homa variable instead; this allows overriding during unit tests.
> + */
> +static struct homa homa_data;
> +struct homa *homa = &homa_data;
> +
> +/* True means that the Homa module is in the process of unloading itself,
> + * so everyone should clean up.
> + */
> +static bool exiting;
> +
> +/* Thread that runs timer code to detect lost packets and crashed peers. */
> +static struct task_struct *timer_kthread;
> +
> +/* This structure defines functions that handle various operations on
> + * Homa sockets. These functions are relatively generic: they are called
> + * to implement top-level system calls. Many of these operations can
> + * be implemented by PF_INET6 functions that are independent of the
> + * Homa protocol.
> + */
> +static const struct proto_ops homa_proto_ops = {
> + .family = PF_INET,
> + .owner = THIS_MODULE,
> + .release = inet_release,
> + .bind = homa_bind,
> + .connect = inet_dgram_connect,
> + .socketpair = sock_no_socketpair,
> + .accept = sock_no_accept,
> + .getname = inet_getname,
> + .poll = homa_poll,
> + .ioctl = inet_ioctl,
> + .listen = sock_no_listen,
> + .shutdown = homa_shutdown,
> + .setsockopt = sock_common_setsockopt,
> + .getsockopt = sock_common_getsockopt,
> + .sendmsg = inet_sendmsg,
> + .recvmsg = inet_recvmsg,
> + .mmap = sock_no_mmap,
> + .set_peek_off = sk_set_peek_off,
> +};
> +
> +static const struct proto_ops homav6_proto_ops = {
> + .family = PF_INET6,
> + .owner = THIS_MODULE,
> + .release = inet6_release,
> + .bind = homa_bind,
> + .connect = inet_dgram_connect,
> + .socketpair = sock_no_socketpair,
> + .accept = sock_no_accept,
> + .getname = inet6_getname,
> + .poll = homa_poll,
> + .ioctl = inet6_ioctl,
> + .listen = sock_no_listen,
> + .shutdown = homa_shutdown,
> + .setsockopt = sock_common_setsockopt,
> + .getsockopt = sock_common_getsockopt,
> + .sendmsg = inet_sendmsg,
> + .recvmsg = inet_recvmsg,
> + .mmap = sock_no_mmap,
> + .set_peek_off = sk_set_peek_off,
> +};
> +
> +/* This structure also defines functions that handle various operations
> + * on Homa sockets. However, these functions are lower-level than those
> + * in homa_proto_ops: they are specific to the PF_INET or PF_INET6
> + * protocol family, and in many cases they are invoked by functions in
> + * homa_proto_ops. Most of these functions have Homa-specific implementations.
> + */
> +static struct proto homa_prot = {
> + .name = "HOMA",
> + .owner = THIS_MODULE,
> + .close = homa_close,
> + .connect = ip4_datagram_connect,
> + .disconnect = homa_disconnect,
> + .ioctl = homa_ioctl,
> + .init = homa_socket,
> + .destroy = NULL,
> + .setsockopt = homa_setsockopt,
> + .getsockopt = homa_getsockopt,
> + .sendmsg = homa_sendmsg,
> + .recvmsg = homa_recvmsg,
> + .backlog_rcv = homa_backlog_rcv,
> + .hash = homa_hash,
> + .unhash = homa_unhash,
> + .get_port = homa_get_port,
> + .sysctl_mem = sysctl_homa_mem,
> + .sysctl_wmem = &sysctl_homa_wmem_min,
> + .sysctl_rmem = &sysctl_homa_rmem_min,
> + .obj_size = sizeof(struct homa_sock),
> + .no_autobind = 1,
> +};
> +
> +static struct proto homav6_prot = {
> + .name = "HOMAv6",
> + .owner = THIS_MODULE,
> + .close = homa_close,
> + .connect = ip6_datagram_connect,
> + .disconnect = homa_disconnect,
> + .ioctl = homa_ioctl,
> + .init = homa_socket,
> + .destroy = NULL,
> + .setsockopt = homa_setsockopt,
> + .getsockopt = homa_getsockopt,
> + .sendmsg = homa_sendmsg,
> + .recvmsg = homa_recvmsg,
> + .backlog_rcv = homa_backlog_rcv,
> + .hash = homa_hash,
> + .unhash = homa_unhash,
> + .get_port = homa_get_port,
> + .sysctl_mem = sysctl_homa_mem,
> + .sysctl_wmem = &sysctl_homa_wmem_min,
> + .sysctl_rmem = &sysctl_homa_rmem_min,
> +
> + /* IPv6 data comes *after* Homa's data, and isn't included in
> + * struct homa_sock.
> + */
> + .obj_size = sizeof(struct homa_sock) + sizeof(struct ipv6_pinfo),
> + .no_autobind = 1,
> +};
> +
> +/* Top-level structure describing the Homa protocol. */
> +static struct inet_protosw homa_protosw = {
> + .type = SOCK_DGRAM,
> + .protocol = IPPROTO_HOMA,
> + .prot = &homa_prot,
> + .ops = &homa_proto_ops,
> + .flags = INET_PROTOSW_REUSE,
> +};
> +
> +static struct inet_protosw homav6_protosw = {
> + .type = SOCK_DGRAM,
> + .protocol = IPPROTO_HOMA,
> + .prot = &homav6_prot,
> + .ops = &homav6_proto_ops,
> + .flags = INET_PROTOSW_REUSE,
> +};
> +
> +/* This structure is used by IP to deliver incoming Homa packets to us. */
> +static struct net_protocol homa_protocol = {
> + .handler = homa_softirq,
> + .err_handler = homa_err_handler_v4,
> + .no_policy = 1,
> +};
> +
> +static struct inet6_protocol homav6_protocol = {
> + .handler = homa_softirq,
> + .err_handler = homa_err_handler_v6,
> + .flags = INET6_PROTO_NOPOLICY | INET6_PROTO_FINAL,
> +};
> +
> +/* Sizes of the headers for each Homa packet type, in bytes. */
> +static __u16 header_lengths[] = {
> + sizeof32(struct data_header),
> + 0,
> + sizeof32(struct resend_header),
> + sizeof32(struct unknown_header),
> + sizeof32(struct busy_header),
> + 0,
> + sizeof32(struct common_header),
> + sizeof32(struct need_ack_header),
> + sizeof32(struct ack_header)
> +};
> +
> +static DECLARE_COMPLETION(timer_thread_done);
> +
> +/**
> + * homa_load() - invoked when this module is loaded into the Linux kernel
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +static int __init homa_load(void)
> +{
> + int status;
> +
> + pr_notice("Homa module loading\n");
> + pr_notice("Homa structure sizes: data_header %u, seg_header %u, ack %u, peer %u, ip_hdr %u flowi %u ipv6_hdr %u, flowi6 %u tcp_sock %u homa_rpc %u sk_buff %u rcvmsg_control %u union sockaddr_in_union %u HOMA_MAX_BPAGES %u NR_CPUS %u nr_cpu_ids %u, MAX_NUMNODES %d\n",
> + sizeof32(struct data_header),
> + sizeof32(struct seg_header),
> + sizeof32(struct homa_ack),
> + sizeof32(struct homa_peer),
> + sizeof32(struct iphdr),
> + sizeof32(struct flowi),
> + sizeof32(struct ipv6hdr),
> + sizeof32(struct flowi6),
> + sizeof32(struct tcp_sock),
> + sizeof32(struct homa_rpc),
> + sizeof32(struct sk_buff),
> + sizeof32(struct homa_recvmsg_args),
> + sizeof32(union sockaddr_in_union),
> + HOMA_MAX_BPAGES,
> + NR_CPUS,
> + nr_cpu_ids,
> + MAX_NUMNODES);
> + status = proto_register(&homa_prot, 1);
> + if (status != 0) {
> + pr_err("proto_register failed for homa_prot: %d\n", status);
> + goto out;
> + }
> + status = proto_register(&homav6_prot, 1);
> + if (status != 0) {
> + pr_err("proto_register failed for homav6_prot: %d\n", status);
> + goto out;
> + }
> + inet_register_protosw(&homa_protosw);
> + inet6_register_protosw(&homav6_protosw);
> + status = inet_add_protocol(&homa_protocol, IPPROTO_HOMA);
> + if (status != 0) {
> + pr_err("inet_add_protocol failed in %s: %d\n", __func__,
> + status);
> + goto out_cleanup;
> + }
> + status = inet6_add_protocol(&homav6_protocol, IPPROTO_HOMA);
> + if (status != 0) {
> + pr_err("inet6_add_protocol failed in %s: %d\n", __func__,
> + status);
> + goto out_cleanup;
> + }
> +
> + status = homa_init(homa);
> + if (status)
> + goto out_cleanup;
> +
> + timer_kthread = kthread_run(homa_timer_main, homa, "homa_timer");
> + if (IS_ERR(timer_kthread)) {
> + status = PTR_ERR(timer_kthread);
> + pr_err("couldn't create homa pacer thread: error %d\n",
> + status);
> + timer_kthread = NULL;
> + goto out_cleanup;
> + }
> +
> + return 0;
> +
> +out_cleanup:
> + homa_destroy(homa);
> + inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> + inet_unregister_protosw(&homa_protosw);
> + inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> + inet6_unregister_protosw(&homav6_protosw);
> + proto_unregister(&homa_prot);
> + proto_unregister(&homav6_prot);
> +out:
> + return status;
> +}
> +
> +/**
> + * homa_unload() - invoked when this module is unloaded from the Linux kernel.
> + */
> +static void __exit homa_unload(void)
> +{
> + pr_notice("Homa module unloading\n");
> + exiting = true;
> +
> + if (timer_kthread)
> + wake_up_process(timer_kthread);
> + wait_for_completion(&timer_thread_done);
> + homa_destroy(homa);
> + inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> + inet_unregister_protosw(&homa_protosw);
> + inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> + inet6_unregister_protosw(&homav6_protosw);
> + proto_unregister(&homa_prot);
> + proto_unregister(&homav6_prot);
> +}
> +
> +module_init(homa_load);
> +module_exit(homa_unload);
> +
> +/**
> + * homa_bind() - Implements the bind system call for Homa sockets: associates
> + * a well-known service port with a socket. Unlike other AF_INET6 protocols,
> + * there is no need to invoke this system call for sockets that are only
> + * used as clients.
> + * @sock: Socket on which the system call was invoked.
> + * @addr: Contains the desired port number.
> + * @addr_len: Number of bytes in uaddr.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> +{
> + struct homa_sock *hsk = homa_sk(sock->sk);
> + union sockaddr_in_union *addr_in = (union sockaddr_in_union *)addr;
> + int port = 0;
> +
> + if (unlikely(addr->sa_family != sock->sk->sk_family))
> + return -EAFNOSUPPORT;
> + if (addr_in->in6.sin6_family == AF_INET6) {
> + if (addr_len < sizeof(struct sockaddr_in6))
> + return -EINVAL;
> + port = ntohs(addr_in->in4.sin_port);
> + } else if (addr_in->in4.sin_family == AF_INET) {
> + if (addr_len < sizeof(struct sockaddr_in))
> + return -EINVAL;
> + port = ntohs(addr_in->in6.sin6_port);
> + }
> + return homa_sock_bind(homa->port_map, hsk, port);
> +}
> +
> +/**
> + * homa_close() - Invoked when close system call is invoked on a Homa socket.
> + * @sk: Socket being closed
> + * @timeout: ??
> + */
> +void homa_close(struct sock *sk, long timeout)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> +
> + homa_sock_destroy(hsk);
> + sk_common_release(sk);
> +}
> +
> +/**
> + * homa_shutdown() - Implements the shutdown system call for Homa sockets.
> + * @sock: Socket to shut down.
> + * @how: Ignored: for other sockets, can independently shut down
> + * sending and receiving, but for Homa any shutdown will
> + * shut down everything.
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_shutdown(struct socket *sock, int how)
> +{
> + homa_sock_shutdown(homa_sk(sock->sk));
> + return 0;
> +}
> +
> +/**
> + * homa_disconnect() - Invoked when disconnect system call is invoked on a
> + * Homa socket.
> + * @sk: Socket to disconnect
> + * @flags: ??
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_disconnect(struct sock *sk, int flags)
> +{
> + pr_warn("unimplemented disconnect invoked on Homa socket\n");
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_ioctl() - Implements the ioctl system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @cmd: Identifier for a particular ioctl operation.
> + * @karg: Operation-specific argument; typically the address of a block
> + * of data in user address space.
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_ioctl(struct sock *sk, int cmd, int *karg)
> +{
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_socket() - Implements the socket(2) system call for sockets.
> + * @sk: Socket on which the system call was invoked. The non-Homa
> + * parts have already been initialized.
> + *
> + * Return: always 0 (success).
> + */
> +int homa_socket(struct sock *sk)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> +
> + homa_sock_init(hsk, homa);
> + return 0;
> +}
> +
> +/**
> + * homa_setsockopt() - Implements the getsockopt system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @level: Level at which the operation should be handled; will always
> + * be IPPROTO_HOMA.
> + * @optname: Identifies a particular setsockopt operation.
> + * @optval: Address in user space of information about the option.
> + * @optlen: Number of bytes of data at @optval.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval,
> + unsigned int optlen)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_set_buf_args args;
> + int ret;
> +
> + if (level != IPPROTO_HOMA || optname != SO_HOMA_SET_BUF ||
> + optlen != sizeof(struct homa_set_buf_args))
> + return -EINVAL;
SO_HOMA_SET_BUF is a bit odd here, maybe HOMA_RCVBUF ? which also can be
implemented in getsockopt.
> +
> + if (copy_from_sockptr(&args, optval, optlen))
> + return -EFAULT;
> +
> + /* Do a trivial test to make sure we can at least write the first
> + * page of the region.
> + */
> + if (copy_to_user((__force void __user *)args.start, &args, sizeof(args)))
> + return -EFAULT;
To share buffer between kernel and userspace, maybe you should refer to the implementation of
io_pin_pbuf_ring()
> +
> + homa_sock_lock(hsk, "homa_setsockopt SO_HOMA_SET_BUF");
> + ret = homa_pool_init(hsk, (__force void __user *)args.start, args.length);
> + homa_sock_unlock(hsk);
It seems that if the option was not set, poll->hsk will not be initialized
and then if recvmsg is called, a panic will be triggered.
homa_recvmsg
homa_wait_for_message
...
homa_rpc_reap
homa_pool_check_waiting
homa_sock_lock(pool->hsk, "buffer pool");
> + return ret;
> +}
> +
> +/**
> + * homa_getsockopt() - Implements the getsockopt system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @level: ??
> + * @optname: Identifies a particular setsockopt operation.
> + * @optval: Address in user space where the option's value should be stored.
> + * @option: ??.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_getsockopt(struct sock *sk, int level, int optname,
> + char __user *optval, int __user *option)
> +{
> + pr_warn("unimplemented getsockopt invoked on Homa socket: level %d, optname %d\n",
> + level, optname);
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_sendmsg() - Send a request or response message on a Homa socket.
> + * @sk: Socket on which the system call was invoked.
> + * @msg: Structure describing the message to send; the msg_control
> + * field points to additional information.
> + * @length: Number of bytes of the message.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_sendmsg(struct sock *sk, struct msghdr *msg, size_t length)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_sendmsg_args args;
> + int result = 0;
> + struct homa_rpc *rpc = NULL;
> + union sockaddr_in_union *addr = (union sockaddr_in_union *)msg->msg_name;
msg->msg_name can be NULL.
> +
> + if (unlikely(!msg->msg_control_is_user)) {
> + result = -EINVAL;
> + goto error;
> + }
> + if (unlikely(copy_from_user(&args,
> + (__force void __user *)msg->msg_control,
> + sizeof(args)))) {
> + result = -EFAULT;
> + goto error;
> + }
> + if (addr->in6.sin6_family != sk->sk_family) {
> + result = -EAFNOSUPPORT;
> + goto error;
> + }
addr->sa.sa_family? Using sin6_family would be odd to me, making it seem like homa can only run with
IPv6.
> + if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> + (msg->msg_namelen < sizeof(struct sockaddr_in6) &&
> + addr->in6.sin6_family == AF_INET6)) {
> + result = -EINVAL;
> + goto error;
> + }
> +
> + if (!args.id) {
> + /* This is a request message. */
> + rpc = homa_rpc_new_client(hsk, addr);
> + if (IS_ERR(rpc)) {
> + result = PTR_ERR(rpc);
> + rpc = NULL;
> + goto error;
> + }
> + rpc->completion_cookie = args.completion_cookie;
> + result = homa_message_out_fill(rpc, &msg->msg_iter, 1);
> + if (result)
> + goto error;
> + args.id = rpc->id;
> + homa_rpc_unlock(rpc);
Strongly recommend that adding comments with all the unlock part to indicate where it was
locked from.
> + rpc = NULL;
> +
> + if (unlikely(copy_to_user((__force void __user *)msg->msg_control,
> + &args, sizeof(args)))) {
> + rpc = homa_find_client_rpc(hsk, args.id);
> + result = -EFAULT;
> + goto error;
> + }
> + } else {
> + /* This is a response message. */
> + struct in6_addr canonical_dest;
> +
> + if (args.completion_cookie != 0) {
> + result = -EINVAL;
> + goto error;
> + }
> + canonical_dest = canonical_ipv6_addr(addr);
Are you treating all addresses as IPv6 addresses just for the sake of simplicity? It's a bit odd,
but okay to me.
> +
> + rpc = homa_find_server_rpc(hsk, &canonical_dest,
> + ntohs(addr->in6.sin6_port), args.id);
> + if (!rpc)
> + /* Return without an error if the RPC doesn't exist;
> + * this could be totally valid (e.g. client is
> + * no longer interested in it).
> + */
> + return 0;
> + if (rpc->error) {
> + result = rpc->error;
> + goto error;
> + }
> + if (rpc->state != RPC_IN_SERVICE) {
> + homa_rpc_unlock(rpc);
> + rpc = NULL;
> + result = -EINVAL;
> + goto error;
> + }
> + rpc->state = RPC_OUTGOING;
> +
> + result = homa_message_out_fill(rpc, &msg->msg_iter, 1);
> + if (result && rpc->state != RPC_DEAD)
> + goto error;
> + homa_rpc_unlock(rpc);
> + }
> + return 0;
> +
> +error:
> + if (rpc) {
> + homa_rpc_free(rpc);
> + homa_rpc_unlock(rpc);
> + }
> + return result;
> +}
> +
> +/**
> + * homa_recvmsg() - Receive a message from a Homa socket.
> + * @sk: Socket on which the system call was invoked.
> + * @msg: Controlling information for the receive.
> + * @len: Total bytes of space available in msg->msg_iov; not used.
> + * @flags: Flags from system call; only MSG_DONTWAIT is used.
> + * @addr_len: Store the length of the sender address here
> + * Return: The length of the message on success, otherwise a negative
> + * errno.
> + */
> +int homa_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags,
> + int *addr_len)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_recvmsg_args control;
> + struct homa_rpc *rpc;
> + int result;
> +
> + if (unlikely(!msg->msg_control)) {
> + /* This test isn't strictly necessary, but it provides a
> + * hook for testing kernel call times.
> + */
> + return -EINVAL;
> + }
> + if (msg->msg_controllen != sizeof(control)) {
> + result = -EINVAL;
> + goto done;
Then you copied an uninitialized control into userspace ...
Is goto really necessary ? Maybe just return.
> + }
> + if (unlikely(copy_from_user(&control,
> + (__force void __user *)msg->msg_control,
> + sizeof(control)))) {
> + result = -EFAULT;
> + goto done;
Same as above, is goto really necessary ?
> + }
> + control.completion_cookie = 0;
> +
> + if (control.num_bpages > HOMA_MAX_BPAGES ||
> + (control.flags & ~HOMA_RECVMSG_VALID_FLAGS)) {
> + result = -EINVAL;
> + goto done;
> + }
> + homa_pool_release_buffers(hsk->buffer_pool, control.num_bpages,
> + control.bpage_offsets);
homa_pool_release_buffers() quietly ignores erroneous bpage_index values passed in from the
userspace. This behavior may obscure more complex issues in userspace, and exposure this problem
could help users identify issues earlier.
> + control.num_bpages = 0;
> +
> + rpc = homa_wait_for_message(hsk, (flags & MSG_DONTWAIT)
> + ? (control.flags | HOMA_RECVMSG_NONBLOCKING)
> + : control.flags, control.id);
> + if (IS_ERR(rpc)) {
> + /* If we get here, it means there was an error that prevented
> + * us from finding an RPC to return. If there's an error in
> + * the RPC itself we won't get here.
> + */
> + result = PTR_ERR(rpc);
> + goto done;
> + }
> + result = rpc->error ? rpc->error : rpc->msgin.length;
A trivial tips.
result = rpc->error ?: rpc->msgin.length;
> +
> + /* Collect result information. */
> + control.id = rpc->id;
> + control.completion_cookie = rpc->completion_cookie;
> + if (likely(rpc->msgin.length >= 0)) {
> + control.num_bpages = rpc->msgin.num_bpages;
> + memcpy(control.bpage_offsets, rpc->msgin.bpage_offsets,
> + sizeof(control.bpage_offsets));
A trivial tips.
Although sizeof(control.bpage_offsets) and sizeof(rpc->msgin.bpage_offsets) are the same, but
passing sizeof(rpc->msgin.bpage_offsets) would be more appropriate.
> + }
> + if (sk->sk_family == AF_INET6) {
> + struct sockaddr_in6 *in6 = msg->msg_name;
> +
> + in6->sin6_family = AF_INET6;
> + in6->sin6_port = htons(rpc->dport);
> + in6->sin6_addr = rpc->peer->addr;
> + *addr_len = sizeof(*in6);
> + } else {
> + struct sockaddr_in *in4 = msg->msg_name;
> +
> + in4->sin_family = AF_INET;
> + in4->sin_port = htons(rpc->dport);
> + in4->sin_addr.s_addr = ipv6_to_ipv4(rpc->peer->addr);
> + *addr_len = sizeof(*in4);
> + }
> +
> + /* This indicates that the application now owns the buffers, so
> + * we won't free them in homa_rpc_free.
> + */
> + rpc->msgin.num_bpages = 0;
> +
> + /* Must release the RPC lock (and potentially free the RPC) before
> + * copying the results back to user space.
> + */
> + if (homa_is_client(rpc->id)) {
> + homa_peer_add_ack(rpc);
> + homa_rpc_free(rpc);
> + } else {
> + if (result < 0)
> + homa_rpc_free(rpc);
> + else
> + rpc->state = RPC_IN_SERVICE;
> + }
> + homa_rpc_unlock(rpc);
> +
> +done:
> + if (unlikely(copy_to_user((__force void __user *)msg->msg_control,
> + &control, sizeof(control)))) {
> + /* Note: in this case the message's buffers will be leaked. */
> + pr_notice("%s couldn't copy back args\n", __func__); > + result = -EFAULT;
> + }
> +
> + return result;
> +}
> +
> +/**
> + * homa_hash() - Not needed for Homa.
> + * @sk: Socket for the operation
> + * Return: ??
> + */
> +int homa_hash(struct sock *sk)
> +{
> + return 0;
> +}
> +
> +/**
> + * homa_unhash() - Not needed for Homa.
> + * @sk: Socket for the operation
> + */
> +void homa_unhash(struct sock *sk)
> +{
> +}
> +
> +/**
> + * homa_get_port() - It appears that this function is called to assign a
> + * default port for a socket.
> + * @sk: Socket for the operation
> + * @snum: Unclear what this is.
> + * Return: Zero for success, or a negative errno for an error.
> + */
> +int homa_get_port(struct sock *sk, unsigned short snum)
> +{
> + /* Homa always assigns ports immediately when a socket is created,
> + * so there is nothing to do here.
> + */
> + return 0;
> +}
> +
> +/**
> + * homa_softirq() - This function is invoked at SoftIRQ level to handle
> + * incoming packets.
> + * @skb: The incoming packet.
> + * Return: Always 0
> + */
> +int homa_softirq(struct sk_buff *skb)
> +{
> + struct sk_buff *packets, *other_pkts, *next;
> + struct sk_buff **prev_link, **other_link;
> + struct common_header *h;
> + int first_packet = 1;
> + int header_offset;
> + int pull_length;
> +
> + /* skb may actually contain many distinct packets, linked through
> + * skb_shinfo(skb)->frag_list by the Homa GRO mechanism. Make a
> + * pass through the list to process all of the short packets,
> + * leaving the longer packets in the list. Also, perform various
> + * prep/cleanup/error checking functions.
> + */
> + skb->next = skb_shinfo(skb)->frag_list;
> + skb_shinfo(skb)->frag_list = NULL;
> + packets = skb;
> + prev_link = &packets;
> + for (skb = packets; skb; skb = next) {
> + next = skb->next;
> +
> + /* Make the header available at skb->data, even if the packet
> + * is fragmented. One complication: it's possible that the IP
> + * header hasn't yet been removed (this happens for GRO packets
> + * on the frag_list, since they aren't handled explicitly by IP.
> + */
> + header_offset = skb_transport_header(skb) - skb->data;
> + pull_length = HOMA_MAX_HEADER + header_offset;
> + if (pull_length > skb->len)
> + pull_length = skb->len;
> + if (!pskb_may_pull(skb, pull_length))
> + goto discard;
> + if (header_offset)
> + __skb_pull(skb, header_offset);
> +
> + /* Reject packets that are too short or have bogus types. */
> + h = (struct common_header *)skb->data;
> + if (unlikely(skb->len < sizeof(struct common_header) ||
> + h->type < DATA || h->type >= BOGUS ||
> + skb->len < header_lengths[h->type - DATA]))
> + goto discard;
> +
> + if (first_packet)
> + first_packet = 0;
> +
> + /* Process the packet now if it is a control packet or
> + * if it contains an entire short message.
> + */
> + if (h->type != DATA || ntohl(((struct data_header *)h)
> + ->message_length) < 1400) {
> + *prev_link = skb->next;
> + skb->next = NULL;
> + homa_dispatch_pkts(skb, homa);
> + } else {
> + prev_link = &skb->next;
> + }
> + continue;
> +
> +discard:
> + *prev_link = skb->next;
> + kfree_skb(skb);
> + }
> +
> + /* Now process the longer packets. Each iteration of this loop
> + * collects all of the packets for a particular RPC and dispatches
> + * them.
> + */
> + while (packets) {
> + struct in6_addr saddr, saddr2;
> + struct common_header *h2;
> + struct sk_buff *skb2;
> +
> + skb = packets;
> + prev_link = &skb->next;
> + saddr = skb_canonical_ipv6_saddr(skb);
> + other_pkts = NULL;
> + other_link = &other_pkts;
> + h = (struct common_header *)skb->data;
> + for (skb2 = skb->next; skb2; skb2 = next) {
> + next = skb2->next;
> + h2 = (struct common_header *)skb2->data;
> + if (h2->sender_id == h->sender_id) {
> + saddr2 = skb_canonical_ipv6_saddr(skb2);
> + if (ipv6_addr_equal(&saddr, &saddr2)) {
> + *prev_link = skb2;
> + prev_link = &skb2->next;
> + continue;
> + }
> + }
> + *other_link = skb2;
> + other_link = &skb2->next;
> + }
> + *prev_link = NULL;
> + *other_link = NULL;
> + homa_dispatch_pkts(packets, homa);
> + packets = other_pkts;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * homa_backlog_rcv() - Invoked to handle packets saved on a socket's
> + * backlog because it was locked when the packets first arrived.
> + * @sk: Homa socket that owns the packet's destination port.
> + * @skb: The incoming packet. This function takes ownership of the packet
> + * (we'll delete it).
> + *
> + * Return: Always returns 0.
> + */
> +int homa_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> +{
> + pr_warn("unimplemented backlog_rcv invoked on Homa socket\n");
> + kfree_skb(skb);
> + return 0;
> +}
> +
> +/**
> + * homa_err_handler_v4() - Invoked by IP to handle an incoming error
> + * packet, such as ICMP UNREACHABLE.
> + * @skb: The incoming packet.
> + * @info: Information about the error that occurred?
> + *
> + * Return: zero, or a negative errno if the error couldn't be handled here.
> + */
> +int homa_err_handler_v4(struct sk_buff *skb, u32 info)
> +{
> + const struct in6_addr saddr = skb_canonical_ipv6_saddr(skb);
> + const struct iphdr *iph = ip_hdr(skb);
> + int type = icmp_hdr(skb)->type;
> + int code = icmp_hdr(skb)->code;
> +
> + if (type == ICMP_DEST_UNREACH && code == ICMP_PORT_UNREACH) {
> + char *icmp = (char *)icmp_hdr(skb);
> + struct common_header *h;
> +
> + iph = (struct iphdr *)(icmp + sizeof(struct icmphdr));
> + h = (struct common_header *)(icmp + sizeof(struct icmphdr)
> + + iph->ihl * 4);
> + homa_abort_rpcs(homa, &saddr, ntohs(h->dport), -ENOTCONN);
> + } else if (type == ICMP_DEST_UNREACH) {
> + int error;
> +
> + if (code == ICMP_PROT_UNREACH)
> + error = -EPROTONOSUPPORT;
> + else
> + error = -EHOSTUNREACH;
> + homa_abort_rpcs(homa, &saddr, 0, error);
> + } else {
> + pr_notice("%s invoked with info %x, ICMP type %d, ICMP code %d\n",
> + __func__, info, type, code);
> + }
> + return 0;
> +}
> +
> +/**
> + * homa_err_handler_v6() - Invoked by IP to handle an incoming error
> + * packet, such as ICMP UNREACHABLE.
> + * @skb: The incoming packet.
> + * @opt: Not used.
> + * @type: Type of ICMP packet.
> + * @code: Additional information about the error.
> + * @offset: Not used.
> + * @info: Information about the error that occurred?
> + *
> + * Return: zero, or a negative errno if the error couldn't be handled here.
> + */
> +int homa_err_handler_v6(struct sk_buff *skb, struct inet6_skb_parm *opt,
> + u8 type, u8 code, int offset, __be32 info)
> +{
> + const struct ipv6hdr *iph = (const struct ipv6hdr *)skb->data;
> +
> + if (type == ICMPV6_DEST_UNREACH && code == ICMPV6_PORT_UNREACH) {
> + char *icmp = (char *)icmp_hdr(skb);
> + struct common_header *h;
> +
> + iph = (struct ipv6hdr *)(icmp + sizeof(struct icmphdr));
> + h = (struct common_header *)(icmp + sizeof(struct icmphdr)
> + + HOMA_IPV6_HEADER_LENGTH);
> + homa_abort_rpcs(homa, &iph->daddr, ntohs(h->dport), -ENOTCONN);
> + } else if (type == ICMPV6_DEST_UNREACH) {
> + int error;
> +
> + if (code == ICMP_PROT_UNREACH)
> + error = -EPROTONOSUPPORT;
> + else
> + error = -EHOSTUNREACH;
> + homa_abort_rpcs(homa, &iph->daddr, 0, error);
> + }
> + return 0;
> +}
> +
> +/**
> + * homa_poll() - Invoked by Linux as part of implementing select, poll,
> + * epoll, etc.
> + * @file: Open file that is participating in a poll, select, etc.
> + * @sock: A Homa socket, associated with @file.
> + * @wait: This table will be registered with the socket, so that it
> + * is notified when the socket's ready state changes.
> + *
> + * Return: A mask of bits such as EPOLLIN, which indicate the current
> + * state of the socket.
> + */
> +__poll_t homa_poll(struct file *file, struct socket *sock,
> + struct poll_table_struct *wait)
> +{
> + struct sock *sk = sock->sk;
> + __u32 mask;
> +
> + /* It seems to be standard practice for poll functions *not* to
> + * acquire the socket lock, so we don't do it here; not sure
> + * why...
> + */
> +
> + sock_poll_wait(file, sock, wait);
> + mask = POLLOUT | POLLWRNORM;
> +
> + if (!list_empty(&homa_sk(sk)->ready_requests) ||
> + !list_empty(&homa_sk(sk)->ready_responses))
> + mask |= POLLIN | POLLRDNORM;
> + return (__force __poll_t)mask;
> +}
> +
> +/**
> + * homa_hrtimer() - This function is invoked by the hrtimer mechanism to
> + * wake up the timer thread. Runs at IRQ level.
> + * @timer: The timer that triggered; not used.
> + *
> + * Return: Always HRTIMER_RESTART.
> + */
> +enum hrtimer_restart homa_hrtimer(struct hrtimer *timer)
> +{
> + wake_up_process(timer_kthread);
> + return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * homa_timer_main() - Top-level function for the timer thread.
> + * @transport: Pointer to struct homa.
> + *
> + * Return: Always 0.
> + */
> +int homa_timer_main(void *transport)
> +{
> + struct homa *homa = (struct homa *)transport;
> + struct hrtimer hrtimer;
> + ktime_t tick_interval;
> + u64 nsec;
> +
> + hrtimer_init(&hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer.function = &homa_hrtimer;
> + nsec = 1000000; /* 1 ms */
> + tick_interval = ns_to_ktime(nsec);
> + while (1) {
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + if (!exiting) {
> + hrtimer_start(&hrtimer, tick_interval, HRTIMER_MODE_REL);
> + schedule();
> + }
> + __set_current_state(TASK_RUNNING);
> + if (exiting)
> + break;
> + homa_timer(homa);
> + }
> + hrtimer_cancel(&hrtimer);
> + kthread_complete_and_exit(&timer_thread_done, 0);
> + return 0;
> +}
> diff --git a/net/homa/homa_utils.c b/net/homa/homa_utils.c
> new file mode 100644
> index 000000000000..905d00c836bd
> --- /dev/null
> +++ b/net/homa/homa_utils.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +
> +/* This file contains miscellaneous utility functions for Homa, such
> + * as initializing and destroying homa structs.
> + */
> +
> +#include "homa_impl.h"
> +#include "homa_peer.h"
> +#include "homa_rpc.h"
> +#include "homa_stub.h"
> +
> +struct completion homa_pacer_kthread_done;
> +
> +/**
> + * homa_init() - Constructor for homa objects.
> + * @homa: Object to initialize.
> + *
> + * Return: 0 on success, or a negative errno if there was an error. Even
> + * if an error occurs, it is safe (and necessary) to call
> + * homa_destroy at some point.
> + */
> +int homa_init(struct homa *homa)
> +{
> + int err;
> +
> + _Static_assert(HOMA_MAX_PRIORITIES >= 8,
> + "homa_init assumes at least 8 priority levels");
> +
> + homa->pacer_kthread = NULL;
> + init_completion(&homa_pacer_kthread_done);
> + atomic64_set(&homa->next_outgoing_id, 2);
> + atomic64_set(&homa->link_idle_time, sched_clock());
> + spin_lock_init(&homa->pacer_mutex);
> + homa->pacer_fifo_fraction = 50;
> + homa->pacer_fifo_count = 1;
> + homa->pacer_wake_time = 0;
> + spin_lock_init(&homa->throttle_lock);
> + INIT_LIST_HEAD_RCU(&homa->throttled_rpcs);
> + homa->throttle_add = 0;
> + homa->throttle_min_bytes = 200;
> + homa->next_client_port = HOMA_MIN_DEFAULT_PORT;
> + homa->port_map = kmalloc(sizeof(*homa->port_map), GFP_KERNEL);
> + if (!homa->port_map) {
> + pr_err("%s couldn't create port_map: kmalloc failure", __func__);
> + return -ENOMEM;
> + }
> + homa_socktab_init(homa->port_map);
> + homa->peers = kmalloc(sizeof(*homa->peers), GFP_KERNEL);
> + if (!homa->peers) {
> + pr_err("%s couldn't create peers: kmalloc failure", __func__);
> + return -ENOMEM;
> + }
> + err = homa_peertab_init(homa->peers);
> + if (err) {
> + pr_err("%s couldn't initialize peer table (errno %d)\n",
> + __func__, -err);
> + return err;
> + }
> +
> + /* Wild guesses to initialize configuration values... */
> + homa->unsched_bytes = 40000;
> + homa->window_param = 100000;
> + homa->link_mbps = 25000;
> + homa->fifo_grant_increment = 10000;
> + homa->grant_fifo_fraction = 50;
> + homa->max_overcommit = 8;
> + homa->max_incoming = 400000;
> + homa->max_rpcs_per_peer = 1;
> + homa->resend_ticks = 5;
> + homa->resend_interval = 5;
> + homa->timeout_ticks = 100;
> + homa->timeout_resends = 5;
> + homa->request_ack_ticks = 2;
> + homa->reap_limit = 10;
> + homa->dead_buffs_limit = 5000;
> + homa->max_dead_buffs = 0;
> + homa->pacer_kthread = kthread_run(homa_pacer_main, homa,
> + "homa_pacer");
> + if (IS_ERR(homa->pacer_kthread)) {
> + err = PTR_ERR(homa->pacer_kthread);
> + homa->pacer_kthread = NULL;
> + pr_err("couldn't create homa pacer thread: error %d\n", err);
> + return err;
> + }
> + homa->pacer_exit = false;
> + homa->max_nic_queue_ns = 5000;
> + homa->ns_per_mbyte = 0;
> + homa->max_gso_size = 10000;
> + homa->gso_force_software = 0;
> + homa->max_gro_skbs = 20;
> + homa->gro_policy = HOMA_GRO_NORMAL;
> + homa->timer_ticks = 0;
> + homa->flags = 0;
> + homa->bpage_lease_usecs = 10000;
> + homa->next_id = 0;
> + homa_outgoing_sysctl_changed(homa);
> + homa_incoming_sysctl_changed(homa);
> + return 0;
> +}
> +
> +/**
> + * homa_destroy() - Destructor for homa objects.
> + * @homa: Object to destroy.
> + */
> +void homa_destroy(struct homa *homa)
> +{
> + if (homa->pacer_kthread) {
> + homa_pacer_stop(homa);
> + wait_for_completion(&homa_pacer_kthread_done);
> + }
> +
> + /* The order of the following statements matters! */
> + if (homa->port_map) {
> + homa_socktab_destroy(homa->port_map);
> + kfree(homa->port_map);
> + homa->port_map = NULL;
> + }
> + if (homa->peers) {
> + homa_peertab_destroy(homa->peers);
> + kfree(homa->peers);
> + homa->peers = NULL;
> + }
> +}
> +
> +/**
> + * homa_spin() - Delay (without sleeping) for a given time interval.
> + * @ns: How long to delay (in nanoseconds)
> + */
> +void homa_spin(int ns)
> +{
> + __u64 end;
> +
> + end = sched_clock() + ns;
> + while (sched_clock() < end)
> + /* Empty loop body.*/
> + ;
> +}
> +
> +/**
> + * homa_throttle_lock_slow() - This function implements the slow path for
> + * acquiring the throttle lock. It is invoked when the lock isn't immediately
> + * available. It waits for the lock, but also records statistics about
> + * the waiting time.
> + * @homa: Overall data about the Homa protocol implementation.
> + */
> +void homa_throttle_lock_slow(struct homa *homa)
> + __acquires(&homa->throttle_lock)
> +{
> + spin_lock_bh(&homa->throttle_lock);
> +}
^ permalink raw reply
* Re: [PATCH RFT v12 4/8] fork: Add shadow stack support to clone3()
From: Yury Khrustalev @ 2024-11-29 16:57 UTC (permalink / raw)
To: Mark Brown
Cc: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan,
linux-kernel, Catalin Marinas, Will Deacon, jannh, Wilco Dijkstra,
linux-kselftest, linux-api, Kees Cook
In-Reply-To: <20241031-clone3-shadow-stack-v12-4-7183eb8bee17@kernel.org>
On Thu, Oct 31, 2024 at 07:25:05PM +0000, Mark Brown wrote:
> Unlike with the normal stack there is no API for configuring the the shadow
> stack for a new thread, instead the kernel will dynamically allocate a new
> shadow stack with the same size as the normal stack. This appears to be due
> to the shadow stack series having been in development since before the more
> extensible clone3() was added rather than anything more deliberate.
>
> Add a paramter to clone3() specifying the shadow stack pointer to use
> for the new thread, this is inconsistent with the way we specify the
> normal stack but during review concerns were expressed about having to
> identify where the shadow stack pointer should be placed especially in
> cases where the shadow stack has been previously active. If no shadow
> stack is specified then the existing implicit allocation behaviour is
> maintained.
>
> If a shadow stack pointer is specified then it is required to have an
> architecture defined token placed on the stack, this will be consumed by
> the new task. If no valid token is present then this will be reported
> with -EINVAL. This token prevents new threads being created pointing at
> the shadow stack of an existing running thread.
>
> If the architecture does not support shadow stacks the shadow stack
> pointer must be not be specified, architectures that do support the
> feature are expected to enforce the same requirement on individual
> systems that lack shadow stack support.
>
> Update the existing arm64 and x86 implementations to pay attention to
> the newly added arguments, in order to maintain compatibility we use the
> existing behaviour if no shadow stack is specified. Since we are now
> using more fields from the kernel_clone_args we pass that into the
> shadow stack code rather than individual fields.
>
> Portions of the x86 architecture code were written by Rick Edgecombe.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
> @@ -101,12 +103,14 @@ struct clone_args {
> __aligned_u64 set_tid;
> __aligned_u64 set_tid_size;
> __aligned_u64 cgroup;
> + __aligned_u64 shadow_stack_pointer;
> };
> #endif
>
> -#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
> -#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
> -#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
> +#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
> +#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
> +#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
> +#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
Acked.
Kind regards,
Yury
^ permalink raw reply
* Re: [PATCH RFT v12 2/8] Documentation: userspace-api: Add shadow stack API documentation
From: Yury Khrustalev @ 2024-11-29 16:18 UTC (permalink / raw)
To: Mark Brown
Cc: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan,
linux-kernel, Catalin Marinas, Will Deacon, jannh, Wilco Dijkstra,
linux-kselftest, linux-api, Kees Cook, Shuah Khan
In-Reply-To: <20241031-clone3-shadow-stack-v12-2-7183eb8bee17@kernel.org>
On Thu, Oct 31, 2024 at 07:25:03PM +0000, Mark Brown wrote:
> There are a number of architectures with shadow stack features which we are
> presenting to userspace with as consistent an API as we can (though there
> are some architecture specifics). Especially given that there are some
> important considerations for userspace code interacting directly with the
> feature let's provide some documentation covering the common aspects.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Tested-by: Kees Cook <kees@kernel.org>
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> Documentation/userspace-api/index.rst | 1 +
> Documentation/userspace-api/shadow_stack.rst | 42 ++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+)
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Roberto Sassu @ 2024-11-29 8:30 UTC (permalink / raw)
To: Luis Chamberlain, mmaurer, samitolvanen, KP Singh
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
da.gomez, akpm, paul, jmorris, serge, shuah, mcoquelin.stm32,
alexandre.torgue, linux-integrity, linux-doc, linux-kernel,
linux-api, linux-modules, linux-security-module, linux-kselftest,
wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai, jannh, dhowells,
jikos, mkoutny, ppavlu, petr.vorel, mzerqung, kgold,
Roberto Sassu
In-Reply-To: <Z0jVQ8Q7AT_9NodI@bombadil.infradead.org>
On Thu, 2024-11-28 at 12:40 -0800, Luis Chamberlain wrote:
> On Thu, Nov 28, 2024 at 09:23:57AM +0100, Roberto Sassu wrote:
> > On Wed, 2024-11-27 at 11:53 -0800, Luis Chamberlain wrote:
> > > On Wed, Nov 27, 2024 at 10:51:11AM +0100, Roberto Sassu wrote:
> > > > For eBPF programs we are also in a need for a better way to
> > > > measure/appraise them.
> > >
> > > I am confused now, I was under the impression this "Integrity Digest
> > > Cache" is just a special thing for LSMs, and so I was under the
> > > impression that kernel_read_file() lsm hook already would take care
> > > of eBPF programs.
> >
> > Yes, the problem is that eBPF programs are transformed in user space
> > before they are sent to the kernel:
> >
> > https://lwn.net/Articles/977394/
>
> That issue seems to be orthogonal to your eandeavor though, which just
> supplements LSMS, right?
Yes, correct, the Integrity Digest Cache would be used to search
whatever digest was calculated by LSMs.
Thanks
Roberto
> Anyway, in case this helps:
>
> The Rust folks faced some slighty related challenges with our CRC
> validations for symbols, our CRC are slapped on with genksyms but this
> relies on the source code and with Rust the compiler may do final
> touches to data. And so DWARF is being used [1].
>
> Although I am not sure of the state of eBPF DWARF support, there is also
> BTF support [0] and most distros are relying on it to make live introspection
> easier, and the output is much smaller. So could DWARF or BTF information
> from eBPF programs be used by the verifier in similar way to verify eBPF
> programs?
>
> Note that to support BTF implicates DWARF and the leap of faith for Rust
> modversions support is that most distros will support DWARF, and so BTF
> can become the norm [2].
>
> [0] https://www.kernel.org/doc/html/latest/bpf/btf.html
> [1] https://lwn.net/Articles/986892/
> [2] https://lwn.net/Articles/991719/
>
> Luis
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Luis Chamberlain @ 2024-11-28 20:40 UTC (permalink / raw)
To: Roberto Sassu, mmaurer, samitolvanen, KP Singh
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <10c8fd4b53f946c2d7e933a35c6eb36557e8c592.camel@huaweicloud.com>
On Thu, Nov 28, 2024 at 09:23:57AM +0100, Roberto Sassu wrote:
> On Wed, 2024-11-27 at 11:53 -0800, Luis Chamberlain wrote:
> > On Wed, Nov 27, 2024 at 10:51:11AM +0100, Roberto Sassu wrote:
> > > For eBPF programs we are also in a need for a better way to
> > > measure/appraise them.
> >
> > I am confused now, I was under the impression this "Integrity Digest
> > Cache" is just a special thing for LSMs, and so I was under the
> > impression that kernel_read_file() lsm hook already would take care
> > of eBPF programs.
>
> Yes, the problem is that eBPF programs are transformed in user space
> before they are sent to the kernel:
>
> https://lwn.net/Articles/977394/
That issue seems to be orthogonal to your eandeavor though, which just
supplements LSMS, right?
Anyway, in case this helps:
The Rust folks faced some slighty related challenges with our CRC
validations for symbols, our CRC are slapped on with genksyms but this
relies on the source code and with Rust the compiler may do final
touches to data. And so DWARF is being used [1].
Although I am not sure of the state of eBPF DWARF support, there is also
BTF support [0] and most distros are relying on it to make live introspection
easier, and the output is much smaller. So could DWARF or BTF information
from eBPF programs be used by the verifier in similar way to verify eBPF
programs?
Note that to support BTF implicates DWARF and the leap of faith for Rust
modversions support is that most distros will support DWARF, and so BTF
can become the norm [2].
[0] https://www.kernel.org/doc/html/latest/bpf/btf.html
[1] https://lwn.net/Articles/986892/
[2] https://lwn.net/Articles/991719/
Luis
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Roberto Sassu @ 2024-11-28 8:23 UTC (permalink / raw)
To: Luis Chamberlain
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <Z0d4vXuCqjTo_QW1@bombadil.infradead.org>
On Wed, 2024-11-27 at 11:53 -0800, Luis Chamberlain wrote:
> On Wed, Nov 27, 2024 at 10:51:11AM +0100, Roberto Sassu wrote:
> > For eBPF programs we are also in a need for a better way to
> > measure/appraise them.
>
> I am confused now, I was under the impression this "Integrity Digest
> Cache" is just a special thing for LSMs, and so I was under the
> impression that kernel_read_file() lsm hook already would take care
> of eBPF programs.
Yes, the problem is that eBPF programs are transformed in user space
before they are sent to the kernel:
https://lwn.net/Articles/977394/
The Integrity Digest Cache can be used for the measurement/appraisal of
the initial eBPF ELF file, when they are accessed from the filesystem,
but the resulting blob sent to the kernel will be different.
> > Now, I'm trying to follow you on the additional kernel_read_file()
> > calls. I agree with you, if a parser tries to open again the file that
> > is being verified it would cause a deadlock in IMA (since the inode
> > mutex is already locked for verifying the original file).
>
> Just document this on the parser as a requirement.
Ok, will do.
> > > > Supporting kernel modules opened the road for new deadlocks, since one
> > > > can ask a digest list to verify a kernel module, but that digest list
> > > > requires the same kernel module. That is why the in-kernel mechanism is
> > > > 100% reliable,
> > >
> > > Are users of this infrastructure really in need of modules for these
> > > parsers?
> >
> > I planned to postpone this to later, and introduced two parsers built-
> > in (TLV and RPM). However, due to Linus's concern regarding the RPM
> > parser, I moved it out in a kernel module.
>
> OK this should be part of the commit log, ie that it is not desirable to
> have an rpm parser in-kernel for some users.
I understand. Will add in the commit log.
Just to clarify, we are not talking about the full blown librpm in the
kernel, but a 243 LOC that I rewrote to obtain only the information I
need. I also formally verified it with pseudo/totally random data with
Frama-C:
https://github.com/robertosassu/rpm-formal/blob/main/validate_rpm.c
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Luis Chamberlain @ 2024-11-27 19:53 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <3dc25195b0362b3e5b6d6964df021ff4e7e1b226.camel@huaweicloud.com>
On Wed, Nov 27, 2024 at 10:51:11AM +0100, Roberto Sassu wrote:
> For eBPF programs we are also in a need for a better way to
> measure/appraise them.
I am confused now, I was under the impression this "Integrity Digest
Cache" is just a special thing for LSMs, and so I was under the
impression that kernel_read_file() lsm hook already would take care
of eBPF programs.
> Now, I'm trying to follow you on the additional kernel_read_file()
> calls. I agree with you, if a parser tries to open again the file that
> is being verified it would cause a deadlock in IMA (since the inode
> mutex is already locked for verifying the original file).
Just document this on the parser as a requirement.
> > > Supporting kernel modules opened the road for new deadlocks, since one
> > > can ask a digest list to verify a kernel module, but that digest list
> > > requires the same kernel module. That is why the in-kernel mechanism is
> > > 100% reliable,
> >
> > Are users of this infrastructure really in need of modules for these
> > parsers?
>
> I planned to postpone this to later, and introduced two parsers built-
> in (TLV and RPM). However, due to Linus's concern regarding the RPM
> parser, I moved it out in a kernel module.
OK this should be part of the commit log, ie that it is not desirable to
have an rpm parser in-kernel for some users.
Luis
^ permalink raw reply
* Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
From: Roberto Sassu @ 2024-11-27 17:56 UTC (permalink / raw)
To: Dr. Greg
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, mcgrof, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <20241127173042.GA1649@wind.enjellic.com>
On Wed, 2024-11-27 at 11:30 -0600, Dr. Greg wrote:
> On Tue, Nov 19, 2024 at 11:49:07AM +0100, Roberto Sassu wrote:
>
> Hi Roberto, I hope the week is going well for you.
>
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Integrity detection and protection has long been a desirable feature, to
> > reach a large user base and mitigate the risk of flaws in the software
> > and attacks.
> >
> > However, while solutions exist, they struggle to reach a large user base,
> > due to requiring higher than desired constraints on performance,
> > flexibility and configurability, that only security conscious people are
> > willing to accept.
> >
> > For example, IMA measurement requires the target platform to collect
> > integrity measurements, and to protect them with the TPM, which introduces
> > a noticeable overhead (up to 10x slower in a microbenchmark) on frequently
> > used system calls, like the open().
> >
> > IMA Appraisal currently requires individual files to be signed and
> > verified, and Linux distributions to rebuild all packages to include file
> > signatures (this approach has been adopted from Fedora 39+). Like a TPM,
> > also signature verification introduces a significant overhead, especially
> > if it is used to check the integrity of many files.
> >
> > This is where the new Integrity Digest Cache comes into play, it offers
> > additional support for new and existing integrity solutions, to make
> > them faster and easier to deploy.
> >
> > The Integrity Digest Cache can help IMA to reduce the number of TPM
> > operations and to make them happen in a deterministic way. If IMA knows
> > that a file comes from a Linux distribution, it can measure files in a
> > different way: measure the list of digests coming from the distribution
> > (e.g. RPM package headers), and subsequently measure a file if it is not
> > found in that list.
> >
> > The performance improvement comes at the cost of IMA not reporting which
> > files from installed packages were accessed, and in which temporal
> > sequence. This approach might not be suitable for all use cases.
> >
> > The Integrity Digest Cache can also help IMA for appraisal. IMA can simply
> > lookup the calculated digest of an accessed file in the list of digests
> > extracted from package headers, after verifying the header signature. It is
> > sufficient to verify only one signature for all files in the package, as
> > opposed to verifying a signature for each file.
>
> Roberto, a big picture question for you, our apologies if we
> completely misunderstand your patch series.
Hi Greg
no need to apologise, happy to answer your questions.
> The performance benefit comes from the fact that the kernel doesn't
> have to read a file and calculate the cryptographic digest when the
> file is accessed. The 'trusted' digest value comes from a signed list
> of digests that a packaging entity provides and the kernel validates.
> So there is an integrity guarantee that the supplied digests were the
> same as when the package was built.
The performance benefit (for appraisal with my benchmark: 65% with
sequential file access and 43% with parallel file access) comes from
verifying the ECDSA signature of 303 digest lists, as opposed to the
ECDSA signature of 12312 files.
The additional performance boost due to switching from file data digest
to fsverity digests is on top of that.
> Is there a guarantee implemented, that we missed, that the on-disk
> file actually has the digest value that was initially generated by the
> packaging entity when the file is accessed operationally?
Yes, the guarantee is provided by IMA by measuring the actual file
digest and searching it in a digest cache. The integration in IMA of
the Integrity Digest Cache is done in a separate patch set:
https://lore.kernel.org/linux-security-module/20241119110103.2780453-1-roberto.sassu@huaweicloud.com/
The integrity evaluation result is invalidated when the file is
modified, or when the digest list used to verify the file is modified
too.
For fsverity, the guarantee similarly comes from searching the fsverity
digest in a digest cache, but as opposed of IMA the integrity
evaluation result does not need to be invalidated for a file write,
since fsverity-protected files are accessible only in read-only mode.
However, the result still needs to be invalidated if the digest list
changes.
> Secondly, and in a related issue, what happens in a container
> environment when a pathname is accessed that is actually a different
> file but with the same effective pathname as a file that is in the
> vendor validated digest list?
At the moment nothing, only the file data are evaluated. Currently, the
Integrity Digest Cache does not store the pathnames associated to a
digest. It can be done as an extension, if desired, and the pathnames
can be compared.
Roberto
> Once again, apologies, if we completely misinterpret the issues
> involved.
>
> Have a good remainder of the week.
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity
> https://github.com/Quixote-Project
^ permalink raw reply
* Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
From: Dr. Greg @ 2024-11-27 17:30 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, mcgrof, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <20241119104922.2772571-1-roberto.sassu@huaweicloud.com>
On Tue, Nov 19, 2024 at 11:49:07AM +0100, Roberto Sassu wrote:
Hi Roberto, I hope the week is going well for you.
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Integrity detection and protection has long been a desirable feature, to
> reach a large user base and mitigate the risk of flaws in the software
> and attacks.
>
> However, while solutions exist, they struggle to reach a large user base,
> due to requiring higher than desired constraints on performance,
> flexibility and configurability, that only security conscious people are
> willing to accept.
>
> For example, IMA measurement requires the target platform to collect
> integrity measurements, and to protect them with the TPM, which introduces
> a noticeable overhead (up to 10x slower in a microbenchmark) on frequently
> used system calls, like the open().
>
> IMA Appraisal currently requires individual files to be signed and
> verified, and Linux distributions to rebuild all packages to include file
> signatures (this approach has been adopted from Fedora 39+). Like a TPM,
> also signature verification introduces a significant overhead, especially
> if it is used to check the integrity of many files.
>
> This is where the new Integrity Digest Cache comes into play, it offers
> additional support for new and existing integrity solutions, to make
> them faster and easier to deploy.
>
> The Integrity Digest Cache can help IMA to reduce the number of TPM
> operations and to make them happen in a deterministic way. If IMA knows
> that a file comes from a Linux distribution, it can measure files in a
> different way: measure the list of digests coming from the distribution
> (e.g. RPM package headers), and subsequently measure a file if it is not
> found in that list.
>
> The performance improvement comes at the cost of IMA not reporting which
> files from installed packages were accessed, and in which temporal
> sequence. This approach might not be suitable for all use cases.
>
> The Integrity Digest Cache can also help IMA for appraisal. IMA can simply
> lookup the calculated digest of an accessed file in the list of digests
> extracted from package headers, after verifying the header signature. It is
> sufficient to verify only one signature for all files in the package, as
> opposed to verifying a signature for each file.
Roberto, a big picture question for you, our apologies if we
completely misunderstand your patch series.
The performance benefit comes from the fact that the kernel doesn't
have to read a file and calculate the cryptographic digest when the
file is accessed. The 'trusted' digest value comes from a signed list
of digests that a packaging entity provides and the kernel validates.
So there is an integrity guarantee that the supplied digests were the
same as when the package was built.
Is there a guarantee implemented, that we missed, that the on-disk
file actually has the digest value that was initially generated by the
packaging entity when the file is accessed operationally?
Secondly, and in a related issue, what happens in a container
environment when a pathname is accessed that is actually a different
file but with the same effective pathname as a file that is in the
vendor validated digest list?
Once again, apologies, if we completely misinterpret the issues
involved.
Have a good remainder of the week.
As always,
Dr. Greg
The Quixote Project - Flailing at the Travails of Cybersecurity
https://github.com/Quixote-Project
^ permalink raw reply
* Re: [PATCH v21 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mimi Zohar @ 2024-11-27 15:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241127.Ob8DaeR9xaul@digikod.net>
On Wed, 2024-11-27 at 13:10 +0100, Mickaël Salaün wrote:
> On Tue, Nov 26, 2024 at 12:41:45PM -0500, Mimi Zohar wrote:
> > On Fri, 2024-11-22 at 15:50 +0100, Mickaël Salaün wrote:
> > > On Thu, Nov 21, 2024 at 03:34:47PM -0500, Mimi Zohar wrote:
> > > > Hi Mickaël,
> > > >
> > > > On Tue, 2024-11-12 at 20:18 +0100, Mickaël Salaün wrote:
> > > > >
> > > > > +
> > > > > +/* Returns 1 on error, 0 otherwise. */
> > > > > +static int interpret_stream(FILE *script, char *const script_name,
> > > > > + char *const *const envp, const bool restrict_stream)
> > > > > +{
> > > > > + int err;
> > > > > + char *const script_argv[] = { script_name, NULL };
> > > > > + char buf[128] = {};
> > > > > + size_t buf_size = sizeof(buf);
> > > > > +
> > > > > + /*
> > > > > + * We pass a valid argv and envp to the kernel to emulate a native
> > > > > + * script execution. We must use the script file descriptor instead of
> > > > > + * the script path name to avoid race conditions.
> > > > > + */
> > > > > + err = execveat(fileno(script), "", script_argv, envp,
> > > > > + AT_EMPTY_PATH | AT_EXECVE_CHECK);
> > > >
> > > > At least with v20, the AT_CHECK always was being set, independent of whether
> > > > set-exec.c set it. I'll re-test with v21.
> > >
> > > AT_EXECVE_CEHCK should always be set, only the interpretation of the
> > > result should be relative to securebits. This is highlighted in the
> > > documentation.
> >
> > Sure, that sounds correct. With an IMA-appraisal policy, any unsigned script
> > with the is_check flag set now emits an "cause=IMA-signature-required" audit
> > message. However since IMA-appraisal isn't enforcing file signatures, this
> > sounds wrong.
> >
> > New audit messages like "IMA-signature-required-by-interpreter" and "IMA-
> > signature-not-required-by-interpreter" would need to be defined based on the
> > SECBIT_EXEC_RESTRICT_FILE.
>
> It makes sense. Could you please send a patch for these
> IMA-*-interpreter changes? I'll include it in the next series.
Sent as an RFC. The audit message is only updated for the missing signature
case. However, all of the audit messages in ima_appraise_measurement() should
be updated. The current method doesn't scale.
Mimi
> >
> >
> > > >
> > > > > + if (err && restrict_stream) {
> > > > > + perror("ERROR: Script execution check");
> > > > > + return 1;
> > > > > + }
> > > > > +
> > > > > + /* Reads script. */
> > > > > + buf_size = fread(buf, 1, buf_size - 1, script);
> > > > > + return interpret_buffer(buf, buf_size);
> > > > > +}
> > > > > +
> > > >
> > > >
> > >
> >
> >
>
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-11-27 15:14 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jeff Xu, Al Viro, Christian Brauner, Kees Cook, Paul Moore,
Serge Hallyn, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Linus Torvalds, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Eric Paris, audit
In-Reply-To: <20241127.aizae7eeHohn@digikod.net>
On Wed, Nov 27, 2024 at 4:07 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Mon, Nov 25, 2024 at 09:38:51AM -0800, Jeff Xu wrote:
> > On Fri, Nov 22, 2024 at 6:50 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > On Thu, Nov 21, 2024 at 10:27:40AM -0800, Jeff Xu wrote:
> > > > On Thu, Nov 21, 2024 at 5:40 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > >
> > > > > On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> > > > > > On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > > > > > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > > > > >
> > > > > > > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > > > > > > be allowed for execution. The main use case is for script interpreters
> > > > > > > > > and dynamic linkers to check execution permission according to the
> > > > > > > > > kernel's security policy. Another use case is to add context to access
> > > > > > > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > > > > > > any executable code, scripts could also use this check [1].
> > > > > > > > >
> > > > > > > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > > > > > > access rights (i.e. inode permission and mount options for regular
> > > > > > > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > > > > > > use case for access(2) is for SUID processes to (partially) check access
> > > > > > > > > on behalf of their caller. The main use case for execveat(2) +
> > > > > > > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > > > > > > according to all the different restrictions in place. Because the use
> > > > > > > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > > > > > > execution, user space gets the same error codes.
> > > > > > > > >
> > > > > > > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > > > > > > it decouples the check from the enforcement. Indeed, the security check
> > > > > > > > > can be logged (e.g. with audit) without blocking an execution
> > > > > > > > > environment not yet ready to enforce a strict security policy.
> > > > > > > > >
> > > > > > > > > LSMs can control or log execution requests with
> > > > > > > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > > > > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > > > > > > restrict file executability, or mesure executed files, with
> > > > > > > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > > > > > > >
> > > > > > > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > > > > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > > > > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > > > > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > > > > > > never called when AT_EXECVE_CHECK is used.
> > > > > > > > >
> > > > > > > > > It should be noted that script interpreters cannot directly use
> > > > > > > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > > > > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > > > > > > being executed to interpret the script. Unlike the kernel, script
> > > > > > > > > interpreters may just interpret the shebang as a simple comment, which
> > > > > > > > > should not change for backward compatibility reasons.
> > > > > > > > >
> > > > > > > > > Because scripts or libraries files might not currently have the
> > > > > > > > > executable permission set, or because we might want specific users to be
> > > > > > > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > > > > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > > > > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > > > > > > >
> > > > > > > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > > > > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > > > > > > This patch has been used for more than a decade with customized script
> > > > > > > > > interpreters. Some examples can be found here:
> > > > > > > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > > > > > > >
> > > > > > > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > > > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > > > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > > > > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > > > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes since v20:
> > > > > > > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > > > > > > Serge Hallyn.
> > > > > > > > > * Move the UAPI documentation to a dedicated RST file.
> > > > > > > > > * Add Reviewed-by: Serge Hallyn
> > > > > > > > >
> > > > > > > > > Changes since v19:
> > > > > > > > > * Remove mention of "role transition" as suggested by Andy.
> > > > > > > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > > > > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > > > > > > discussed with Jeff.
> > > > > > > > > * Improve documentation both in UAPI comments and kernel comments
> > > > > > > > > (requested by Kees).
> > > > > > > > >
> > > > > > > > > New design since v18:
> > > > > > > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > > > > > > ---
> > > > > > > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > > > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > > > > > fs/exec.c | 20 +++++++++++--
> > > > > > > > > include/linux/binfmts.h | 7 ++++-
> > > > > > > > > include/uapi/linux/fcntl.h | 4 +++
> > > > > > > > > kernel/audit.h | 1 +
> > > > > > > > > kernel/auditsc.c | 1 +
> > > > > > > > > security/security.c | 10 +++++++
> > > > > > > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > > > > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..ad1aeaa5f6c0
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > > > > > > @@ -0,0 +1,34 @@
> > > > > > > > > +===================
> > > > > > > > > +Executability check
> > > > > > > > > +===================
> > > > > > > > > +
> > > > > > > > > +AT_EXECVE_CHECK
> > > > > > > > > +===============
> > > > > > > > > +
> > > > > > > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > > > > > > +check on a regular file and returns 0 if execution of this file would be
> > > > > > > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > > > > > > +(e.g. ELF libraries, script's shebang).
> > > > > > > > > +
> > > > > > > > > +Programs should always perform this check to apply kernel-level checks against
> > > > > > > > > +files that are not directly executed by the kernel but passed to a user space
> > > > > > > > > +interpreter instead. All files that contain executable code, from the point of
> > > > > > > > > +view of the interpreter, should be checked. However the result of this check
> > > > > > > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > > > > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > > > > > > Regarding "should only"
> > > > > > > > Userspace (e.g. libc) could decide to enforce even when
> > > > > > > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > > > > > > doesn't make sense.
> > > > > > >
> > > > > > > User space is always in control, but I don't think it would be wise to
> > > > > > > not follow the configuration securebits (in a generic system) because
> > > > > > > this could result to unattended behaviors (I don't have a specific one
> > > > > > > in mind but...). That being said, configuration and checks are
> > > > > > > standalones and specific/tailored systems are free to do the checks they
> > > > > > > want.
> > > > > > >
> > > > > > In the case of dynamic linker, we can always enforce honoring the
> > > > > > execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> > > > > > to, the dynamic linker doesn't need to check the
> > > > > > SECBIT_EXEC_RESTRICT_FILE bit.
> > > > >
> > > > > If the library file is not allowed to be executed by *all* access
> > > > > control systems (not just mount and file permission, but all LSMs), then
> > > > > the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
> > > > > requirement.
> > > > Yes. specifically for the library loading case, I can't think of a
> > > > case where we need to by-pass LSMs. (letting user space to by-pass
> > > > LSM check seems questionable in concept, and should only be used when
> > > > there aren't other solutions). In the context of SELINUX enforcing
> > > > mode, we will want to enforce it. In the context of process level LSM
> > > > such as landlock, the process can already decide for itself by
> > > > selecting the policy for its own domain, it is unnecessary to use
> > > > another opt-out solution.
> > >
> > > My answer wasn't clear. The execveat(AT_EXECVE_CHECK) can and should
> > > always be done, but user space should only enforce restrictions
> > > according to the securebits.
> > >
> > I knew this part (AT_EXESCVE_CHECK is called always)
> > Since the securebits are enforced by userspace, setting it to 0 is
> > equivalent to opt-out enforcement, that is what I meant by opt-out.
>
> OK, that was confusing because these bits are set to 0 by default (for
> compatibility reasons).
>
> >
> > > It doesn't make sense to talk about user space "bypassing" kernel
> > > checks. This patch series provides a feature to enable user space to
> > > enforce (at its level) the same checks as the kernel.
> > >
> > > There is no opt-out solution, but compatibility configuration bits
> > > through securebits (which can also be set by LSMs).
> > >
> > > To answer your question about the dynamic linker, there should be no
> > > difference of behavior with a script interpreter. Both should check
> > > executability but only enforce restriction according to the securebits
> > > (as explained in the documentation). Doing otherwise on a generic
> > > distro could lead to unexpected behaviors (e.g. if a user enforced a
> > > specific SELinux policy that doesn't allow execution of library files).
> > >
> > > >
> > > > There is one case where I see a difference:
> > > > ld.so a.out (when a.out is on non-exec mount)
> > > >
> > > > If the dynamic linker doesn't read SECBIT_EXEC_RESTRICT_FILE setting,
> > > > above will always fail. But that is more of a bugfix.
> > >
> > > No, the dynamic linker should only enforce restrictions according to the
> > > securebits, otherwise a user space update (e.g. with a new dynamic
> > > linker ignoring the securebits) could break an existing system.
> > >
> > OK. upgrade is a valid concern. Previously, I was just thinking about
> > a new LSM based on this check, not existing LSM policies.
> > Do you happen to know which SELinux policy/LSM could break ? i.e. it
> > will be applied to libraries once we add AT_EXESCVE_CHECK in the
> > dynamic linker.
>
> We cannot assume anything about LSM policies because of custom and
> private ones.
>
That is a good point.
> > We could give heads up and prepare for that.
> >
> > > >
> > > > >Relying on the securebits to know if this is a hard
> > > > > requirement or not enables system administrator and distros to control
> > > > > this potential behavior change.
> > > > >
> > > > I think, for the dynamic linker, it can be a hard requirement.
> > >
> > > Not on a generic distro.
> > >
> > Ok. Maybe this can be done through a configuration option for the
> > dynamic linker.
>
> Yes, we could have a built-time option (disabled by default) for the
> dynamic linker to enforce that.
>
> >
> > The consideration I have is: securebits is currently designed to
> > control both dynamic linker and shell scripts.
> > The case for dynamic linker is simpler than scripts cases, (non-exec
> > mount, and perhaps some LSM policies for libraries) and distributions
> > such as ChromeOS can enforce the dynamic linker case ahead of scripts
> > interrupter cases, i.e. without waiting for python/shell being
> > upgraded, that can take sometimes.
>
> For secure systems, the end goal is to always enforce such restrictions,
> so once interpretation/execution of a set of file types (e.g. ELF
> libraries) are tested enough in such a system, we can remove the
> securebits checks for the related library/executable (e.g. ld.so) and
> consider that they are always set, independently of the current
> user/credentials.
Great! I agree with that.
Thanks.
^ permalink raw reply
* Re: [PATCH v21 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mickaël Salaün @ 2024-11-27 12:10 UTC (permalink / raw)
To: Mimi Zohar
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <623f89b4de41ac14e0e48e106b846abc9e9d70cf.camel@linux.ibm.com>
On Tue, Nov 26, 2024 at 12:41:45PM -0500, Mimi Zohar wrote:
> On Fri, 2024-11-22 at 15:50 +0100, Mickaël Salaün wrote:
> > On Thu, Nov 21, 2024 at 03:34:47PM -0500, Mimi Zohar wrote:
> > > Hi Mickaël,
> > >
> > > On Tue, 2024-11-12 at 20:18 +0100, Mickaël Salaün wrote:
> > > >
> > > > +
> > > > +/* Returns 1 on error, 0 otherwise. */
> > > > +static int interpret_stream(FILE *script, char *const script_name,
> > > > + char *const *const envp, const bool restrict_stream)
> > > > +{
> > > > + int err;
> > > > + char *const script_argv[] = { script_name, NULL };
> > > > + char buf[128] = {};
> > > > + size_t buf_size = sizeof(buf);
> > > > +
> > > > + /*
> > > > + * We pass a valid argv and envp to the kernel to emulate a native
> > > > + * script execution. We must use the script file descriptor instead of
> > > > + * the script path name to avoid race conditions.
> > > > + */
> > > > + err = execveat(fileno(script), "", script_argv, envp,
> > > > + AT_EMPTY_PATH | AT_EXECVE_CHECK);
> > >
> > > At least with v20, the AT_CHECK always was being set, independent of whether
> > > set-exec.c set it. I'll re-test with v21.
> >
> > AT_EXECVE_CEHCK should always be set, only the interpretation of the
> > result should be relative to securebits. This is highlighted in the
> > documentation.
>
> Sure, that sounds correct. With an IMA-appraisal policy, any unsigned script
> with the is_check flag set now emits an "cause=IMA-signature-required" audit
> message. However since IMA-appraisal isn't enforcing file signatures, this
> sounds wrong.
>
> New audit messages like "IMA-signature-required-by-interpreter" and "IMA-
> signature-not-required-by-interpreter" would need to be defined based on the
> SECBIT_EXEC_RESTRICT_FILE.
It makes sense. Could you please send a patch for these
IMA-*-interpreter changes? I'll include it in the next series.
>
>
> > >
> > > > + if (err && restrict_stream) {
> > > > + perror("ERROR: Script execution check");
> > > > + return 1;
> > > > + }
> > > > +
> > > > + /* Reads script. */
> > > > + buf_size = fread(buf, 1, buf_size - 1, script);
> > > > + return interpret_buffer(buf, buf_size);
> > > > +}
> > > > +
> > >
> > >
> >
>
>
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-11-27 12:07 UTC (permalink / raw)
To: Jeff Xu
Cc: Jeff Xu, Al Viro, Christian Brauner, Kees Cook, Paul Moore,
Serge Hallyn, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Linus Torvalds, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Eric Paris, audit
In-Reply-To: <CALmYWFuYVHHz7aoxk+U=auLLT4xvJdzyOyzQ2u+E0kM3uc_rTw@mail.gmail.com>
On Mon, Nov 25, 2024 at 09:38:51AM -0800, Jeff Xu wrote:
> On Fri, Nov 22, 2024 at 6:50 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > On Thu, Nov 21, 2024 at 10:27:40AM -0800, Jeff Xu wrote:
> > > On Thu, Nov 21, 2024 at 5:40 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > >
> > > > On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> > > > > On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > >
> > > > > > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > > > > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > > > >
> > > > > > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > > > > > be allowed for execution. The main use case is for script interpreters
> > > > > > > > and dynamic linkers to check execution permission according to the
> > > > > > > > kernel's security policy. Another use case is to add context to access
> > > > > > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > > > > > any executable code, scripts could also use this check [1].
> > > > > > > >
> > > > > > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > > > > > access rights (i.e. inode permission and mount options for regular
> > > > > > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > > > > > use case for access(2) is for SUID processes to (partially) check access
> > > > > > > > on behalf of their caller. The main use case for execveat(2) +
> > > > > > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > > > > > according to all the different restrictions in place. Because the use
> > > > > > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > > > > > execution, user space gets the same error codes.
> > > > > > > >
> > > > > > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > > > > > it decouples the check from the enforcement. Indeed, the security check
> > > > > > > > can be logged (e.g. with audit) without blocking an execution
> > > > > > > > environment not yet ready to enforce a strict security policy.
> > > > > > > >
> > > > > > > > LSMs can control or log execution requests with
> > > > > > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > > > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > > > > > restrict file executability, or mesure executed files, with
> > > > > > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > > > > > >
> > > > > > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > > > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > > > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > > > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > > > > > never called when AT_EXECVE_CHECK is used.
> > > > > > > >
> > > > > > > > It should be noted that script interpreters cannot directly use
> > > > > > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > > > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > > > > > being executed to interpret the script. Unlike the kernel, script
> > > > > > > > interpreters may just interpret the shebang as a simple comment, which
> > > > > > > > should not change for backward compatibility reasons.
> > > > > > > >
> > > > > > > > Because scripts or libraries files might not currently have the
> > > > > > > > executable permission set, or because we might want specific users to be
> > > > > > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > > > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > > > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > > > > > >
> > > > > > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > > > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > > > > > This patch has been used for more than a decade with customized script
> > > > > > > > interpreters. Some examples can be found here:
> > > > > > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > > > > > >
> > > > > > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > > > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes since v20:
> > > > > > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > > > > > Serge Hallyn.
> > > > > > > > * Move the UAPI documentation to a dedicated RST file.
> > > > > > > > * Add Reviewed-by: Serge Hallyn
> > > > > > > >
> > > > > > > > Changes since v19:
> > > > > > > > * Remove mention of "role transition" as suggested by Andy.
> > > > > > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > > > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > > > > > discussed with Jeff.
> > > > > > > > * Improve documentation both in UAPI comments and kernel comments
> > > > > > > > (requested by Kees).
> > > > > > > >
> > > > > > > > New design since v18:
> > > > > > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > > > > > ---
> > > > > > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > > > > fs/exec.c | 20 +++++++++++--
> > > > > > > > include/linux/binfmts.h | 7 ++++-
> > > > > > > > include/uapi/linux/fcntl.h | 4 +++
> > > > > > > > kernel/audit.h | 1 +
> > > > > > > > kernel/auditsc.c | 1 +
> > > > > > > > security/security.c | 10 +++++++
> > > > > > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > > > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > > > > > >
> > > > > > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..ad1aeaa5f6c0
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > > > > > @@ -0,0 +1,34 @@
> > > > > > > > +===================
> > > > > > > > +Executability check
> > > > > > > > +===================
> > > > > > > > +
> > > > > > > > +AT_EXECVE_CHECK
> > > > > > > > +===============
> > > > > > > > +
> > > > > > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > > > > > +check on a regular file and returns 0 if execution of this file would be
> > > > > > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > > > > > +(e.g. ELF libraries, script's shebang).
> > > > > > > > +
> > > > > > > > +Programs should always perform this check to apply kernel-level checks against
> > > > > > > > +files that are not directly executed by the kernel but passed to a user space
> > > > > > > > +interpreter instead. All files that contain executable code, from the point of
> > > > > > > > +view of the interpreter, should be checked. However the result of this check
> > > > > > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > > > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > > > > > Regarding "should only"
> > > > > > > Userspace (e.g. libc) could decide to enforce even when
> > > > > > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > > > > > doesn't make sense.
> > > > > >
> > > > > > User space is always in control, but I don't think it would be wise to
> > > > > > not follow the configuration securebits (in a generic system) because
> > > > > > this could result to unattended behaviors (I don't have a specific one
> > > > > > in mind but...). That being said, configuration and checks are
> > > > > > standalones and specific/tailored systems are free to do the checks they
> > > > > > want.
> > > > > >
> > > > > In the case of dynamic linker, we can always enforce honoring the
> > > > > execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> > > > > to, the dynamic linker doesn't need to check the
> > > > > SECBIT_EXEC_RESTRICT_FILE bit.
> > > >
> > > > If the library file is not allowed to be executed by *all* access
> > > > control systems (not just mount and file permission, but all LSMs), then
> > > > the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
> > > > requirement.
> > > Yes. specifically for the library loading case, I can't think of a
> > > case where we need to by-pass LSMs. (letting user space to by-pass
> > > LSM check seems questionable in concept, and should only be used when
> > > there aren't other solutions). In the context of SELINUX enforcing
> > > mode, we will want to enforce it. In the context of process level LSM
> > > such as landlock, the process can already decide for itself by
> > > selecting the policy for its own domain, it is unnecessary to use
> > > another opt-out solution.
> >
> > My answer wasn't clear. The execveat(AT_EXECVE_CHECK) can and should
> > always be done, but user space should only enforce restrictions
> > according to the securebits.
> >
> I knew this part (AT_EXESCVE_CHECK is called always)
> Since the securebits are enforced by userspace, setting it to 0 is
> equivalent to opt-out enforcement, that is what I meant by opt-out.
OK, that was confusing because these bits are set to 0 by default (for
compatibility reasons).
>
> > It doesn't make sense to talk about user space "bypassing" kernel
> > checks. This patch series provides a feature to enable user space to
> > enforce (at its level) the same checks as the kernel.
> >
> > There is no opt-out solution, but compatibility configuration bits
> > through securebits (which can also be set by LSMs).
> >
> > To answer your question about the dynamic linker, there should be no
> > difference of behavior with a script interpreter. Both should check
> > executability but only enforce restriction according to the securebits
> > (as explained in the documentation). Doing otherwise on a generic
> > distro could lead to unexpected behaviors (e.g. if a user enforced a
> > specific SELinux policy that doesn't allow execution of library files).
> >
> > >
> > > There is one case where I see a difference:
> > > ld.so a.out (when a.out is on non-exec mount)
> > >
> > > If the dynamic linker doesn't read SECBIT_EXEC_RESTRICT_FILE setting,
> > > above will always fail. But that is more of a bugfix.
> >
> > No, the dynamic linker should only enforce restrictions according to the
> > securebits, otherwise a user space update (e.g. with a new dynamic
> > linker ignoring the securebits) could break an existing system.
> >
> OK. upgrade is a valid concern. Previously, I was just thinking about
> a new LSM based on this check, not existing LSM policies.
> Do you happen to know which SELinux policy/LSM could break ? i.e. it
> will be applied to libraries once we add AT_EXESCVE_CHECK in the
> dynamic linker.
We cannot assume anything about LSM policies because of custom and
private ones.
> We could give heads up and prepare for that.
>
> > >
> > > >Relying on the securebits to know if this is a hard
> > > > requirement or not enables system administrator and distros to control
> > > > this potential behavior change.
> > > >
> > > I think, for the dynamic linker, it can be a hard requirement.
> >
> > Not on a generic distro.
> >
> Ok. Maybe this can be done through a configuration option for the
> dynamic linker.
Yes, we could have a built-time option (disabled by default) for the
dynamic linker to enforce that.
>
> The consideration I have is: securebits is currently designed to
> control both dynamic linker and shell scripts.
> The case for dynamic linker is simpler than scripts cases, (non-exec
> mount, and perhaps some LSM policies for libraries) and distributions
> such as ChromeOS can enforce the dynamic linker case ahead of scripts
> interrupter cases, i.e. without waiting for python/shell being
> upgraded, that can take sometimes.
For secure systems, the end goal is to always enforce such restrictions,
so once interpretation/execution of a set of file types (e.g. ELF
libraries) are tested enough in such a system, we can remove the
securebits checks for the related library/executable (e.g. ld.so) and
consider that they are always set, independently of the current
user/credentials.
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Roberto Sassu @ 2024-11-27 9:51 UTC (permalink / raw)
To: Luis Chamberlain
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <Z0Ybvzy7ianR-Sx9@bombadil.infradead.org>
On Tue, 2024-11-26 at 11:04 -0800, Luis Chamberlain wrote:
> On Tue, Nov 26, 2024 at 11:25:07AM +0100, Roberto Sassu wrote:
> > On Mon, 2024-11-25 at 15:53 -0800, Luis Chamberlain wrote:
> >
> > Firmware, eBPF programs and so on are supposed
>
> Keyword: "supposed".
It depends if they are in a policy. They can also be verified with
other methods, such as file signatures.
For eBPF programs we are also in a need for a better way to
measure/appraise them.
> > As far as the LSM infrastructure is concerned, I'm not adding new LSM
> > hooks, nor extending/modifying the existing ones. The operations the
> > Integrity Digest Cache is doing match the usage expectation by LSM (net
> > denying access, as discussed with Paul Moore).
>
> If modules are the only proven exception to your security model you are
> not making the case for it clearly.
The Integrity Digest Cache is not implementing any security model, this
is demanded to other LSMs which might decide to use the Integrity
Digest Cache based on a policy.
If we want to be super picky, the ksys_finit_module() helper is not
calling security_kernel_module_request(), which is done when using
request_module(). On the other hand, ksys_finit_module() is not
triggering user space, as the description of the function states
(anyway, apologies for not bringing up this earlier).
Net this, and we can discuss if it is more appropriate to call the LSM
hook, the helper does not introduce any exception since
security_file_open() is called when the kernel opens the file
descriptor, and security_kernel_read_file() and
security_kernel_post_read_file() are called in the same way regardless
if it is user space doing insmod or the kernel calling
ksys_finit_module().
The only exception is that the Integrity Digest Cache is unable to
verify the kernel modules containing the parsers, but I believe this is
fine because they are verified with their appended signature.
If there are any other concerns I'm missing, please let me know.
> > The Integrity Digest Cache is supposed to be used as a supporting tool
> > for other LSMs to do regular access control based on file data and
> > metadata integrity. In doing that, it still needs the LSM
> > infrastructure to notify about filesystem changes, and to store
> > additional information in the inode and file descriptor security blobs.
> >
> > The kernel_post_read_file LSM hook should be implemented by another LSM
> > to verify the integrity of a digest list, when the Integrity Digest
> > Cache calls kernel_read_file() to read that digest list.
>
> If LSM folks *do* agree that this work is *suplementing* LSMS then sure,
> it was not clear from the commit logs. But then you need to ensure the
> parsers are special snowflakes which won't ever incur other additional
> kernel_read_file() calls.
The Integrity Digest Cache was originally called digest_cache LSM, but
was renamed due to Paul's concern that it is not a proper LSM enforcing
a security model. If you are interested, I gave a talk at LSS NA 2024:
https://www.youtube.com/watch?v=aNwlKYSksg8
Given that the Integrity Digest Cache could not be standalone and use
the LSM infrastructure facilities, it is going to be directly
integrated in IMA, although it is not strictly necessary.
I planned to support IPE and BPF LSM as other users.
Uhm, let me clarify your last sentence a bit.
Let's assume that IMA is asked to verify a parser, when invoked through
the kernel_post_read_file hook. IMA is not handling the exception, and
is calling digest_cache_get() as usual. Normally, this would succeed,
but because digest_cache_get() realizes that the file descriptor passed
as argument is marked (i.e. it was opened by the Integrity Digest Cache
itself), it returns NULL.
That means that IMA falls back on another verification method, which is
verifying the appended signature.
The most important design principle that I introduced is that users of
the Integrity Digest Cache don't need to be aware of any exception,
everything is handled by the Integrity Digest Cache itself.
The same occurs when a kernel read occurs with file ID
READING_DIGEST_LIST (introduced in this patch set). Yes, I forbid
specifying an IMA policy which requires the Integrity Digest Cache to
verify digest lists, but due to the need of handling kernel modules
I decided to handle the exceptions in the Integrity Digest Cache itself
(this is why now I'm passing a file descriptor to digest_cache_get()
instead of a dentry).
Now, I'm trying to follow you on the additional kernel_read_file()
calls. I agree with you, if a parser tries to open again the file that
is being verified it would cause a deadlock in IMA (since the inode
mutex is already locked for verifying the original file).
In the Integrity Digest Cache itself, this is not going to happen,
since the file being verified with a digest cache is known and an
internal open of the same file fails. If it is really necessary, we can
pass the information to the parsers so that they are aware, it is just
an additional parameter.
However, I was assuming that a parser just receives the data read by
the Integrity Digest Cache, and just calls the Parser API to add the
extracted digests to the new digest cache. Also this can be discussed,
but I guess there is no immediate need.
> > Supporting kernel modules opened the road for new deadlocks, since one
> > can ask a digest list to verify a kernel module, but that digest list
> > requires the same kernel module. That is why the in-kernel mechanism is
> > 100% reliable,
>
> Are users of this infrastructure really in need of modules for these
> parsers?
I planned to postpone this to later, and introduced two parsers built-
in (TLV and RPM). However, due to Linus's concern regarding the RPM
parser, I moved it out in a kernel module.
Also, a parser cannot be in user space, since the trust anchor is in
the kernel (the public keys and the signature verification mechanism),
it is not something that can be established in the initial ram disk
since the Integrity Digest Cache will be continously used in the
running system (maybe more parsers will be loaded on demand depending
on requests from user space).
And finally, the parser cannot run in user space, since it would be at
the same level of what the kernel is verifying.
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Luis Chamberlain @ 2024-11-26 19:04 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <d428a5d926d695ebec170e98463f7501a1b00793.camel@huaweicloud.com>
On Tue, Nov 26, 2024 at 11:25:07AM +0100, Roberto Sassu wrote:
> On Mon, 2024-11-25 at 15:53 -0800, Luis Chamberlain wrote:
>
> Firmware, eBPF programs and so on are supposed
Keyword: "supposed".
> As far as the LSM infrastructure is concerned, I'm not adding new LSM
> hooks, nor extending/modifying the existing ones. The operations the
> Integrity Digest Cache is doing match the usage expectation by LSM (net
> denying access, as discussed with Paul Moore).
If modules are the only proven exception to your security model you are
not making the case for it clearly.
> The Integrity Digest Cache is supposed to be used as a supporting tool
> for other LSMs to do regular access control based on file data and
> metadata integrity. In doing that, it still needs the LSM
> infrastructure to notify about filesystem changes, and to store
> additional information in the inode and file descriptor security blobs.
>
> The kernel_post_read_file LSM hook should be implemented by another LSM
> to verify the integrity of a digest list, when the Integrity Digest
> Cache calls kernel_read_file() to read that digest list.
If LSM folks *do* agree that this work is *suplementing* LSMS then sure,
it was not clear from the commit logs. But then you need to ensure the
parsers are special snowflakes which won't ever incur other additional
kernel_read_file() calls.
> Supporting kernel modules opened the road for new deadlocks, since one
> can ask a digest list to verify a kernel module, but that digest list
> requires the same kernel module. That is why the in-kernel mechanism is
> 100% reliable,
Are users of this infrastructure really in need of modules for these
parsers?
Luis
^ permalink raw reply
* Re: [PATCH v21 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mimi Zohar @ 2024-11-26 17:41 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241122.ahY1pooz1ing@digikod.net>
On Fri, 2024-11-22 at 15:50 +0100, Mickaël Salaün wrote:
> On Thu, Nov 21, 2024 at 03:34:47PM -0500, Mimi Zohar wrote:
> > Hi Mickaël,
> >
> > On Tue, 2024-11-12 at 20:18 +0100, Mickaël Salaün wrote:
> > >
> > > +
> > > +/* Returns 1 on error, 0 otherwise. */
> > > +static int interpret_stream(FILE *script, char *const script_name,
> > > + char *const *const envp, const bool restrict_stream)
> > > +{
> > > + int err;
> > > + char *const script_argv[] = { script_name, NULL };
> > > + char buf[128] = {};
> > > + size_t buf_size = sizeof(buf);
> > > +
> > > + /*
> > > + * We pass a valid argv and envp to the kernel to emulate a native
> > > + * script execution. We must use the script file descriptor instead of
> > > + * the script path name to avoid race conditions.
> > > + */
> > > + err = execveat(fileno(script), "", script_argv, envp,
> > > + AT_EMPTY_PATH | AT_EXECVE_CHECK);
> >
> > At least with v20, the AT_CHECK always was being set, independent of whether
> > set-exec.c set it. I'll re-test with v21.
>
> AT_EXECVE_CEHCK should always be set, only the interpretation of the
> result should be relative to securebits. This is highlighted in the
> documentation.
Sure, that sounds correct. With an IMA-appraisal policy, any unsigned script
with the is_check flag set now emits an "cause=IMA-signature-required" audit
message. However since IMA-appraisal isn't enforcing file signatures, this
sounds wrong.
New audit messages like "IMA-signature-required-by-interpreter" and "IMA-
signature-not-required-by-interpreter" would need to be defined based on the
SECBIT_EXEC_RESTRICT_FILE.
> >
> > > + if (err && restrict_stream) {
> > > + perror("ERROR: Script execution check");
> > > + return 1;
> > > + }
> > > +
> > > + /* Reads script. */
> > > + buf_size = fread(buf, 1, buf_size - 1, script);
> > > + return interpret_buffer(buf, buf_size);
> > > +}
> > > +
> >
> >
>
^ permalink raw reply
* Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
From: Roberto Sassu @ 2024-11-26 10:41 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Dmitry Kasatkin, corbet@lwn.net, mcgrof@kernel.org,
petr.pavlu@suse.com, samitolvanen@google.com,
da.gomez@samsung.com, Andrew Morton, paul@paul-moore.com,
jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-modules@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, wufan@linux.microsoft.com,
pbrobinson@gmail.com, zbyszek@in.waw.pl, hch@lst.de,
mjg59@srcf.ucam.org, pmatilai@redhat.com, jannh@google.com,
dhowells@redhat.com, jikos@kernel.org, mkoutny@suse.com,
ppavlu@suse.com, petr.vorel@gmail.com, mzerqung@0pointer.de,
kgold@linux.ibm.com, Roberto Sassu
In-Reply-To: <C4BE31F8-1FA3-4AD1-A712-ED2AA7E61E96@oracle.com>
On Tue, 2024-11-26 at 00:13 +0000, Eric Snowberg wrote:
>
> > On Nov 19, 2024, at 3:49 AM, Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> >
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > The Integrity Digest Cache can also help IMA for appraisal. IMA can simply
> > lookup the calculated digest of an accessed file in the list of digests
> > extracted from package headers, after verifying the header signature. It is
> > sufficient to verify only one signature for all files in the package, as
> > opposed to verifying a signature for each file.
>
> Is there a way to maintain integrity over time? Today if a CVE is discovered
> in a signed program, the program hash can be added to the blacklist keyring.
> Later if IMA appraisal is used, the signature validation will fail just for that
> program. With the Integrity Digest Cache, is there a way to do this?
As far as I can see, the ima_check_blacklist() call is before
ima_appraise_measurement(). If it fails, appraisal with the Integrity
Digest Cache will not be done.
In the future, we might use the Integrity Digest Cache for blacklists
too. Since a digest cache is reset on a file/directory change, IMA
would have to revalidate the program digest against a new digest cache.
Thanks
Roberto
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox