[PATCH RFC v3 0/3] Rlimit for module space

[PATCH RFC v3 0/3] Rlimit for module space

[Rick Edgecombe]

If BPF JIT is on, there is no effective limit to prevent filling the entire
module space with JITed e/BPF filters. For classic BPF filters attached with
setsockopt SO_ATTACH_FILTER, there is no memlock rlimit check to limit the
number of insertions like there is for the bpf syscall.

This patch adds a per user rlimit for module space, as well as a system wide
limit for BPF JIT. In a previously reviewed patchset, Jann Horn pointed out the
problem that in some cases a user can get access to 65536 UIDs, so the effective
limit cannot be set low enough to stop an attacker and be useful for the general
case. A discussed alternative solution was a system wide limit for BPF JIT
filters. This much more simply resolves the problem of exhaustion and
de-randomizing in the case of non-CONFIG_BPF_JIT_ALWAYS_ON. If
CONFIG_BPF_JIT_ALWAYS_ON is on however, BPF insertions will fail if another user
exhausts the BPF JIT limit. In this case a per user limit is still needed. If
the subuid facility is disabled for normal users, this should still be ok
because the higher limit will not be able to be worked around that way.

The new BPF JIT limit can be set like this:
echo 5000000 > /proc/sys/net/core/bpf_jit_limit

So I *think* this patchset should resolve that issue except for the
configuration of CONFIG_BPF_JIT_ALWAYS_ON and subuid allowed for normal users.
Better module space KASLR is another way to resolve the de-randomizing issue,
and so then you would just be left with the BPF DOS in that configuration.

Jann also pointed out how, with purposely fragmenting the module space, you
could make the effective module space blockage area much larger. This is also
somewhat un-resolved. The impact would depend on how big of a space you are
trying to allocate. The limit has been lowered on x86_64 so that at least
typical sized BPF filters cannot be blocked.

If anyone with more experience with subuid/user namespaces has any suggestions
I'd be glad to hear. On an Ubuntu machine it didn't seem like a un-privileged
user could do this. I am going to keep working on this and see if I can find a
better solution.

Changes since v2:
 - System wide BPF JIT limit (discussion with Jann Horn)
 - Holding reference to user correctly (Jann)
 - Having arch versions of modulde_alloc (Dave Hansen, Jessica Yu)
 - Shrinking of default limits, to help prevent the limit being worked around
   with fragmentation (Jann)

Changes since v1:
 - Plug in for non-x86
 - Arch specific default values


Rick Edgecombe (3):
  modules: Create arch versions of module alloc/free
  modules: Create rlimit for module space
  bpf: Add system wide BPF JIT limit

 arch/arm/kernel/module.c                |   2 +-
 arch/arm64/kernel/module.c              |   2 +-
 arch/mips/kernel/module.c               |   2 +-
 arch/nds32/kernel/module.c              |   2 +-
 arch/nios2/kernel/module.c              |   4 +-
 arch/parisc/kernel/module.c             |   2 +-
 arch/s390/kernel/module.c               |   2 +-
 arch/sparc/kernel/module.c              |   2 +-
 arch/unicore32/kernel/module.c          |   2 +-
 arch/x86/include/asm/pgtable_32_types.h |   3 +
 arch/x86/include/asm/pgtable_64_types.h |   2 +
 arch/x86/kernel/module.c                |   2 +-
 fs/proc/base.c                          |   1 +
 include/asm-generic/resource.h          |   8 ++
 include/linux/bpf.h                     |   7 ++
 include/linux/filter.h                  |   1 +
 include/linux/sched/user.h              |   4 +
 include/uapi/asm-generic/resource.h     |   3 +-
 kernel/bpf/core.c                       |  22 +++-
 kernel/bpf/inode.c                      |  16 +++
 kernel/module.c                         | 152 +++++++++++++++++++++++-
 net/core/sysctl_net_core.c              |   7 ++
 22 files changed, 233 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] modules: Create arch versions of module alloc/free

[Rick Edgecombe]

In prep for module space rlimit, create a singular cross platform
module_alloc and module_memfree that call into arch specific
implementations.

This has only been tested on x86.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm/kernel/module.c       |  2 +-
 arch/arm64/kernel/module.c     |  2 +-
 arch/mips/kernel/module.c      |  2 +-
 arch/nds32/kernel/module.c     |  2 +-
 arch/nios2/kernel/module.c     |  4 ++--
 arch/parisc/kernel/module.c    |  2 +-
 arch/s390/kernel/module.c      |  2 +-
 arch/sparc/kernel/module.c     |  2 +-
 arch/unicore32/kernel/module.c |  2 +-
 arch/x86/kernel/module.c       |  2 +-
 kernel/module.c                | 14 ++++++++++++--
 11 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..359838a4bb06 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -38,7 +38,7 @@
 #endif
 
 #ifdef CONFIG_MMU
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f0f27aeefb73..a6891eb2fc16 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -30,7 +30,7 @@
 #include <asm/insn.h>
 #include <asm/sections.h>
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 491605137b03..e9ee8e7544f9 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -45,7 +45,7 @@ static LIST_HEAD(dbe_list);
 static DEFINE_SPINLOCK(dbe_lock);
 
 #ifdef MODULE_START
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	return __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
 				GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
diff --git a/arch/nds32/kernel/module.c b/arch/nds32/kernel/module.c
index 1e31829cbc2a..75535daa22a5 100644
--- a/arch/nds32/kernel/module.c
+++ b/arch/nds32/kernel/module.c
@@ -7,7 +7,7 @@
 #include <linux/moduleloader.h>
 #include <asm/pgtable.h>
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
 				    GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
diff --git a/arch/nios2/kernel/module.c b/arch/nios2/kernel/module.c
index e2e3f13f98d5..cd059a8e9a7b 100644
--- a/arch/nios2/kernel/module.c
+++ b/arch/nios2/kernel/module.c
@@ -28,7 +28,7 @@
  * from 0x80000000 (vmalloc area) to 0xc00000000 (kernel) (kmalloc returns
  * addresses in 0xc0000000)
  */
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	if (size == 0)
 		return NULL;
@@ -36,7 +36,7 @@ void *module_alloc(unsigned long size)
 }
 
 /* Free memory returned from module_alloc */
-void module_memfree(void *module_region)
+void arch_module_memfree(void *module_region)
 {
 	kfree(module_region);
 }
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index b5b3cb00f1fb..72ab3c8b103b 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -213,7 +213,7 @@ static inline int reassemble_22(int as22)
 		((as22 & 0x0003ff) << 3));
 }
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	/* using RWX means less protection for modules, but it's
 	 * easier than trying to map the text, data, init_text and
diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index d298d3cb46d0..e07c4a9384c0 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -30,7 +30,7 @@
 
 #define PLT_ENTRY_SIZE 20
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
diff --git a/arch/sparc/kernel/module.c b/arch/sparc/kernel/module.c
index df39580f398d..870581ba9205 100644
--- a/arch/sparc/kernel/module.c
+++ b/arch/sparc/kernel/module.c
@@ -40,7 +40,7 @@ static void *module_map(unsigned long size)
 }
 #endif /* CONFIG_SPARC64 */
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	void *ret;
 
