Message ID | 20231009124046.74710-3-hengqi.chen@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | seccomp: Make seccomp filter reusable | expand |
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...
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
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
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
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; }
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(+)