[PATCH] bpf: Add CONFIG_BPF_HELPER_STRICT

[PATCH] bpf: Add CONFIG_BPF_HELPER_STRICT

[Roland]

In container environment, ebpf helpers could be used maliciously to 
   leak information, DOS, even escape from containers. 
   CONFIG_BPF_HELPER_STRICT is as a mitigation of it. 
   Related Link: https://rolandorange.zone/report.html

Signed-off-by: Roland <kernel.pwn@outlook.com>
---
 include/linux/kernel.h     |  7 ++++++
 include/linux/sched.h      |  4 +++-
 include/uapi/linux/bpf.h   |  4 ++++
 include/uapi/linux/sched.h |  4 ++++
 kernel/bpf/Kconfig         |  6 +++++
 kernel/bpf/syscall.c       | 48 +++++++++++++++++++++++++++++++++++---
 kernel/fork.c              | 13 +++++++++++
 kernel/trace/bpf_trace.c   | 29 +++++++++++++++++++++--
 8 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fe6efb24d..61f7fcbc9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -509,3 +509,10 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	 BUILD_BUG_ON_ZERO((perms) & 2) +					\
 	 (perms))
 #endif
+
+#ifdef CONFIG_BPF_HELPER_STRICT
+# define BPF_PROBE_WRITE_BIT 1
+# define BPF_PROBE_READ_BIT  2
+# define BPF_SEND_SIGNAL_BIT 3
+# define BPF_OVERRIDE_RETURN_BIT 4
+#endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ffb6eb55c..a3ec52056 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -760,7 +760,9 @@ struct task_struct {
 	/* Per task flags (PF_*), defined further below: */
 	unsigned int			flags;
 	unsigned int			ptrace;
-
+#ifdef CONFIG_BPF_HELPER_STRICT
+	unsigned int			bpf_helper_bitfield;
+#endif
 #ifdef CONFIG_SMP
 	int				on_cpu;
 	struct __call_single_node	wake_entry;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 51b9aa640..99a90d0f5 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -900,6 +900,7 @@ enum bpf_cmd {
 	BPF_ITER_CREATE,
 	BPF_LINK_DETACH,
 	BPF_PROG_BIND_MAP,
+	BPF_HELPER_BITS_SET,
 };
 
 enum bpf_map_type {
@@ -1326,6 +1327,9 @@ union bpf_attr {
 		 * to using 5 hash functions).
 		 */
 		__u64	map_extra;
+#ifdef CONFIG_BPF_HELPER_STRICT
+		__u32 security_helper_bits;
+#endif
 	};
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 3bac0a8ce..c2fd463be 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -43,6 +43,10 @@
  */
 #define CLONE_NEWTIME	0x00000080	/* New time namespace */
 
+#ifdef CONFIG_BPF_HELPER_STRICT
+#define CLONE_BITFIELD	0x00000040	/* set if bpf_helper_bitfield shared between processes */
+#endif
+
 #ifndef __ASSEMBLY__
 /**
  * struct clone_args - arguments for the clone3 syscall
diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
index 2dfe1079f..c2c2fcf76 100644
--- a/kernel/bpf/Kconfig
+++ b/kernel/bpf/Kconfig
@@ -99,4 +99,10 @@ config BPF_LSM
 
 	  If you are unsure how to answer this question, answer N.
 
+config BPF_HELPER_STRICT
+        bool "Enable BPF HELPER Check bits"
+        depends on BPF_SYSCALL
+        help
+            Enable several check bits for bpf helpers' security improvements.
+
+	     BPF_HELPER_STRICT restricts the function of helpers.
+	     Currently it can be used for restrict override return,
+	     read, write, and send signal.
endmenu # "BPF subsystem"
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7b373a5e8..2f05ea50f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -68,6 +68,45 @@ static const struct bpf_map_ops * const bpf_map_types[] = {
 #undef BPF_LINK_TYPE
 };
 
+#ifdef CONFIG_BPF_HELPER_STRICT
+static __always_inline int HelperWrite(unsigned int bits)
+{
+	return ((unsigned int)bits & BPF_PROBE_WRITE_BIT) != 0;
+}
+static __always_inline int HelperRead(unsigned int bits)
+{
+	return ((unsigned int)bits & BPF_PROBE_READ_BIT) != 0;
+}
+static __always_inline int HelperSendSignal(unsigned int bits)
+{
+	return ((unsigned int)bits & BPF_SEND_SIGNAL_BIT) != 0;
+}
+static __always_inline int HelperOverrideReturn(unsigned int bits)
+{
+	return ((unsigned int)bits & BPF_OVERRIDE_RETURN_BIT) != 0;
+}
+int bpf_set_security_helper_bits(union bpf_attr *attr)
+{
+	int res;
+	unsigned int bits_to_set;
+	unsigned int expected_bpf_helper_bitfield = 0;
+
+	bits_to_set = attr->security_helper_bits;
+
+	if (HelperWrite(bits_to_set))
+		expected_bpf_helper_bitfield |= 1 << BPF_PROBE_WRITE_BIT;
+	if (HelperRead(bits_to_set))
+		expected_bpf_helper_bitfield |= 1 << BPF_PROBE_READ_BIT;
+	if (HelperSendSignal(bits_to_set))
+		expected_bpf_helper_bitfield |= 1 << BPF_SEND_SIGNAL_BIT;
+	if (HelperOverrideReturn(bits_to_set))
+		expected_bpf_helper_bitfield |= 1 << BPF_OVERRIDE_RETURN_BIT;
+
+	current->bpf_helper_bitfield = expected_bpf_helper_bitfield;
+	res = 0;
+	return res;
+}
+#endif
 /*
  * If we're handed a bigger struct than we know of, ensure all the unknown bits
  * are 0 - i.e. new user-space does not rely on any kernel feature extensions
@@ -4913,7 +4952,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 	union bpf_attr attr;
 	bool capable;
 	int err;
-
+
 	capable = bpf_capable() || !sysctl_unprivileged_bpf_disabled;
 
 	/* Intent here is for unprivileged_bpf_disabled to block key object
@@ -4925,7 +4964,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 	 * and other operations.
 	 */
 	if (!capable &&
-	    (cmd == BPF_MAP_CREATE || cmd == BPF_PROG_LOAD))
+	    (cmd == BPF_MAP_CREATE || cmd == BPF_PROG_LOAD || cmd == BPF_HELPER_BITS_SET))
 		return -EPERM;
 
 	err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