diff --git a/arch/unicore32/kernel/module.c b/arch/unicore32/kernel/module.c
index e191b3448bd3..53ea96459d8c 100644
--- a/arch/unicore32/kernel/module.c
+++ b/arch/unicore32/kernel/module.c
@@ -22,7 +22,7 @@
 #include <asm/pgtable.h>
 #include <asm/sections.h>
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
 				GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index f58336af095c..032e49180577 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -77,7 +77,7 @@ static unsigned long int get_module_load_offset(void)
 }
 #endif
 
-void *module_alloc(unsigned long size)
+void *arch_module_alloc(unsigned long size)
 {
 	void *p;
 
diff --git a/kernel/module.c b/kernel/module.c
index 6746c85511fe..41c22aba8209 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2110,11 +2110,16 @@ static void free_module_elf(struct module *mod)
 }
 #endif /* CONFIG_LIVEPATCH */
 
-void __weak module_memfree(void *module_region)
+void __weak arch_module_memfree(void *module_region)
 {
 	vfree(module_region);
 }
 
+void module_memfree(void *module_region)
+{
+	arch_module_memfree(module_region);
+}
+
 void __weak module_arch_cleanup(struct module *mod)
 {
 }
@@ -2728,11 +2733,16 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
 		ddebug_remove_module(mod->name);
 }
 
-void * __weak module_alloc(unsigned long size)
+void * __weak arch_module_alloc(unsigned long size)
 {
 	return vmalloc_exec(size);
 }
 
+void *module_alloc(unsigned long size)
+{
+	return arch_module_alloc(size);
+}
+
 #ifdef CONFIG_DEBUG_KMEMLEAK
 static void kmemleak_load_module(const struct module *mod,
 				 const struct load_info *info)
-- 
2.17.1

[PATCH v3 2/3] modules: Create rlimit for module space

[Rick Edgecombe]

This introduces a new rlimit, RLIMIT_MODSPACE, which limits the amount of
module space a user can use. The intention is to be able to limit module space
allocations that may come from un-privlidged users inserting e/BPF filters.

Since filters attached to sockets can be passed to other processes via domain
sockets and freed there, there is new tracking for the uid of each allocation.
This way if the allocation is freed by a different user, it will not throw off
the accounting.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/include/asm/pgtable_32_types.h |   3 +
 arch/x86/include/asm/pgtable_64_types.h |   2 +
 fs/proc/base.c                          |   1 +
 include/asm-generic/resource.h          |   8 ++
 include/linux/sched/user.h              |   4 +
 include/uapi/asm-generic/resource.h     |   3 +-
 kernel/module.c                         | 140 +++++++++++++++++++++++-
 7 files changed, 159 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_32_types.h b/arch/x86/include/asm/pgtable_32_types.h
index b0bc0fff5f1f..185e382fa8c3 100644
--- a/arch/x86/include/asm/pgtable_32_types.h
+++ b/arch/x86/include/asm/pgtable_32_types.h
@@ -68,6 +68,9 @@ extern bool __vmalloc_start_set; /* set once high_memory is set */
 #define MODULES_END	VMALLOC_END
 #define MODULES_LEN	(MODULES_VADDR - MODULES_END)
 
+/* Half of 128MB vmalloc space */
+#define MODSPACE_LIMIT (1 << 25)
+
 #define MAXMEM	(VMALLOC_END - PAGE_OFFSET - __VMALLOC_RESERVE)
 
 #endif /* _ASM_X86_PGTABLE_32_DEFS_H */
diff --git a/arch/x86/include/asm/pgtable_64_types.h b/arch/x86/include/asm/pgtable_64_types.h
index 04edd2d58211..39288812be5a 100644
--- a/arch/x86/include/asm/pgtable_64_types.h
+++ b/arch/x86/include/asm/pgtable_64_types.h
@@ -143,6 +143,8 @@ extern unsigned int ptrs_per_p4d;
 #define MODULES_END		_AC(0xffffffffff000000, UL)
 #define MODULES_LEN		(MODULES_END - MODULES_VADDR)
 
+#define MODSPACE_LIMIT		(MODULES_LEN / 10)
+
 #define ESPFIX_PGD_ENTRY	_AC(-2, UL)
 #define ESPFIX_BASE_ADDR	(ESPFIX_PGD_ENTRY << P4D_SHIFT)
 
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 7e9f07bf260d..84824f50e9f8 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -562,6 +562,7 @@ static const struct limit_names lnames[RLIM_NLIMITS] = {
 	[RLIMIT_NICE] = {"Max nice priority", NULL},
 	[RLIMIT_RTPRIO] = {"Max realtime priority", NULL},
 	[RLIMIT_RTTIME] = {"Max realtime timeout", "us"},
+	[RLIMIT_MODSPACE] = {"Max module space", "bytes"},
 };
 
 /* Display limits for a process */
diff --git a/include/asm-generic/resource.h b/include/asm-generic/resource.h
index 8874f681b056..94c150e3dd12 100644
--- a/include/asm-generic/resource.h
+++ b/include/asm-generic/resource.h
@@ -4,6 +4,13 @@
 
 #include <uapi/asm-generic/resource.h>
 
+/*
+ * If the module space rlimit is not defined in an arch specific way, leave
+ * room for 10000 large eBPF filters.
+ */
+#ifndef MODSPACE_LIMIT
+#define MODSPACE_LIMIT (5*PAGE_SIZE*10000)
+#endif
 
 /*
  * boot-time rlimit defaults for the init task:
@@ -26,6 +33,7 @@
 	[RLIMIT_NICE]		= { 0, 0 },				\
 	[RLIMIT_RTPRIO]		= { 0, 0 },				\
 	[RLIMIT_RTTIME]		= {  RLIM_INFINITY,  RLIM_INFINITY },	\
+	[RLIMIT_MODSPACE]	= {  MODSPACE_LIMIT,  MODSPACE_LIMIT },	\
 }
 
 #endif
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index 39ad98c09c58..4c6d99d066fe 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -44,6 +44,10 @@ struct user_struct {
 	atomic_long_t locked_vm;
 #endif
 
+#ifdef CONFIG_MODULES
+	atomic_long_t module_vm;
+#endif
+
 	/* Miscellaneous per-user rate limit */
 	struct ratelimit_state ratelimit;
 };
diff --git a/include/uapi/asm-generic/resource.h b/include/uapi/asm-generic/resource.h
index f12db7a0da64..3f998340ed30 100644
--- a/include/uapi/asm-generic/resource.h
+++ b/include/uapi/asm-generic/resource.h
@@ -46,7 +46,8 @@
 					   0-39 for nice level 19 .. -20 */
 #define RLIMIT_RTPRIO		14	/* maximum realtime priority */
 #define RLIMIT_RTTIME		15	/* timeout for RT tasks in us */
-#define RLIM_NLIMITS		16
+#define RLIMIT_MODSPACE		16	/* max module space address usage */
+#define RLIM_NLIMITS		17
 
 /*
  * SuS says limits have to be unsigned.
diff --git a/kernel/module.c b/kernel/module.c
index 41c22aba8209..c26ad50365dd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2110,6 +2110,134 @@ static void free_module_elf(struct module *mod)
 }
 #endif /* CONFIG_LIVEPATCH */
 
