* [PATCH 1/4] seccomp: Refactor filter copy/create for reuse
2023-10-09 12:40 [PATCH 0/4] seccomp: Make seccomp filter reusable Hengqi Chen
@ 2023-10-09 12:40 ` Hengqi Chen
2023-10-11 0:14 ` Kees Cook
2023-10-09 12:40 ` [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation Hengqi Chen
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Hengqi Chen @ 2023-10-09 12:40 UTC (permalink / raw)
To: linux-kernel, bpf
Cc: keescook, ast, daniel, andrii, luto, wad, alexyonghe, hengqi.chen
This extracts two helpers for reuse in subsequent additions.
No functional change intended, just a prep work.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
kernel/seccomp.c | 76 ++++++++++++++++++++++++++++++++++--------------
1 file changed, 54 insertions(+), 22 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 255999ba9190..37490497f687 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -640,14 +640,14 @@ static inline void seccomp_sync_threads(unsigned long flags)
}
/**
- * seccomp_prepare_filter: Prepares a seccomp filter for use.
- * @fprog: BPF program to install
+ * seccomp_prepare_prog - prepares a JITed BPF filter for use.
+ * @pfp: the unattached filter that is created
+ * @fprog: the filter program
*
- * Returns filter on success or an ERR_PTR on failure.
+ * Returns 0 on success and non-zero otherwise.
*/
-static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
+static int seccomp_prepare_prog(struct bpf_prog **pfp, struct sock_fprog *fprog)
{
- struct seccomp_filter *sfilter;
int ret;
const bool save_orig =
#if defined(CONFIG_CHECKPOINT_RESTORE) || defined(SECCOMP_ARCH_NATIVE)
@@ -657,10 +657,28 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
#endif
if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
BUG_ON(INT_MAX / fprog->len < sizeof(struct sock_filter));
+ ret = bpf_prog_create_from_user(pfp, fprog, seccomp_check_filter, save_orig);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * seccomp_prepare_filter: Prepares a seccomp filter for use.
+ * @fprog: BPF program to install
+ *
+ * Returns filter on success or an ERR_PTR on failure.
+ */
+static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
+{
+ struct seccomp_filter *sfilter;
+ int ret;
+
/*
* Installing a seccomp filter requires that the task has
* CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
@@ -677,8 +695,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
return ERR_PTR(-ENOMEM);
mutex_init(&sfilter->notify_lock);
- ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
- seccomp_check_filter, save_orig);
+ ret = seccomp_prepare_prog(&sfilter->prog, fprog);
if (ret < 0) {
kfree(sfilter);
return ERR_PTR(ret);
@@ -692,31 +709,46 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
}
/**
- * seccomp_prepare_user_filter - prepares a user-supplied sock_fprog
+ * seccomp_copy_user_filter - copies a user-supplied sock_fprog
* @user_filter: pointer to the user data containing a sock_fprog.
+ * @fprog: pointer to store the copied BPF program
*
* Returns 0 on success and non-zero otherwise.
*/
-static struct seccomp_filter *
-seccomp_prepare_user_filter(const char __user *user_filter)
+static int seccomp_copy_user_filter(const char __user *user_filter, struct sock_fprog *fprog)
{
- struct sock_fprog fprog;
- struct seccomp_filter *filter = ERR_PTR(-EFAULT);
-
#ifdef CONFIG_COMPAT
if (in_compat_syscall()) {
struct compat_sock_fprog fprog32;
if (copy_from_user(&fprog32, user_filter, sizeof(fprog32)))
- goto out;
- fprog.len = fprog32.len;
- fprog.filter = compat_ptr(fprog32.filter);
+ return -EFAULT;
+ fprog->len = fprog32.len;
+ fprog->filter = compat_ptr(fprog32.filter);
} else /* falls through to the if below. */
#endif
- if (copy_from_user(&fprog, user_filter, sizeof(fprog)))
- goto out;
- filter = seccomp_prepare_filter(&fprog);
-out:
- return filter;
+ if (copy_from_user(fprog, user_filter, sizeof(*fprog)))
+ return -EFAULT;
+
+ return 0;
+}
+
+/**
+ * seccomp_prepare_user_filter - prepares a user-supplied sock_fprog
+ * @user_filter: pointer to the user data containing a sock_fprog.
+ *
+ * Returns filter on success or an ERR_PTR on failure.
+ */
+static struct seccomp_filter *
+seccomp_prepare_user_filter(const char __user *user_filter)
+{
+ struct sock_fprog fprog;
+ int ret;
+
+ ret = seccomp_copy_user_filter(user_filter, &fprog);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return seccomp_prepare_filter(&fprog);
}
#ifdef SECCOMP_ARCH_NATIVE
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] seccomp: Refactor filter copy/create for reuse
2023-10-09 12:40 ` [PATCH 1/4] seccomp: Refactor filter copy/create for reuse Hengqi Chen
@ 2023-10-11 0:14 ` Kees Cook
0 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2023-10-11 0:14 UTC (permalink / raw)
To: Hengqi Chen; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Mon, Oct 09, 2023 at 12:40:43PM +0000, Hengqi Chen wrote:
> This extracts two helpers for reuse in subsequent additions.
> No functional change intended, just a prep work.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Thanks! This looks like a clean refactoring. I actually think the error
handling is more obvious now too. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
2023-10-09 12:40 [PATCH 0/4] seccomp: Make seccomp filter reusable Hengqi Chen
2023-10-09 12:40 ` [PATCH 1/4] seccomp: Refactor filter copy/create for reuse Hengqi Chen
@ 2023-10-09 12:40 ` Hengqi Chen
2023-10-11 0:24 ` Kees Cook
` (2 more replies)
2023-10-09 12:40 ` [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation Hengqi Chen
2023-10-09 12:40 ` [PATCH 4/4] selftests/seccomp: Test SECCOMP_LOAD_FILTER and SECCOMP_ATTACH_FILTER Hengqi Chen
3 siblings, 3 replies; 13+ messages in thread
From: Hengqi Chen @ 2023-10-09 12:40 UTC (permalink / raw)
To: linux-kernel, bpf
Cc: keescook, ast, daniel, andrii, luto, wad, alexyonghe, hengqi.chen
This patch adds a new operation named SECCOMP_LOAD_FILTER.
It accepts the same arguments as SECCOMP_SET_MODE_FILTER
but only performs the loading process. If succeed, return a
new fd associated with the JITed BPF program (the filter).
The filter can then be pinned to bpffs using the returned
fd and reused for different processes. To distinguish the
filter from other BPF progs, BPF_PROG_TYPE_SECCOMP is added.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
include/uapi/linux/bpf.h | 1 +
include/uapi/linux/seccomp.h | 1 +
kernel/seccomp.c | 40 ++++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 70bfa997e896..8890fb776bbb 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -995,6 +995,7 @@ enum bpf_prog_type {
BPF_PROG_TYPE_SK_LOOKUP,
BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */
BPF_PROG_TYPE_NETFILTER,
+ BPF_PROG_TYPE_SECCOMP,
};
enum bpf_attach_type {
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index dbfc9b37fcae..ee2c83697810 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -16,6 +16,7 @@
#define SECCOMP_SET_MODE_FILTER 1
#define SECCOMP_GET_ACTION_AVAIL 2
#define SECCOMP_GET_NOTIF_SIZES 3
+#define SECCOMP_LOAD_FILTER 4
/* Valid flags for SECCOMP_SET_MODE_FILTER */
#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 37490497f687..3ae43db3b642 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -2028,12 +2028,47 @@ static long seccomp_set_mode_filter(unsigned int flags,
seccomp_filter_free(prepared);
return ret;
}
+
+static long seccomp_load_filter(const char __user *filter)
+{
+ struct sock_fprog fprog;
+ struct bpf_prog *prog;
+ int ret;
+
+ ret = seccomp_copy_user_filter(filter, &fprog);
+ if (ret)
+ return ret;
+
+ ret = seccomp_prepare_prog(&prog, &fprog);
+ if (ret)
+ return ret;
+
+ ret = security_bpf_prog_alloc(prog->aux);
+ if (ret) {
+ bpf_prog_free(prog);
+ return ret;
+ }
+
+ prog->aux->user = get_current_user();
+ atomic64_set(&prog->aux->refcnt, 1);
+ prog->type = BPF_PROG_TYPE_SECCOMP;
+
+ ret = bpf_prog_new_fd(prog);
+ if (ret < 0)
+ bpf_prog_put(prog);
+ return ret;
+}
#else
static inline long seccomp_set_mode_filter(unsigned int flags,
const char __user *filter)
{
return -EINVAL;
}
+
+static inline long seccomp_load_filter(const char __user *filter)
+{
+ return -EINVAL;
+}
#endif
static long seccomp_get_action_avail(const char __user *uaction)
@@ -2095,6 +2130,11 @@ static long do_seccomp(unsigned int op, unsigned int flags,
return -EINVAL;
return seccomp_get_notif_sizes(uargs);
+ case SECCOMP_LOAD_FILTER:
+ if (flags != 0)
+ return -EINVAL;
+
+ return seccomp_load_filter(uargs);
default:
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
2023-10-09 12:40 ` [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation Hengqi Chen
@ 2023-10-11 0:24 ` Kees Cook
2023-10-12 1:48 ` Hengqi Chen
2023-10-11 7:16 ` kernel test robot
2023-10-11 9:15 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2023-10-11 0:24 UTC (permalink / raw)
To: Hengqi Chen; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Mon, Oct 09, 2023 at 12:40:44PM +0000, Hengqi Chen wrote:
> This patch adds a new operation named SECCOMP_LOAD_FILTER.
> It accepts the same arguments as SECCOMP_SET_MODE_FILTER
> but only performs the loading process. If succeed, return a
> new fd associated with the JITed BPF program (the filter).
> The filter can then be pinned to bpffs using the returned
> fd and reused for different processes. To distinguish the
> filter from other BPF progs, BPF_PROG_TYPE_SECCOMP is added.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
This part looks okay, I think. I need to spend some more time looking at
the BPF side. I want to make sure it is only possible to build a
BPF_PROG_TYPE_SECCOMP prog by going through seccomp. I want to make sure
we can never side-load some kind of unexpected program into seccomp,
etc. Since BPF_PROG_TYPE_SECCOMP is part of UAPI, is this controllable
through the bpf() syscall?
One thought I had, though, is I wonder if flags are needed to be
included with the fd? I'll ponder this a bit more...
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
2023-10-11 0:24 ` Kees Cook
@ 2023-10-12 1:48 ` Hengqi Chen
0 siblings, 0 replies; 13+ messages in thread
From: Hengqi Chen @ 2023-10-12 1:48 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Wed, Oct 11, 2023 at 8:24 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 09, 2023 at 12:40:44PM +0000, Hengqi Chen wrote:
> > This patch adds a new operation named SECCOMP_LOAD_FILTER.
> > It accepts the same arguments as SECCOMP_SET_MODE_FILTER
> > but only performs the loading process. If succeed, return a
> > new fd associated with the JITed BPF program (the filter).
> > The filter can then be pinned to bpffs using the returned
> > fd and reused for different processes. To distinguish the
> > filter from other BPF progs, BPF_PROG_TYPE_SECCOMP is added.
> >
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>
> This part looks okay, I think. I need to spend some more time looking at
> the BPF side. I want to make sure it is only possible to build a
> BPF_PROG_TYPE_SECCOMP prog by going through seccomp. I want to make sure
> we can never side-load some kind of unexpected program into seccomp,
> etc. Since BPF_PROG_TYPE_SECCOMP is part of UAPI, is this controllable
> through the bpf() syscall?
>
Currently, it failed at find_prog_type() since we don't register the
prog type to BPF.
> One thought I had, though, is I wonder if flags are needed to be
> included with the fd? I'll ponder this a bit more...
>
bpf_prog_new_fd() already set O_RDWR and O_CLOEXEC.
> --
> Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
2023-10-09 12:40 ` [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation Hengqi Chen
2023-10-11 0:24 ` Kees Cook
@ 2023-10-11 7:16 ` kernel test robot
2023-10-11 9:15 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-10-11 7:16 UTC (permalink / raw)
To: Hengqi Chen, linux-kernel, bpf
Cc: llvm, oe-kbuild-all, keescook, ast, daniel, andrii, luto, wad,
alexyonghe, hengqi.chen
Hi Hengqi,
kernel test robot noticed the following build errors:
[auto build test ERROR on kees/for-next/seccomp]
[also build test ERROR on bpf-next/master bpf/master kees/for-next/pstore kees/for-next/kspp linus/master v6.6-rc5 next-20231010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hengqi-Chen/seccomp-Refactor-filter-copy-create-for-reuse/20231010-100354
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/seccomp
patch link: https://lore.kernel.org/r/20231009124046.74710-3-hengqi.chen%40gmail.com
patch subject: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20231011/202310111556.DzEDzt3Z-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/202310111556.DzEDzt3Z-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310111556.DzEDzt3Z-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> kernel/seccomp.c:2046:8: error: implicit declaration of function 'security_bpf_prog_alloc' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
ret = security_bpf_prog_alloc(prog->aux);
^
kernel/seccomp.c:2046:8: note: did you mean 'security_msg_msg_alloc'?
include/linux/security.h:1245:19: note: 'security_msg_msg_alloc' declared here
static inline int security_msg_msg_alloc(struct msg_msg *msg)
^
>> kernel/seccomp.c:2056:8: error: implicit declaration of function 'bpf_prog_new_fd' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
ret = bpf_prog_new_fd(prog);
^
12 warnings and 2 errors generated.
vim +/security_bpf_prog_alloc +2046 kernel/seccomp.c
2031
2032 static long seccomp_load_filter(const char __user *filter)
2033 {
2034 struct sock_fprog fprog;
2035 struct bpf_prog *prog;
2036 int ret;
2037
2038 ret = seccomp_copy_user_filter(filter, &fprog);
2039 if (ret)
2040 return ret;
2041
2042 ret = seccomp_prepare_prog(&prog, &fprog);
2043 if (ret)
2044 return ret;
2045
> 2046 ret = security_bpf_prog_alloc(prog->aux);
2047 if (ret) {
2048 bpf_prog_free(prog);
2049 return ret;
2050 }
2051
2052 prog->aux->user = get_current_user();
2053 atomic64_set(&prog->aux->refcnt, 1);
2054 prog->type = BPF_PROG_TYPE_SECCOMP;
2055
> 2056 ret = bpf_prog_new_fd(prog);
2057 if (ret < 0)
2058 bpf_prog_put(prog);
2059 return ret;
2060 }
2061 #else
2062 static inline long seccomp_set_mode_filter(unsigned int flags,
2063 const char __user *filter)
2064 {
2065 return -EINVAL;
2066 }
2067
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
2023-10-09 12:40 ` [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation Hengqi Chen
2023-10-11 0:24 ` Kees Cook
2023-10-11 7:16 ` kernel test robot
@ 2023-10-11 9:15 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-10-11 9:15 UTC (permalink / raw)
To: Hengqi Chen, linux-kernel, bpf
Cc: llvm, oe-kbuild-all, keescook, ast, daniel, andrii, luto, wad,
alexyonghe, hengqi.chen
Hi Hengqi,
kernel test robot noticed the following build errors:
[auto build test ERROR on kees/for-next/seccomp]
[also build test ERROR on bpf-next/master bpf/master kees/for-next/pstore kees/for-next/kspp linus/master v6.6-rc5 next-20231010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hengqi-Chen/seccomp-Refactor-filter-copy-create-for-reuse/20231010-100354
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/seccomp
patch link: https://lore.kernel.org/r/20231009124046.74710-3-hengqi.chen%40gmail.com
patch subject: [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation
config: um-allnoconfig (https://download.01.org/0day-ci/archive/20231011/202310111723.tp3m8LGq-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/202310111723.tp3m8LGq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310111723.tp3m8LGq-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from kernel/seccomp.c:29:
In file included from include/linux/syscalls.h:90:
In file included from include/trace/syscall.h:7:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
>> kernel/seccomp.c:2046:8: error: call to undeclared function 'security_bpf_prog_alloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
2046 | ret = security_bpf_prog_alloc(prog->aux);
| ^
kernel/seccomp.c:2046:8: note: did you mean 'security_msg_msg_alloc'?
include/linux/security.h:1245:19: note: 'security_msg_msg_alloc' declared here
1245 | static inline int security_msg_msg_alloc(struct msg_msg *msg)
| ^
>> kernel/seccomp.c:2056:8: error: call to undeclared function 'bpf_prog_new_fd'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
2056 | ret = bpf_prog_new_fd(prog);
| ^
12 warnings and 2 errors generated.
vim +/security_bpf_prog_alloc +2046 kernel/seccomp.c
2031
2032 static long seccomp_load_filter(const char __user *filter)
2033 {
2034 struct sock_fprog fprog;
2035 struct bpf_prog *prog;
2036 int ret;
2037
2038 ret = seccomp_copy_user_filter(filter, &fprog);
2039 if (ret)
2040 return ret;
2041
2042 ret = seccomp_prepare_prog(&prog, &fprog);
2043 if (ret)
2044 return ret;
2045
> 2046 ret = security_bpf_prog_alloc(prog->aux);
2047 if (ret) {
2048 bpf_prog_free(prog);
2049 return ret;
2050 }
2051
2052 prog->aux->user = get_current_user();
2053 atomic64_set(&prog->aux->refcnt, 1);
2054 prog->type = BPF_PROG_TYPE_SECCOMP;
2055
> 2056 ret = bpf_prog_new_fd(prog);
2057 if (ret < 0)
2058 bpf_prog_put(prog);
2059 return ret;
2060 }
2061 #else
2062 static inline long seccomp_set_mode_filter(unsigned int flags,
2063 const char __user *filter)
2064 {
2065 return -EINVAL;
2066 }
2067
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation
2023-10-09 12:40 [PATCH 0/4] seccomp: Make seccomp filter reusable Hengqi Chen
2023-10-09 12:40 ` [PATCH 1/4] seccomp: Refactor filter copy/create for reuse Hengqi Chen
2023-10-09 12:40 ` [PATCH 2/4] seccomp, bpf: Introduce SECCOMP_LOAD_FILTER operation Hengqi Chen
@ 2023-10-09 12:40 ` Hengqi Chen
2023-10-11 0:22 ` Kees Cook
2023-10-09 12:40 ` [PATCH 4/4] selftests/seccomp: Test SECCOMP_LOAD_FILTER and SECCOMP_ATTACH_FILTER Hengqi Chen
3 siblings, 1 reply; 13+ messages in thread
From: Hengqi Chen @ 2023-10-09 12:40 UTC (permalink / raw)
To: linux-kernel, bpf
Cc: keescook, ast, daniel, andrii, luto, wad, alexyonghe, hengqi.chen
The SECCOMP_ATTACH_FILTER operation is used to attach
a loaded filter to the current process. The loaded filter
is represented by a fd which is either returned by the
SECCOMP_LOAD_FILTER operation or obtained from bpffs using
bpf syscall.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
include/uapi/linux/seccomp.h | 1 +
kernel/seccomp.c | 68 +++++++++++++++++++++++++++++++++---
2 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index ee2c83697810..fbe30262fdfc 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -17,6 +17,7 @@
#define SECCOMP_GET_ACTION_AVAIL 2
#define SECCOMP_GET_NOTIF_SIZES 3
#define SECCOMP_LOAD_FILTER 4
+#define SECCOMP_ATTACH_FILTER 5
/* Valid flags for SECCOMP_SET_MODE_FILTER */
#define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 3ae43db3b642..9f9d8a7a1d6e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -523,7 +523,10 @@ static inline pid_t seccomp_can_sync_threads(void)
static inline void seccomp_filter_free(struct seccomp_filter *filter)
{
if (filter) {
- bpf_prog_destroy(filter->prog);
+ if (filter->prog->type == BPF_PROG_TYPE_SECCOMP)
+ bpf_prog_put(filter->prog);
+ else
+ bpf_prog_destroy(filter->prog);
kfree(filter);
}
}
@@ -894,7 +897,7 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
#endif /* SECCOMP_ARCH_NATIVE */
/**
- * seccomp_attach_filter: validate and attach filter
+ * seccomp_do_attach_filter: validate and attach filter
* @flags: flags to change filter behavior
* @filter: seccomp filter to add to the current process
*
@@ -905,8 +908,8 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
* seccomp mode or did not have an ancestral seccomp filter
* - in NEW_LISTENER mode: the fd of the new listener
*/
-static long seccomp_attach_filter(unsigned int flags,
- struct seccomp_filter *filter)
+static long seccomp_do_attach_filter(unsigned int flags,
+ struct seccomp_filter *filter)
{
unsigned long total_insns;
struct seccomp_filter *walker;
@@ -2001,7 +2004,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
goto out;
}
- ret = seccomp_attach_filter(flags, prepared);
+ ret = seccomp_do_attach_filter(flags, prepared);
if (ret)
goto out;
/* Do not free the successfully attached filter. */
@@ -2058,6 +2061,51 @@ static long seccomp_load_filter(const char __user *filter)
bpf_prog_put(prog);
return ret;
}
+
+static long seccomp_attach_filter(const char __user *ufd)
+{
+ const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
+ struct seccomp_filter *sfilter;
+ struct bpf_prog *prog;
+ int flags = 0;
+ int fd, ret;
+
+ if (copy_from_user(&fd, ufd, sizeof(fd)))
+ return -EFAULT;
+
+ prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SECCOMP);
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
+
+ sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL | __GFP_NOWARN);
+ if (!sfilter) {
+ bpf_prog_put(prog);
+ return -ENOMEM;
+ }
+
+ sfilter->prog = prog;
+ refcount_set(&sfilter->refs, 1);
+ refcount_set(&sfilter->users, 1);
+ mutex_init(&sfilter->notify_lock);
+ init_waitqueue_head(&sfilter->wqh);
+
+ spin_lock_irq(¤t->sighand->siglock);
+
+ ret = -EINVAL;
+ if (!seccomp_may_assign_mode(seccomp_mode))
+ goto out;
+
+ ret = seccomp_do_attach_filter(flags, sfilter);
+ if (ret)
+ goto out;
+
+ sfilter = NULL;
+ seccomp_assign_mode(current, seccomp_mode, flags);
+out:
+ spin_unlock_irq(¤t->sighand->siglock);
+ seccomp_filter_free(sfilter);
+ return ret;
+}
#else
static inline long seccomp_set_mode_filter(unsigned int flags,
const char __user *filter)
@@ -2069,6 +2117,11 @@ static inline long seccomp_load_filter(const char __user *filter)
{
return -EINVAL;
}
+
+static inline long seccomp_attach_filter(const char __user *ufd)
+{
+ return -EINVAL;
+}
#endif
static long seccomp_get_action_avail(const char __user *uaction)
@@ -2135,6 +2188,11 @@ static long do_seccomp(unsigned int op, unsigned int flags,
return -EINVAL;
return seccomp_load_filter(uargs);
+ case SECCOMP_ATTACH_FILTER:
+ if (flags != 0)
+ return -EINVAL;
+
+ return seccomp_attach_filter(uargs);
default:
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation
2023-10-09 12:40 ` [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation Hengqi Chen
@ 2023-10-11 0:22 ` Kees Cook
2023-10-12 1:49 ` Hengqi Chen
0 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2023-10-11 0:22 UTC (permalink / raw)
To: Hengqi Chen; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Mon, Oct 09, 2023 at 12:40:45PM +0000, Hengqi Chen wrote:
> The SECCOMP_ATTACH_FILTER operation is used to attach
> a loaded filter to the current process. The loaded filter
> is represented by a fd which is either returned by the
> SECCOMP_LOAD_FILTER operation or obtained from bpffs using
> bpf syscall.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> include/uapi/linux/seccomp.h | 1 +
> kernel/seccomp.c | 68 +++++++++++++++++++++++++++++++++---
> 2 files changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index ee2c83697810..fbe30262fdfc 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -17,6 +17,7 @@
> #define SECCOMP_GET_ACTION_AVAIL 2
> #define SECCOMP_GET_NOTIF_SIZES 3
> #define SECCOMP_LOAD_FILTER 4
> +#define SECCOMP_ATTACH_FILTER 5
>
> /* Valid flags for SECCOMP_SET_MODE_FILTER */
> #define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 3ae43db3b642..9f9d8a7a1d6e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -523,7 +523,10 @@ static inline pid_t seccomp_can_sync_threads(void)
> static inline void seccomp_filter_free(struct seccomp_filter *filter)
> {
> if (filter) {
> - bpf_prog_destroy(filter->prog);
> + if (filter->prog->type == BPF_PROG_TYPE_SECCOMP)
> + bpf_prog_put(filter->prog);
> + else
> + bpf_prog_destroy(filter->prog);
> kfree(filter);
> }
> }
> @@ -894,7 +897,7 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
> #endif /* SECCOMP_ARCH_NATIVE */
>
> /**
> - * seccomp_attach_filter: validate and attach filter
> + * seccomp_do_attach_filter: validate and attach filter
> * @flags: flags to change filter behavior
> * @filter: seccomp filter to add to the current process
> *
> @@ -905,8 +908,8 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
> * seccomp mode or did not have an ancestral seccomp filter
> * - in NEW_LISTENER mode: the fd of the new listener
> */
> -static long seccomp_attach_filter(unsigned int flags,
> - struct seccomp_filter *filter)
> +static long seccomp_do_attach_filter(unsigned int flags,
> + struct seccomp_filter *filter)
> {
> unsigned long total_insns;
> struct seccomp_filter *walker;
> @@ -2001,7 +2004,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
> goto out;
> }
>
> - ret = seccomp_attach_filter(flags, prepared);
> + ret = seccomp_do_attach_filter(flags, prepared);
> if (ret)
> goto out;
> /* Do not free the successfully attached filter. */
> @@ -2058,6 +2061,51 @@ static long seccomp_load_filter(const char __user *filter)
> bpf_prog_put(prog);
> return ret;
> }
> +
> +static long seccomp_attach_filter(const char __user *ufd)
> +{
> + const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
> + struct seccomp_filter *sfilter;
> + struct bpf_prog *prog;
> + int flags = 0;
> + int fd, ret;
> +
> + if (copy_from_user(&fd, ufd, sizeof(fd)))
> + return -EFAULT;
> +
> + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SECCOMP);
> + if (IS_ERR(prog))
> + return PTR_ERR(prog);
> +
> + sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL | __GFP_NOWARN);
> + if (!sfilter) {
> + bpf_prog_put(prog);
> + return -ENOMEM;
> + }
> +
> + sfilter->prog = prog;
> + refcount_set(&sfilter->refs, 1);
> + refcount_set(&sfilter->users, 1);
> + mutex_init(&sfilter->notify_lock);
> + init_waitqueue_head(&sfilter->wqh);
> +
> + spin_lock_irq(¤t->sighand->siglock);
> +
> + ret = -EINVAL;
> + if (!seccomp_may_assign_mode(seccomp_mode))
> + goto out;
> +
> + ret = seccomp_do_attach_filter(flags, sfilter);
> + if (ret)
> + goto out;
> +
> + sfilter = NULL;
> + seccomp_assign_mode(current, seccomp_mode, flags);
> +out:
> + spin_unlock_irq(¤t->sighand->siglock);
> + seccomp_filter_free(sfilter);
> + return ret;
> +}
This is duplicating part of seccomp_set_mode_filter() but without
handling flags at all. This isn't really workable, since we need things
like TSYNC, etc. I think it would be better to adjust
SECCOMP_SET_MODE_FILTER to take a new flag that indicates that the user
arg is an fd, not a filter. Then the middle of seccomp_set_mode_filter()
can choosen between seccomp_prepare_user_filter() and a wrapped call to
bpf_prog_get_type() on the fd, etc.
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation
2023-10-11 0:22 ` Kees Cook
@ 2023-10-12 1:49 ` Hengqi Chen
0 siblings, 0 replies; 13+ messages in thread
From: Hengqi Chen @ 2023-10-12 1:49 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Wed, Oct 11, 2023 at 8:22 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 09, 2023 at 12:40:45PM +0000, Hengqi Chen wrote:
> > The SECCOMP_ATTACH_FILTER operation is used to attach
> > a loaded filter to the current process. The loaded filter
> > is represented by a fd which is either returned by the
> > SECCOMP_LOAD_FILTER operation or obtained from bpffs using
> > bpf syscall.
> >
> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> > ---
> > include/uapi/linux/seccomp.h | 1 +
> > kernel/seccomp.c | 68 +++++++++++++++++++++++++++++++++---
> > 2 files changed, 64 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > index ee2c83697810..fbe30262fdfc 100644
> > --- a/include/uapi/linux/seccomp.h
> > +++ b/include/uapi/linux/seccomp.h
> > @@ -17,6 +17,7 @@
> > #define SECCOMP_GET_ACTION_AVAIL 2
> > #define SECCOMP_GET_NOTIF_SIZES 3
> > #define SECCOMP_LOAD_FILTER 4
> > +#define SECCOMP_ATTACH_FILTER 5
> >
> > /* Valid flags for SECCOMP_SET_MODE_FILTER */
> > #define SECCOMP_FILTER_FLAG_TSYNC (1UL << 0)
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 3ae43db3b642..9f9d8a7a1d6e 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -523,7 +523,10 @@ static inline pid_t seccomp_can_sync_threads(void)
> > static inline void seccomp_filter_free(struct seccomp_filter *filter)
> > {
> > if (filter) {
> > - bpf_prog_destroy(filter->prog);
> > + if (filter->prog->type == BPF_PROG_TYPE_SECCOMP)
> > + bpf_prog_put(filter->prog);
> > + else
> > + bpf_prog_destroy(filter->prog);
> > kfree(filter);
> > }
> > }
> > @@ -894,7 +897,7 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
> > #endif /* SECCOMP_ARCH_NATIVE */
> >
> > /**
> > - * seccomp_attach_filter: validate and attach filter
> > + * seccomp_do_attach_filter: validate and attach filter
> > * @flags: flags to change filter behavior
> > * @filter: seccomp filter to add to the current process
> > *
> > @@ -905,8 +908,8 @@ static void seccomp_cache_prepare(struct seccomp_filter *sfilter)
> > * seccomp mode or did not have an ancestral seccomp filter
> > * - in NEW_LISTENER mode: the fd of the new listener
> > */
> > -static long seccomp_attach_filter(unsigned int flags,
> > - struct seccomp_filter *filter)
> > +static long seccomp_do_attach_filter(unsigned int flags,
> > + struct seccomp_filter *filter)
> > {
> > unsigned long total_insns;
> > struct seccomp_filter *walker;
> > @@ -2001,7 +2004,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > goto out;
> > }
> >
> > - ret = seccomp_attach_filter(flags, prepared);
> > + ret = seccomp_do_attach_filter(flags, prepared);
> > if (ret)
> > goto out;
> > /* Do not free the successfully attached filter. */
> > @@ -2058,6 +2061,51 @@ static long seccomp_load_filter(const char __user *filter)
> > bpf_prog_put(prog);
> > return ret;
> > }
> > +
> > +static long seccomp_attach_filter(const char __user *ufd)
> > +{
> > + const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
> > + struct seccomp_filter *sfilter;
> > + struct bpf_prog *prog;
> > + int flags = 0;
> > + int fd, ret;
> > +
> > + if (copy_from_user(&fd, ufd, sizeof(fd)))
> > + return -EFAULT;
> > +
> > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SECCOMP);
> > + if (IS_ERR(prog))
> > + return PTR_ERR(prog);
> > +
> > + sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL | __GFP_NOWARN);
> > + if (!sfilter) {
> > + bpf_prog_put(prog);
> > + return -ENOMEM;
> > + }
> > +
> > + sfilter->prog = prog;
> > + refcount_set(&sfilter->refs, 1);
> > + refcount_set(&sfilter->users, 1);
> > + mutex_init(&sfilter->notify_lock);
> > + init_waitqueue_head(&sfilter->wqh);
> > +
> > + spin_lock_irq(¤t->sighand->siglock);
> > +
> > + ret = -EINVAL;
> > + if (!seccomp_may_assign_mode(seccomp_mode))
> > + goto out;
> > +
> > + ret = seccomp_do_attach_filter(flags, sfilter);
> > + if (ret)
> > + goto out;
> > +
> > + sfilter = NULL;
> > + seccomp_assign_mode(current, seccomp_mode, flags);
> > +out:
> > + spin_unlock_irq(¤t->sighand->siglock);
> > + seccomp_filter_free(sfilter);
> > + return ret;
> > +}
>
> This is duplicating part of seccomp_set_mode_filter() but without
> handling flags at all. This isn't really workable, since we need things
> like TSYNC, etc. I think it would be better to adjust
> SECCOMP_SET_MODE_FILTER to take a new flag that indicates that the user
> arg is an fd, not a filter. Then the middle of seccomp_set_mode_filter()
> can choosen between seccomp_prepare_user_filter() and a wrapped call to
> bpf_prog_get_type() on the fd, etc.
>
Great, that would make things easier. Thanks.
> --
> Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] selftests/seccomp: Test SECCOMP_LOAD_FILTER and SECCOMP_ATTACH_FILTER
2023-10-09 12:40 [PATCH 0/4] seccomp: Make seccomp filter reusable Hengqi Chen
` (2 preceding siblings ...)
2023-10-09 12:40 ` [PATCH 3/4] seccomp: Introduce SECCOMP_ATTACH_FILTER operation Hengqi Chen
@ 2023-10-09 12:40 ` Hengqi Chen
2023-10-11 0:26 ` Kees Cook
3 siblings, 1 reply; 13+ messages in thread
From: Hengqi Chen @ 2023-10-09 12:40 UTC (permalink / raw)
To: linux-kernel, bpf
Cc: keescook, ast, daniel, andrii, luto, wad, alexyonghe, hengqi.chen
Add a testcase to exercise the newly added SECCOMP_LOAD_FILTER
and SECCOMP_ATTACH_FILTER operations.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 38f651469968..8f7010482194 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4735,6 +4735,26 @@ TEST(user_notification_wait_killable_fatal)
EXPECT_EQ(SIGTERM, WTERMSIG(status));
}
+TEST(seccomp_filter_load_and_attach)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+ int fd, ret;
+
+ fd = seccomp(SECCOMP_LOAD_FILTER, 0, &prog);
+ ASSERT_GT(fd, -1);
+
+ ret = seccomp(SECCOMP_ATTACH_FILTER, 0, &fd);
+ ASSERT_EQ(ret, 0);
+
+ close(fd);
+}
+
/*
* TODO:
* - expand NNP testing
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] selftests/seccomp: Test SECCOMP_LOAD_FILTER and SECCOMP_ATTACH_FILTER
2023-10-09 12:40 ` [PATCH 4/4] selftests/seccomp: Test SECCOMP_LOAD_FILTER and SECCOMP_ATTACH_FILTER Hengqi Chen
@ 2023-10-11 0:26 ` Kees Cook
0 siblings, 0 replies; 13+ messages in thread
From: Kees Cook @ 2023-10-11 0:26 UTC (permalink / raw)
To: Hengqi Chen; +Cc: linux-kernel, bpf, ast, daniel, andrii, luto, wad, alexyonghe
On Mon, Oct 09, 2023 at 12:40:46PM +0000, Hengqi Chen wrote:
> Add a testcase to exercise the newly added SECCOMP_LOAD_FILTER
> and SECCOMP_ATTACH_FILTER operations.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> tools/testing/selftests/seccomp/seccomp_bpf.c | 20 +++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index 38f651469968..8f7010482194 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -4735,6 +4735,26 @@ TEST(user_notification_wait_killable_fatal)
> EXPECT_EQ(SIGTERM, WTERMSIG(status));
> }
>
> +TEST(seccomp_filter_load_and_attach)
> +{
> + struct sock_filter filter[] = {
> + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> + };
> + struct sock_fprog prog = {
> + .len = (unsigned short)ARRAY_SIZE(filter),
> + .filter = filter,
> + };
> + int fd, ret;
> +
> + fd = seccomp(SECCOMP_LOAD_FILTER, 0, &prog);
> + ASSERT_GT(fd, -1);
> +
> + ret = seccomp(SECCOMP_ATTACH_FILTER, 0, &fd);
> + ASSERT_EQ(ret, 0);
> +
> + close(fd);
> +}
This is a good start -- please check all the error paths as well.
Thanks for continuing to work on this!
--
Kees Cook
^ permalink raw reply [flat|nested] 13+ messages in thread