@@ -4938,7 +4977,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 	if (copy_from_bpfptr(&attr, uattr, size) != 0)
 		return -EFAULT;
 
-	err = security_bpf(cmd, &attr, size);
+	err = security_bpf(cmd, &attr, size);
 	if (err < 0)
 		return err;
 
@@ -5056,6 +5095,9 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 	case BPF_PROG_BIND_MAP:
 		err = bpf_prog_bind_map(&attr);
 		break;
+	case BPF_HELPER_BITS_SET:
+		err = bpf_set_security_helper_bits(&attr);
+		break;
 	default:
 		err = -EINVAL;
 		break;
diff --git a/kernel/fork.c b/kernel/fork.c
index 08969f5aa..6168da0b8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1998,6 +1998,10 @@ static __latent_entropy struct task_struct *copy_process(
 	const u64 clone_flags = args->flags;
 	struct nsproxy *nsp = current->nsproxy;
 
+#ifdef CONFIG_BPF_HELPER_STRICT
+	unsigned int bitfield = current->bpf_helper_bitfield;
+#endif
+
 	/*
 	 * Don't allow sharing the root directory with processes in a different
 	 * namespace
@@ -2102,6 +2106,7 @@ static __latent_entropy struct task_struct *copy_process(
 	 */
 	p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? args->child_tid : NULL;
 
+
 	ftrace_graph_init_task(p);
 
 	rt_mutex_init_task(p);
@@ -2490,6 +2495,14 @@ static __latent_entropy struct task_struct *copy_process(
 
 	copy_oom_score_adj(clone_flags, p);
 
+#ifdef CONFIG_BPF_HELPER_STRICT
+	/* Only if explicit set CLONE_BITFIELD or
+	 * the forked process has both CAP_BPF and CAP_SYS_ADMIN,
+	 * we will set the bitfield
+	 */
+	p->bpf_helper_bitfield = (clone_flags & CLONE_BITFIELD) ? bitfield : 0;
+	if (capable(CAP_BPF) && capable(CAP_SYS_ADMIN))
+		p->bpf_helper_bitfield = bitfield;
+#endif
 	return p;
 
 bad_fork_cancel_cgroup:
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1ed08967f..5c4232d45 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -39,6 +39,16 @@
 #define bpf_event_rcu_dereference(p)					\
 	rcu_dereference_protected(p, lockdep_is_held(&bpf_event_mutex))
 
+#ifdef CONFIG_BPF_HELPER_STRICT
+static bool check_bpf_bitfield(unsigned int flags)
+{
+	unsigned int bits = current->bpf_helper_bitfield;
+
+	if (!(bits & (1 << flags)))
+		return false;
+
+	return true;
+}
+#endif
+
 #ifdef CONFIG_MODULES
 struct bpf_trace_module {
 	struct module *module;
@@ -145,6 +155,10 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
 #ifdef CONFIG_BPF_KPROBE_OVERRIDE
 BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
 {
+#ifdef CONFIG_BPF_HELPER_STRICT
+	if (unlikely(!check_bpf_bitfield(BPF_OVERRIDE_RETURN_BIT)))
+		return -EPERM;
+#endif
 	regs_set_return_value(regs, rc);
 	override_function_with_return(regs);
 	return 0;
@@ -162,8 +176,8 @@ static const struct bpf_func_proto bpf_override_return_proto = {
 static __always_inline int
 bpf_probe_read_user_common(void *dst, u32 size, const void __user *unsafe_ptr)
 {
-	int ret;
 
+	int ret;
 	ret = copy_from_user_nofault(dst, unsafe_ptr, size);
 	if (unlikely(ret < 0))
 		memset(dst, 0, size);
@@ -287,6 +301,10 @@ const struct bpf_func_proto bpf_probe_read_kernel_str_proto = {
 BPF_CALL_3(bpf_probe_read_compat, void *, dst, u32, size,
 	   const void *, unsafe_ptr)
 {
+#ifdef CONFIG_BPF_HELPER_STRICT
+	if (unlikely(!check_bpf_bitfield(BPF_PROBE_READ_BIT)))
+		return -EPERM;
+#endif
 	if ((unsigned long)unsafe_ptr < TASK_SIZE) {
 		return bpf_probe_read_user_common(dst, size,
 				(__force void __user *)unsafe_ptr);
@@ -338,7 +356,10 @@ BPF_CALL_3(bpf_probe_write_user, void __user *, unsafe_ptr, const void *, src,
 	 * state, when the task or mm are switched. This is specifically
 	 * required to prevent the use of temporary mm.
 	 */
-
+#ifdef CONFIG_BPF_HELPER_STRICT
+	if (unlikely(!check_bpf_bitfield(BPF_PROBE_WRITE_BIT)))
+		return -EPERM;
+#endif
 	if (unlikely(in_interrupt() ||
 		     current->flags & (PF_KTHREAD | PF_EXITING)))
 		return -EPERM;
@@ -843,6 +864,10 @@ static int bpf_send_signal_common(u32 sig, enum pid_type type)
 	 * permitted in order to send signal to the current
 	 * task.
 	 */
+#ifdef CONFIG_BPF_HELPER_STRICT
+	if (unlikely(!check_bpf_bitfield(BPF_SEND_SIGNAL_BIT)))
+		return -EPERM;
+#endif
 	if (unlikely(current->flags & (PF_KTHREAD | PF_EXITING)))
 		return -EPERM;
 	if (unlikely(!nmi_uaccess_okay()))
-- 
2.25.1

Re: [PATCH] bpf: Add CONFIG_BPF_HELPER_STRICT

[Alexei Starovoitov]

On Fri, Jan 13, 2023 at 11:53 PM Roland <kernel.pwn@outlook.com> wrote:
>
> In container environment, ebpf helpers could be used maliciously to
>    leak information, DOS, even escape from containers.
>    CONFIG_BPF_HELPER_STRICT is as a mitigation of it.
>    Related Link: https://rolandorange.zone/report.html

The link is arguing that a process with CAP_SYS_ADMIN permissions
can read memory of user processes, leak kernel addresses, etc.
And this is somehow an issue with bpf helpers?
and your suggested "temporary mitigation" is to CONFIG_BPF=n ?
While this patch is a "proper fix" ?
Sorry, but please stay with your "temporary mitigation" forever.

Re: [PATCH] bpf: Add CONFIG_BPF_HELPER_STRICT

[KP Singh]

On Sun, Jan 15, 2023 at 10:32 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jan 13, 2023 at 11:53 PM Roland <kernel.pwn@outlook.com> wrote:
> >
> > In container environment, ebpf helpers could be used maliciously to
> >    leak information, DOS, even escape from containers.
> >    CONFIG_BPF_HELPER_STRICT is as a mitigation of it.
> >    Related Link: https://rolandorange.zone/report.html
>
> The link is arguing that a process with CAP_SYS_ADMIN permissions
> can read memory of user processes, leak kernel addresses, etc.
> And this is somehow an issue with bpf helpers?
> and your suggested "temporary mitigation" is to CONFIG_BPF=n ?
> While this patch is a "proper fix" ?
> Sorry, but please stay with your "temporary mitigation" forever.

100% agreeing with Alexei here, if you are running your containers
with CAP_SYS_ADMIN there are a lot of other things you need to worry
about than just BPF helpers. You need to revisit your threat model and
consider not using CAP_SYS_ADMIN and more fine grained policies using
Mandatory Access Control.

Re: [PATCH] bpf: Add CONFIG_BPF_HELPER_STRICT

[Alexei Starovoitov]

On Mon, Jan 16, 2023 at 10:25 PM Pwn Kernel <kernel.pwn@outlook.com> wrote:
>
> Thank you for your suggestion so much, we understand it.
>
> In the other hand, Do you think if add several lockdown reasons like LOCKDOWN_BPF_SEND_SIGNAL and  LOCKDOWN_BPF_OVERRIDE_RETURN will be better or acceptable for this?
>
> Thanks for your advice!

Please do not top post and don't use html in your replies.
Please learn the process first:
https://docs.kernel.org/process/development-process.html