+struct mod_alloc_user {
+	struct rb_node node;
+	unsigned long addr;
+	unsigned long pages;
+	struct user_struct *user;
+};
+
+static struct rb_root alloc_users = RB_ROOT;
+static DEFINE_SPINLOCK(alloc_users_lock);
+
+static unsigned int get_mod_page_cnt(unsigned long size)
+{
+	/* Add one for guard page */
+	return (PAGE_ALIGN(size) >> PAGE_SHIFT) + 1;
+}
+
+void update_mod_rlimit(void *addr, unsigned long size)
+{
+	unsigned long addrl = (unsigned long) addr;
+	struct rb_node **new = &(alloc_users.rb_node), *parent = NULL;
+	struct mod_alloc_user *track = kmalloc(sizeof(struct mod_alloc_user),
+				GFP_KERNEL);
+	unsigned int pages = get_mod_page_cnt(size);
+	struct user_struct *user = get_current_user();
+
+	/*
+	 * If addr is NULL, then we need to reverse the earlier increment that
+	 * would have happened in an check_inc_mod_rlimit call.
+	 */
+	if (!addr) {
+		atomic_long_sub(pages, &user->module_vm);
+		free_uid(user);
+		return;
+	}
+
+	/* Now, add tracking for the uid that allocated this */
+	track->addr = addrl;
+	track->pages = pages;
+	track->user = user;
+
+	spin_lock(&alloc_users_lock);
+
+	while (*new) {
+		struct mod_alloc_user *cur =
+				rb_entry(*new, struct mod_alloc_user, node);
+		parent = *new;
+		if (cur->addr > addrl)
+			new = &(*new)->rb_left;
+		else
+			new = &(*new)->rb_right;
+	}
+
+	rb_link_node(&(track->node), parent, new);
+	rb_insert_color(&(track->node), &alloc_users);
+
+	spin_unlock(&alloc_users_lock);
+}
+
+/* Remove user allocation tracking, return NULL if allocation untracked */
+static struct user_struct *remove_user_alloc(void *addr, unsigned long *pages)
+{
+	struct rb_node *cur_node = alloc_users.rb_node;
+	unsigned long addrl = (unsigned long) addr;
+	struct mod_alloc_user *cur_alloc_user = NULL;
+	struct user_struct *user;
+
+	spin_lock(&alloc_users_lock);
+	while (cur_node) {
+		cur_alloc_user =
+			rb_entry(cur_node, struct mod_alloc_user, node);
+		if (cur_alloc_user->addr > addrl)
+			cur_node = cur_node->rb_left;
+		else if (cur_alloc_user->addr < addrl)
+			cur_node = cur_node->rb_right;
+		else
+			goto found;
+	}
+	spin_unlock(&alloc_users_lock);
+
+	return NULL;
+found:
+	rb_erase(&cur_alloc_user->node, &alloc_users);
+	spin_unlock(&alloc_users_lock);
+
+	user = cur_alloc_user->user;
+	*pages = cur_alloc_user->pages;
+	kfree(cur_alloc_user);
+
+	return user;
+}
+
+int check_inc_mod_rlimit(unsigned long size)
+{
+	struct user_struct *user = get_current_user();
+	unsigned long modspace_pages = rlimit(RLIMIT_MODSPACE) >> PAGE_SHIFT;
+	unsigned long cur_pages = atomic_long_read(&user->module_vm);
+	unsigned long new_pages = get_mod_page_cnt(size);
+
+	if (rlimit(RLIMIT_MODSPACE) != RLIM_INFINITY
+			&& cur_pages + new_pages > modspace_pages) {
+		free_uid(user);
+		return 1;
+	}
+
+	atomic_long_add(new_pages, &user->module_vm);
+
+	if (atomic_long_read(&user->module_vm) > modspace_pages) {
+		atomic_long_sub(new_pages, &user->module_vm);
+		free_uid(user);
+		return 1;
+	}
+
+	free_uid(user);
+	return 0;
+}
+
+void dec_mod_rlimit(void *addr)
+{
+	unsigned long pages;
+	struct user_struct *user = remove_user_alloc(addr, &pages);
+
+	if (!user)
+		return;
+
+	atomic_long_sub(pages, &user->module_vm);
+	free_uid(user);
+}
+
 void __weak arch_module_memfree(void *module_region)
 {
 	vfree(module_region);
@@ -2118,6 +2246,7 @@ void __weak arch_module_memfree(void *module_region)
 void module_memfree(void *module_region)
 {
 	arch_module_memfree(module_region);
+	dec_mod_rlimit(module_region);
 }
 
 void __weak module_arch_cleanup(struct module *mod)
@@ -2740,7 +2869,16 @@ void * __weak arch_module_alloc(unsigned long size)
 
 void *module_alloc(unsigned long size)
 {
-	return arch_module_alloc(size);
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = arch_module_alloc(size);
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 #ifdef CONFIG_DEBUG_KMEMLEAK
-- 
2.17.1

[PATCH v3 3/3] bpf: Add system wide BPF JIT limit

[Rick Edgecombe]

In case of games played with multiple users, also add a system wide limit
(in bytes) for BPF JIT. The default intends to be big enough for 10000 BPF JIT
filters. This cannot help with the DOS in the case of
CONFIG_BPF_JIT_ALWAYS_ON, but it can help with DOS for module space and
with forcing a module to be loaded at a paticular address.

The limit can be set like this:
echo 5000000 > /proc/sys/net/core/bpf_jit_limit

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 include/linux/bpf.h        |  7 +++++++
 include/linux/filter.h     |  1 +
 kernel/bpf/core.c          | 22 +++++++++++++++++++++-
 kernel/bpf/inode.c         | 16 ++++++++++++++++
 net/core/sysctl_net_core.c |  7 +++++++
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 523481a3471b..4d7b729a1fe7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -827,4 +827,11 @@ extern const struct bpf_func_proto bpf_get_local_storage_proto;
 void bpf_user_rnd_init_once(void);
 u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 
+#ifndef MOD_BPF_LIMIT_DEFAULT
+/*
+ * Leave room for 10000 large eBPF filters as default.
+ */
+#define MOD_BPF_LIMIT_DEFAULT (5 * PAGE_SIZE * 10000)
+#endif
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 6791a0ac0139..3e91ffc7962b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -854,6 +854,7 @@ bpf_run_sk_reuseport(struct sock_reuseport *reuse, struct sock *sk,
 extern int bpf_jit_enable;
 extern int bpf_jit_harden;
 extern int bpf_jit_kallsyms;
+extern int bpf_jit_limit;
 
 typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 3f5bf1af0826..12c20fa6f04b 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -369,6 +369,9 @@ void bpf_prog_kallsyms_del_all(struct bpf_prog *fp)
 int bpf_jit_enable   __read_mostly = IS_BUILTIN(CONFIG_BPF_JIT_ALWAYS_ON);
 int bpf_jit_harden   __read_mostly;
 int bpf_jit_kallsyms __read_mostly;
+int bpf_jit_limit __read_mostly;
+
+static atomic_long_t module_vm;
 
 static __always_inline void
 bpf_get_prog_addr_region(const struct bpf_prog *prog,
@@ -583,17 +586,31 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 		     bpf_jit_fill_hole_t bpf_fill_ill_insns)
 {
 	struct bpf_binary_header *hdr;
-	unsigned int size, hole, start;
+	unsigned int size, hole, start, vpages;
 
 	/* Most of BPF filters are really small, but if some of them
 	 * fill a page, allow at least 128 extra bytes to insert a
 	 * random section of illegal instructions.
 	 */
 	size = round_up(proglen + sizeof(*hdr) + 128, PAGE_SIZE);
+
+	/* Size plus a guard page */
+	vpages = (PAGE_ALIGN(size) >> PAGE_SHIFT) + 1;
+
+	if (atomic_long_read(&module_vm) + vpages > bpf_jit_limit >> PAGE_SHIFT)
+		return NULL;
+
 	hdr = module_alloc(size);
 	if (hdr == NULL)
 		return NULL;
 
+	atomic_long_add(vpages, &module_vm);
+
+	if (atomic_long_read(&module_vm) > bpf_jit_limit >> PAGE_SHIFT) {
+		bpf_jit_binary_free(hdr);
+		return NULL;
+	}
+
 	/* Fill space with illegal/arch-dep instructions. */
 	bpf_fill_ill_insns(hdr, size);
 
@@ -610,7 +627,10 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 
 void bpf_jit_binary_free(struct bpf_binary_header *hdr)
 {
+	/* Size plus the guard page */
+	unsigned int vpages = hdr->pages + 1;
 	module_memfree(hdr);
+	atomic_long_sub(vpages, &module_vm);
 }
 
 /* This symbol is only overridden by archs that have different
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 2ada5e21dfa6..d0a109733294 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -667,10 +667,26 @@ static struct file_system_type bpf_fs_type = {
 	.kill_sb	= kill_litter_super,
 };
 
+#ifdef CONFIG_BPF_JIT
+void set_bpf_jit_limit(void)
+{
+	bpf_jit_limit = MOD_BPF_LIMIT_DEFAULT;
+}
+#else
+void set_bpf_jit_limit(void)
+{
+}
+#endif
+
 static int __init bpf_init(void)
 {
 	int ret;
 
+	/*
+	 * Module space size can be non-compile time constant so set it here.
+	 */
+	set_bpf_jit_limit();
+
 	ret = sysfs_create_mount_point(fs_kobj, "bpf");
 	if (ret)
 		return ret;
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index b1a2c5e38530..6bdf4a3da2b2 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -396,6 +396,13 @@ static struct ctl_table net_core_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+	{
+		.procname	= "bpf_jit_limit",
+		.data		= &bpf_jit_limit,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler	= proc_dointvec,
+	},
 # endif
 #endif
 	{
-- 
2.17.1

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Ard Biesheuvel]

Hi Rick,

On 19 October 2018 at 22:47, Rick Edgecombe <rick.p.edgecombe@intel.com> wrote:
> If BPF JIT is on, there is no effective limit to prevent filling the entire
> module space with JITed e/BPF filters.

Why do BPF filters use the module space, and does this reason apply to
all architectures?

On arm64, we already support loading plain modules far away from the
core kernel (i.e. out of range for ordinary relative jump/calll
instructions), and so I'd expect BPF to be able to deal with this
already as well. So for arm64, I wonder why an ordinary vmalloc_exec()
wouldn't be more appropriate.

So before refactoring the module alloc/free routines to accommodate
BPF, I'd like to take one step back and assess whether it wouldn't be
more appropriate to have a separate bpf_alloc/free API, which could be
totally separate from module alloc/free if the arch permits it.


> For classic BPF filters attached with
> setsockopt SO_ATTACH_FILTER, there is no memlock rlimit check to limit the
> number of insertions like there is for the bpf syscall.
>
> This patch adds a per user rlimit for module space, as well as a system wide
> limit for BPF JIT. In a previously reviewed patchset, Jann Horn pointed out the
> problem that in some cases a user can get access to 65536 UIDs, so the effective
> limit cannot be set low enough to stop an attacker and be useful for the general
> case. A discussed alternative solution was a system wide limit for BPF JIT
> filters. This much more simply resolves the problem of exhaustion and
> de-randomizing in the case of non-CONFIG_BPF_JIT_ALWAYS_ON. If
> CONFIG_BPF_JIT_ALWAYS_ON is on however, BPF insertions will fail if another user
> exhausts the BPF JIT limit. In this case a per user limit is still needed. If
> the subuid facility is disabled for normal users, this should still be ok
> because the higher limit will not be able to be worked around that way.
>
> The new BPF JIT limit can be set like this:
> echo 5000000 > /proc/sys/net/core/bpf_jit_limit
>
> So I *think* this patchset should resolve that issue except for the
> configuration of CONFIG_BPF_JIT_ALWAYS_ON and subuid allowed for normal users.
> Better module space KASLR is another way to resolve the de-randomizing issue,
> and so then you would just be left with the BPF DOS in that configuration.
>
> Jann also pointed out how, with purposely fragmenting the module space, you
> could make the effective module space blockage area much larger. This is also
> somewhat un-resolved. The impact would depend on how big of a space you are
> trying to allocate. The limit has been lowered on x86_64 so that at least
> typical sized BPF filters cannot be blocked.
>
> If anyone with more experience with subuid/user namespaces has any suggestions
> I'd be glad to hear. On an Ubuntu machine it didn't seem like a un-privileged
> user could do this. I am going to keep working on this and see if I can find a
> better solution.
>
> Changes since v2:
>  - System wide BPF JIT limit (discussion with Jann Horn)
>  - Holding reference to user correctly (Jann)
>  - Having arch versions of modulde_alloc (Dave Hansen, Jessica Yu)
>  - Shrinking of default limits, to help prevent the limit being worked around
>    with fragmentation (Jann)
>
> Changes since v1:
>  - Plug in for non-x86
>  - Arch specific default values
>
>
> Rick Edgecombe (3):
>   modules: Create arch versions of module alloc/free
>   modules: Create rlimit for module space
>   bpf: Add system wide BPF JIT limit
>
>  arch/arm/kernel/module.c                |   2 +-
>  arch/arm64/kernel/module.c              |   2 +-
>  arch/mips/kernel/module.c               |   2 +-
>  arch/nds32/kernel/module.c              |   2 +-
>  arch/nios2/kernel/module.c              |   4 +-
>  arch/parisc/kernel/module.c             |   2 +-
>  arch/s390/kernel/module.c               |   2 +-
>  arch/sparc/kernel/module.c              |   2 +-
>  arch/unicore32/kernel/module.c          |   2 +-
>  arch/x86/include/asm/pgtable_32_types.h |   3 +
>  arch/x86/include/asm/pgtable_64_types.h |   2 +
>  arch/x86/kernel/module.c                |   2 +-
>  fs/proc/base.c                          |   1 +
>  include/asm-generic/resource.h          |   8 ++
>  include/linux/bpf.h                     |   7 ++
>  include/linux/filter.h                  |   1 +
>  include/linux/sched/user.h              |   4 +
>  include/uapi/asm-generic/resource.h     |   3 +-
>  kernel/bpf/core.c                       |  22 +++-
>  kernel/bpf/inode.c                      |  16 +++
>  kernel/module.c                         | 152 +++++++++++++++++++++++-
>  net/core/sysctl_net_core.c              |   7 ++
>  22 files changed, 233 insertions(+), 15 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Edgecombe, Rick P]

On Sat, 2018-10-20 at 19:20 +0200, Ard Biesheuvel wrote:
> Hi Rick,
> 
> On 19 October 2018 at 22:47, Rick Edgecombe <rick.p.edgecombe@intel.com>
> wrote:
> > If BPF JIT is on, there is no effective limit to prevent filling the entire
> > module space with JITed e/BPF filters.
> 
> Why do BPF filters use the module space, and does this reason apply to
> all architectures?
> 
> On arm64, we already support loading plain modules far away from the
> core kernel (i.e. out of range for ordinary relative jump/calll
> instructions), and so I'd expect BPF to be able to deal with this
> already as well. So for arm64, I wonder why an ordinary vmalloc_exec()
> wouldn't be more appropriate.
AFAIK, it's like you said about relative instruction limits, but also because
some predictors don't predict past a certain distance. So performance as well.
Not sure the reasons for each arch, or if they all apply for BPF JIT. There seem
to be 8 by my count, that have a dedicated module space for some reason.

> So before refactoring the module alloc/free routines to accommodate
> BPF, I'd like to take one step back and assess whether it wouldn't be
> more appropriate to have a separate bpf_alloc/free API, which could be
> totally separate from module alloc/free if the arch permits it.
> 
I am not a BPF JIT expert unfortunately, hopefully someone more authoritative
will chime in. I only ran into this because I was trying to increase
randomization for the module space and wanted to find out how many allocations
needed to be supported.

I'd guess though, that BPF JIT is just assuming that there will be some arch
specific constraints about where text can be placed optimally and they would
already be taken into account in the module space allocator.

If there are no constraints for some arch, I'd wonder why not just update its
module_alloc to use the whole space available. What exactly are the constraints
for arm64 for normal modules?

It seems fine to me for architectures to have the option of solving this a
different way. If potentially the rlimit ends up being the best solution for
some architectures though, do you think this refactor (pretty close to just a
name change) is that intrusive?

I guess it could be just a BPF JIT per user limit and not touch module space,
but I thought the module space limit was a more general solution as that is the
actual limited resource.

> > For classic BPF filters attached with
> > setsockopt SO_ATTACH_FILTER, there is no memlock rlimit check to limit the
> > number of insertions like there is for the bpf syscall.
> > 
> > This patch adds a per user rlimit for module space, as well as a system wide
> > limit for BPF JIT. In a previously reviewed patchset, Jann Horn pointed out
> > the
> > problem that in some cases a user can get access to 65536 UIDs, so the
> > effective
> > limit cannot be set low enough to stop an attacker and be useful for the
> > general
> > case. A discussed alternative solution was a system wide limit for BPF JIT
> > filters. This much more simply resolves the problem of exhaustion and
> > de-randomizing in the case of non-CONFIG_BPF_JIT_ALWAYS_ON. If
> > CONFIG_BPF_JIT_ALWAYS_ON is on however, BPF insertions will fail if another
> > user
> > exhausts the BPF JIT limit. In this case a per user limit is still needed.
> > If
> > the subuid facility is disabled for normal users, this should still be ok
> > because the higher limit will not be able to be worked aroThey might und
> > that way.
> > 
> > The new BPF JIT limit can be set like this:
> > echo 5000000 > /proc/sys/net/core/bpf_jit_limit
> > 
> > So I *think* this patchset should resolve that issue except for the
> > configuration of CONFIG_BPF_JIT_ALWAYS_ON and subuid allowed for normal
> > users.
> > Better module space KASLR is another way to resolve the de-randomizing
> > issue,
> > and so then you would just be left with the BPF DOS in that configuration.
> > 
> > Jann also pointed out how, with purposely fragmenting the module space, you
> > could make the effective module space blockage area much larger. This is
> > also
> > somewhat un-resolved. The impact would depend on how big of a space you are
> > trying to allocate. The limit has been lowered on x86_64 so that at least
> > typical sized BPF filters cannot be blocked.
> > 
> > If anyone with more experience with subuid/user namespaces has any
> > suggestions
> > I'd be glad to hear. On an Ubuntu machine it didn't seem like a un-
> > privileged
> > user could do this. I am going to keep working on this and see if I can find
> > a
> > better solution.
> > 
> > Changes since v2:
> >  - System wide BPF JIT limit (discussion with Jann Horn)
> >  - Holding reference to user correctly (Jann)
> >  - Having arch versions of modulde_alloc (Dave Hansen, Jessica Yu)
> >  - Shrinking of default limits, to help prevent the limit being worked
> > around
> >    with fragmentation (Jann)
> > 
> > Changes since v1:
> >  - Plug in for non-x86
> >  - Arch specific default values
> > 
> > 
> > Rick Edgecombe (3):
> >   modules: Create arch versions of module alloc/free
> >   modules: Create rlimit for module space
> >   bpf: Add system wide BPF JIT limit
> > 
> >  arch/arm/kernel/module.c                |   2 +-
> >  arch/arm64/kernel/module.c              |   2 +-
> >  arch/mips/kernel/module.c               |   2 +-
> >  arch/nds32/kernel/module.c              |   2 +-
> >  arch/nios2/kernel/module.c              |   4 +-
> >  arch/parisc/kernel/module.c             |   2 +-
> >  arch/s390/kernel/module.c               |   2 +-
> >  arch/sparc/kernel/module.c              |   2 +-
> >  arch/unicore32/kernel/module.c          |   2 +-
> >  arch/x86/include/asm/pgtable_32_types.h |   3 +
> >  arch/x86/include/asm/pgtable_64_types.h |   2 +
> >  arch/x86/kernel/module.c                |   2 +-
> >  fs/proc/base.c                          |   1 +
> >  include/asm-generic/resource.h          |   8 ++
> >  include/linux/bpf.h                     |   7 ++
> >  include/linux/filter.h                  |   1 +
> >  include/linux/sched/user.h              |   4 +
> >  include/uapi/asm-generic/resource.h     |   3 +-
> >  kernel/bpf/core.c                       |  22 +++-
> >  kernel/bpf/inode.c                      |  16 +++
> >  kernel/module.c                         | 152 +++++++++++++++++++++++-
> >  net/core/sysctl_net_core.c              |   7 ++
> >  22 files changed, 233 insertions(+), 15 deletions(-)
> > 
> > --
> > 2.17.1
> > 
branching predictors may not be able to predict past a certain distance. So
performance as well.

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Ard Biesheuvel]

On 22 October 2018 at 20:06, Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Sat, 2018-10-20 at 19:20 +0200, Ard Biesheuvel wrote:
>> Hi Rick,
>>
>> On 19 October 2018 at 22:47, Rick Edgecombe <rick.p.edgecombe@intel.com>
>> wrote:
>> > If BPF JIT is on, there is no effective limit to prevent filling the entire
>> > module space with JITed e/BPF filters.
>>
>> Why do BPF filters use the module space, and does this reason apply to
>> all architectures?
>>
>> On arm64, we already support loading plain modules far away from the
>> core kernel (i.e. out of range for ordinary relative jump/calll
>> instructions), and so I'd expect BPF to be able to deal with this
>> already as well. So for arm64, I wonder why an ordinary vmalloc_exec()
>> wouldn't be more appropriate.
> AFAIK, it's like you said about relative instruction limits, but also because
> some predictors don't predict past a certain distance. So performance as well.
> Not sure the reasons for each arch, or if they all apply for BPF JIT. There seem
> to be 8 by my count, that have a dedicated module space for some reason.
>
>> So before refactoring the module alloc/free routines to accommodate
>> BPF, I'd like to take one step back and assess whether it wouldn't be
>> more appropriate to have a separate bpf_alloc/free API, which could be
>> totally separate from module alloc/free if the arch permits it.
>>
> I am not a BPF JIT expert unfortunately, hopefully someone more authoritative
> will chime in. I only ran into this because I was trying to increase
> randomization for the module space and wanted to find out how many allocations
> needed to be supported.
>
> I'd guess though, that BPF JIT is just assuming that there will be some arch
> specific constraints about where text can be placed optimally and they would
> already be taken into account in the module space allocator.
>
> If there are no constraints for some arch, I'd wonder why not just update its
> module_alloc to use the whole space available. What exactly are the constraints
> for arm64 for normal modules?
>

Relative branches and the interactions with KAsan.

We just fixed something similar in the kprobes code: it was using
RWX-mapped module memory to store kprobed instructions, and we
replaced that with a simple vmalloc_exec() [and code to remap it
read-only], which was possible given that relative branches are always
emulated by arm64 kprobes.

So it depends on whether BPF code needs to be in relative branching
range from the calling code, and whether the BPF code itself is
emitted using relative branches into other parts of the code.

> It seems fine to me for architectures to have the option of solving this a
> different way. If potentially the rlimit ends up being the best solution for
> some architectures though, do you think this refactor (pretty close to just a
> name change) is that intrusive?
>
> I guess it could be just a BPF JIT per user limit and not touch module space,
> but I thought the module space limit was a more general solution as that is the
> actual limited resource.
>

I think it is wrong to conflate the two things. Limiting the number of
BPF allocations and the limiting number of module allocations are two
separate things, and the fact that BPF reuses module_alloc() out of
convenience does not mean a single rlimit for both is appropriate.


>> > For classic BPF filters attached with
>> > setsockopt SO_ATTACH_FILTER, there is no memlock rlimit check to limit the
>> > number of insertions like there is for the bpf syscall.
>> >
>> > This patch adds a per user rlimit for module space, as well as a system wide
>> > limit for BPF JIT. In a previously reviewed patchset, Jann Horn pointed out
>> > the
>> > problem that in some cases a user can get access to 65536 UIDs, so the
>> > effective
>> > limit cannot be set low enough to stop an attacker and be useful for the
>> > general
>> > case. A discussed alternative solution was a system wide limit for BPF JIT
>> > filters. This much more simply resolves the problem of exhaustion and
>> > de-randomizing in the case of non-CONFIG_BPF_JIT_ALWAYS_ON. If
>> > CONFIG_BPF_JIT_ALWAYS_ON is on however, BPF insertions will fail if another
>> > user
>> > exhausts the BPF JIT limit. In this case a per user limit is still needed.
>> > If
>> > the subuid facility is disabled for normal users, this should still be ok
>> > because the higher limit will not be able to be worked aroThey might und
>> > that way.
>> >
>> > The new BPF JIT limit can be set like this:
>> > echo 5000000 > /proc/sys/net/core/bpf_jit_limit
>> >
>> > So I *think* this patchset should resolve that issue except for the
>> > configuration of CONFIG_BPF_JIT_ALWAYS_ON and subuid allowed for normal
>> > users.
>> > Better module space KASLR is another way to resolve the de-randomizing
>> > issue,
>> > and so then you would just be left with the BPF DOS in that configuration.
>> >
>> > Jann also pointed out how, with purposely fragmenting the module space, you
>> > could make the effective module space blockage area much larger. This is
>> > also
>> > somewhat un-resolved. The impact would depend on how big of a space you are
>> > trying to allocate. The limit has been lowered on x86_64 so that at least
>> > typical sized BPF filters cannot be blocked.
>> >
>> > If anyone with more experience with subuid/user namespaces has any
>> > suggestions
>> > I'd be glad to hear. On an Ubuntu machine it didn't seem like a un-
>> > privileged
>> > user could do this. I am going to keep working on this and see if I can find
>> > a
>> > better solution.
>> >
>> > Changes since v2:
>> >  - System wide BPF JIT limit (discussion with Jann Horn)
>> >  - Holding reference to user correctly (Jann)
>> >  - Having arch versions of modulde_alloc (Dave Hansen, Jessica Yu)
>> >  - Shrinking of default limits, to help prevent the limit being worked
>> > around
>> >    with fragmentation (Jann)
>> >
>> > Changes since v1:
>> >  - Plug in for non-x86
>> >  - Arch specific default values
>> >
>> >
>> > Rick Edgecombe (3):
>> >   modules: Create arch versions of module alloc/free
>> >   modules: Create rlimit for module space
>> >   bpf: Add system wide BPF JIT limit
>> >
>> >  arch/arm/kernel/module.c                |   2 +-
>> >  arch/arm64/kernel/module.c              |   2 +-
>> >  arch/mips/kernel/module.c               |   2 +-
>> >  arch/nds32/kernel/module.c              |   2 +-
>> >  arch/nios2/kernel/module.c              |   4 +-
>> >  arch/parisc/kernel/module.c             |   2 +-
>> >  arch/s390/kernel/module.c               |   2 +-
>> >  arch/sparc/kernel/module.c              |   2 +-
>> >  arch/unicore32/kernel/module.c          |   2 +-
>> >  arch/x86/include/asm/pgtable_32_types.h |   3 +
>> >  arch/x86/include/asm/pgtable_64_types.h |   2 +
>> >  arch/x86/kernel/module.c                |   2 +-
>> >  fs/proc/base.c                          |   1 +
>> >  include/asm-generic/resource.h          |   8 ++
>> >  include/linux/bpf.h                     |   7 ++
>> >  include/linux/filter.h                  |   1 +
>> >  include/linux/sched/user.h              |   4 +
>> >  include/uapi/asm-generic/resource.h     |   3 +-
>> >  kernel/bpf/core.c                       |  22 +++-
>> >  kernel/bpf/inode.c                      |  16 +++
>> >  kernel/module.c                         | 152 +++++++++++++++++++++++-
>> >  net/core/sysctl_net_core.c              |   7 ++
>> >  22 files changed, 233 insertions(+), 15 deletions(-)
>> >
>> > --
>> > 2.17.1
>> >
> branching predictors may not be able to predict past a certain distance. So
> performance as well.

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Jessica Yu]

+++ Ard Biesheuvel [23/10/18 08:54 -0300]:
>On 22 October 2018 at 20:06, Edgecombe, Rick P
><rick.p.edgecombe@intel.com> wrote:
>> On Sat, 2018-10-20 at 19:20 +0200, Ard Biesheuvel wrote:
>>> Hi Rick,
>>>
>>> On 19 October 2018 at 22:47, Rick Edgecombe <rick.p.edgecombe@intel.com>
>>> wrote:
>>> > If BPF JIT is on, there is no effective limit to prevent filling the entire
>>> > module space with JITed e/BPF filters.
>>>
>>> Why do BPF filters use the module space, and does this reason apply to
>>> all architectures?
>>>
>>> On arm64, we already support loading plain modules far away from the
>>> core kernel (i.e. out of range for ordinary relative jump/calll
>>> instructions), and so I'd expect BPF to be able to deal with this
>>> already as well. So for arm64, I wonder why an ordinary vmalloc_exec()
>>> wouldn't be more appropriate.
>> AFAIK, it's like you said about relative instruction limits, but also because
>> some predictors don't predict past a certain distance. So performance as well.
>> Not sure the reasons for each arch, or if they all apply for BPF JIT. There seem
>> to be 8 by my count, that have a dedicated module space for some reason.
>>
>>> So before refactoring the module alloc/free routines to accommodate
>>> BPF, I'd like to take one step back and assess whether it wouldn't be
>>> more appropriate to have a separate bpf_alloc/free API, which could be
>>> totally separate from module alloc/free if the arch permits it.
>>>
>> I am not a BPF JIT expert unfortunately, hopefully someone more authoritative
>> will chime in. I only ran into this because I was trying to increase
>> randomization for the module space and wanted to find out how many allocations
>> needed to be supported.
>>
>> I'd guess though, that BPF JIT is just assuming that there will be some arch
>> specific constraints about where text can be placed optimally and they would
>> already be taken into account in the module space allocator.
>>
>> If there are no constraints for some arch, I'd wonder why not just update its
>> module_alloc to use the whole space available. What exactly are the constraints
>> for arm64 for normal modules?
>>
>
>Relative branches and the interactions with KAsan.
>
>We just fixed something similar in the kprobes code: it was using
>RWX-mapped module memory to store kprobed instructions, and we
>replaced that with a simple vmalloc_exec() [and code to remap it
>read-only], which was possible given that relative branches are always
>emulated by arm64 kprobes.
>
>So it depends on whether BPF code needs to be in relative branching
>range from the calling code, and whether the BPF code itself is
>emitted using relative branches into other parts of the code.
>
>> It seems fine to me for architectures to have the option of solving this a
>> different way. If potentially the rlimit ends up being the best solution for
>> some architectures though, do you think this refactor (pretty close to just a
>> name change) is that intrusive?
>>
>> I guess it could be just a BPF JIT per user limit and not touch module space,
>> but I thought the module space limit was a more general solution as that is the
>> actual limited resource.
>>
>
>I think it is wrong to conflate the two things. Limiting the number of
>BPF allocations and the limiting number of module allocations are two
>separate things, and the fact that BPF reuses module_alloc() out of
>convenience does not mean a single rlimit for both is appropriate.

Hm, I think Ard has a good point. AFAIK, and correct me if I'm wrong,
users of module_alloc() i.e. kprobes, ftrace, bpf, seem to use it
because it is an easy way to obtain executable kernel memory (and
depending on the needs of the architecture, being additionally
reachable via relative branches) during runtime. The side effect is
that all these users share the "module" memory space, even though this
memory region is not exclusively used by modules (well, personally I
think it technically should be, because seeing module_alloc() usage
outside of the module loader is kind of a misuse of the module API and
it's confusing for people who don't know the reason behind its usage
outside of the module loader).

Right now I'm not sure if it makes sense to impose a blanket limit on
all module_alloc() allocations when the real motivation behind the
rlimit is related to BPF, i.e., to stop unprivileged users from
hogging up all the vmalloc space for modules with JITed BPF filters.
So the rlimit has more to do with limiting the memory usage of BPF
filters than it has to do with modules themselves.

I think Ard's suggestion of having a separate bpf_alloc/free API makes
a lot of sense if we want to keep track of bpf-related allocations
(and then the rlimit would be enforced for those). Maybe part of the
module mapping space could be carved out for bpf filters (e.g. have
BPF_VADDR, BPF_VSIZE, etc like how we have it for modules), or
continue sharing the region but explicitly define a separate bpf_alloc
API, depending on an architecture's needs. What do people think?

Thanks,

Jessica

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Daniel Borkmann]

[ +Alexei, netdev ]

On 10/24/2018 05:07 PM, Jessica Yu wrote:
> +++ Ard Biesheuvel [23/10/18 08:54 -0300]:
>> On 22 October 2018 at 20:06, Edgecombe, Rick P
>> <rick.p.edgecombe@intel.com> wrote:
[...]
>> I think it is wrong to conflate the two things. Limiting the number of
>> BPF allocations and the limiting number of module allocations are two
>> separate things, and the fact that BPF reuses module_alloc() out of
>> convenience does not mean a single rlimit for both is appropriate.
> 
> Hm, I think Ard has a good point. AFAIK, and correct me if I'm wrong,
> users of module_alloc() i.e. kprobes, ftrace, bpf, seem to use it
> because it is an easy way to obtain executable kernel memory (and
> depending on the needs of the architecture, being additionally
> reachable via relative branches) during runtime. The side effect is
> that all these users share the "module" memory space, even though this
> memory region is not exclusively used by modules (well, personally I
> think it technically should be, because seeing module_alloc() usage
> outside of the module loader is kind of a misuse of the module API and
> it's confusing for people who don't know the reason behind its usage
> outside of the module loader).
> 
> Right now I'm not sure if it makes sense to impose a blanket limit on
> all module_alloc() allocations when the real motivation behind the
> rlimit is related to BPF, i.e., to stop unprivileged users from
> hogging up all the vmalloc space for modules with JITed BPF filters.
> So the rlimit has more to do with limiting the memory usage of BPF
> filters than it has to do with modules themselves.
> 
> I think Ard's suggestion of having a separate bpf_alloc/free API makes
> a lot of sense if we want to keep track of bpf-related allocations
> (and then the rlimit would be enforced for those). Maybe part of the
> module mapping space could be carved out for bpf filters (e.g. have
> BPF_VADDR, BPF_VSIZE, etc like how we have it for modules), or
> continue sharing the region but explicitly define a separate bpf_alloc
> API, depending on an architecture's needs. What do people think?

Hmm, I think here are several issues mixed up at the same time which is just
very confusing, imho:

1) The fact that there are several non-module users of module_alloc()
as Jessica notes such as kprobes, ftrace, bpf, for example. While all of
them are not being modules, they all need to alloc some piece of executable
memory. It's nothing new, this exists for 7 years now since 0a14842f5a3c
("net: filter: Just In Time compiler for x86-64") from BPF side; effectively
that is even /before/ eBPF existed. Having some different API perhaps for all
these users seems to make sense if the goal is not to interfere with modules
themselves. It might also help as a benefit to potentially increase that
memory pool if we're hitting limits at scale which would not be a concern
for normal kernel modules since there's usually just a very few of them
needed (unlike dynamically tracing various kernel parts 24/7 w/ or w/o BPF,
running BPF-seccomp policies, networking BPF policies, etc which need to
scale w/ application or container deployment; so this is of much more
dynamic and unpredictable nature).

2) Then there is rlimit which is proposing to have a "fairer" share among
unprivileged users. I'm not fully sure yet whether rlimit is actually a
nice usable interface for all this. I'd agree that something is needed
on that regard, but I also tend to like Michal Hocko's cgroup proposal,
iow, to have such resource control as part of memory cgroup could be
something to consider _especially_ since it already _exists_ for vmalloc()
backed memory so this should be not much different than that. It sounds
like 2) comes on top of 1).

3) Last but not least, there's a short term fix which is needed independently
of 1) and 2) and should be done immediately which is to account for
unprivileged users and restrict them based on a global configurable
limit such that privileged use keeps functioning, and 2) could enforce
based on the global upper limit, for example. Pending fix is under
https://patchwork.ozlabs.org/patch/987971/ which we intend to ship to
Linus as soon as possible as short term fix. Then something like memcg
can be considered on top since it seems this makes most sense from a
usability point.

Thanks a lot,
Daniel

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Edgecombe, Rick P]

On Wed, 2018-10-24 at 18:04 +0200, Daniel Borkmann wrote:
> [ +Alexei, netdev ]
> 
> On 10/24/2018 05:07 PM, Jessica Yu wrote:
> > +++ Ard Biesheuvel [23/10/18 08:54 -0300]:
> > > On 22 October 2018 at 20:06, Edgecombe, Rick P
> > > <rick.p.edgecombe@intel.com> wrote:
> 
> [...]
> > > I think it is wrong to conflate the two things. Limiting the number of
> > > BPF allocations and the limiting number of module allocations are two
> > > separate things, and the fact that BPF reuses module_alloc() out of
> > > convenience does not mean a single rlimit for both is appropriate.
> > 
> > Hm, I think Ard has a good point. AFAIK, and correct me if I'm wrong,
> > users of module_alloc() i.e. kprobes, ftrace, bpf, seem to use it
> > because it is an easy way to obtain executable kernel memory (and
> > depending on the needs of the architecture, being additionally
> > reachable via relative branches) during runtime. The side effect is
> > that all these users share the "module" memory space, even though this
> > memory region is not exclusively used by modules (well, personally I
> > think it technically should be, because seeing module_alloc() usage
> > outside of the module loader is kind of a misuse of the module API and
> > it's confusing for people who don't know the reason behind its usage
> > outside of the module loader).
> > 
> > Right now I'm not sure if it makes sense to impose a blanket limit on
> > all module_alloc() allocations when the real motivation behind the
> > rlimit is related to BPF, i.e., to stop unprivileged users from
> > hogging up all the vmalloc space for modules with JITed BPF filters.
> > So the rlimit has more to do with limiting the memory usage of BPF
> > filters than it has to do with modules themselves.
> > 
> > I think Ard's suggestion of having a separate bpf_alloc/free API makes
> > a lot of sense if we want to keep track of bpf-related allocations
> > (and then the rlimit would be enforced for those). Maybe part of the
> > module mapping space could be carved out for bpf filters (e.g. have
> > BPF_VADDR, BPF_VSIZE, etc like how we have it for modules), or
> > continue sharing the region but explicitly define a separate bpf_alloc
> > API, depending on an architecture's needs. What do people think?
> 
> Hmm, I think here are several issues mixed up at the same time which is just
> very confusing, imho:
> 
> 1) The fact that there are several non-module users of module_alloc()
> as Jessica notes such as kprobes, ftrace, bpf, for example. While all of
> them are not being modules, they all need to alloc some piece of executable
> memory. It's nothing new, this exists for 7 years now since 0a14842f5a3c
> ("net: filter: Just In Time compiler for x86-64") from BPF side; effectively
> that is even /before/ eBPF existed. Having some different API perhaps for all
> these users seems to make sense if the goal is not to interfere with modules
> themselves. It might also help as a benefit to potentially increase that
> memory pool if we're hitting limits at scale which would not be a concern
> for normal kernel modules since there's usually just a very few of them
> needed (unlike dynamically tracing various kernel parts 24/7 w/ or w/o BPF,
> running BPF-seccomp policies, networking BPF policies, etc which need to
> scale w/ application or container deployment; so this is of much more
> dynamic and unpredictable nature).
> 
> 2) Then there is rlimit which is proposing to have a "fairer" share among
> unprivileged users. I'm not fully sure yet whether rlimit is actually a
> nice usable interface for all this. I'd agree that something is needed
> on that regard, but I also tend to like Michal Hocko's cgroup proposal,
> iow, to have such resource control as part of memory cgroup could be
> something to consider _especially_ since it already _exists_ for vmalloc()
> backed memory so this should be not much different than that. It sounds
> like 2) comes on top of 1).
FWIW, cgroups seems like a better solution than rlimit to me too. Maybe you all
already know, but it looks like the cgroups vmalloc charge is done in the main
page allocator and counts against the whole kernel memory limit. A user may want
to have a higher kernel limit than the module space size, so it seems it isn't
enough by itself and some new limit would need to be added.

As for what the limit should be, I wonder if some of the disagreement is just
from the name "module space".

There is a limited resource of physical memory, so we have limits for it. There
is a limited resource of CPU time, so we have limits for it. If there is a
limited resource for preferred address space for executable code, why not just
continue that trend? If other forms of unprivileged JIT come along, would it be
better to have N limits for each type? Request_module probably can't fill the
space, but technically there are already 2 unprivileged users. So IMHO, its a
more forward looking solution.

If there are some usage/architecture combos that don't need the preferred space
they can allocate in vmalloc and have it not count against the preferred space
limit but still count against the cgroups kernel memory limit.

Another benefit of centralizing the allocation of the "executable memory
preferred space" is KASLR. Right now that is only done in module_alloc and so
all users of it get randomized. If they all call vmalloc by themselves they will
just use the normal allocator.

Anyway, it seems like either type of limit (BPF JIT or all module space) will
solve the problem equally well today.

> 3) Last but not least, there's a short term fix which is needed independently
> of 1) and 2) and should be done immediately which is to account for
> unprivileged users and restrict them based on a global configurable
> limit such that privileged use keeps functioning, and 2) could enforce
> based on the global upper limit, for example. Pending fix is under
> https://patchwork.ozlabs.org/patch/987971/ which we intend to ship to
> Linus as soon as possible as short term fix. Then something like memcg
> can be considered on top since it seems this makes most sense from a
> usability point.
> 
> Thanks a lot,
> Daniel

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Michal Hocko]

On Thu 25-10-18 01:01:44, Edgecombe, Rick P wrote:
[...]
> FWIW, cgroups seems like a better solution than rlimit to me too. Maybe you all
> already know, but it looks like the cgroups vmalloc charge is done in the main
> page allocator and counts against the whole kernel memory limit.

I am not sure I understand what you are saying but let me clarify that
vmalloc memory is accounted against memory cgroup associated with the
user context calling vmalloc. All you need to do is to add __GFP_ACCOUNT
to the gfp mask. The only challenge here is the charged memory life
cycle. When does it get deallocated? In the worst case the memory is not
tight to any user context and as such it doesn't get deallocated by
killing all processes which could lead to memcg limit exhaustion.

> A user may want
> to have a higher kernel limit than the module space size, so it seems it isn't
> enough by itself and some new limit would need to be added.

If there is another restriction on this memory then you are right.
-- 
Michal Hocko
SUSE Labs