[PATCH v2 0/7] Rlimit for module space

[PATCH v2 0/7] Rlimit for module space

[Rick Edgecombe]

Hi,

This is v2 of a patch series that was first sent to security@kernel.org. The
recommendation was to pursue the fix on public lists. First I’ll describe the
issue that this is trying to solve, and then the general solution being
proposed, and lastly summarize the feedback so far. At a high level, this is
coming from a local DOS on eBPF, a KASLR module offset leak and desire for
general hardening of module space usage.

Problem
-------
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 this is for the bpf syscall. The cBPF gets converted
to eBPF and then handled by the JIT depending on if JIT is enabled. There is a
low enough default limit for open file descriptors per process, but this can be
worked around easily by forking before inserting. If the memlock rlimit is set
high for some other reason, eBPF programs inserted with the bpf syscall can also
exhaust the space.

This can cause problems not limited to:
 - Filling the entire module space with filters so that kernel modules cannot
   be loaded.
 - If CONFIG_BPF_JIT_ALWAYS_ON is configured, then if the module space is full,
   other BPF insertions will fail. This could cause filters that apps are
   relying on for security to fail to insert.
 - Counting the allocations until failure, since the module space is
   allocated linearly, the number of allocations can be used to de-randomize
   modules, with KASLR module base randomization. (This has been POCed with some
   assumptions)

Thanks to Daniel Borkmann for helping me understand what was happening with the
classic BPF JIT compilation and CONFIG_BPF_JIT_ALWAYS_ON config.

Proposed solution
-----------------
The solution being proposed here is to add a per user rlimit for module space,
similar to memlock rlimit. For the case of fds with attached filters being sent
over domain sockets, there is tracking for the uid of each module allocation.

Hopefully this could also be used for helping prevent BPF JIT spray type attacks
if a lower, more locked down setting is used.

The default memlock rlimit is pretty low, so just adding a check to classic BPF
similar to what happens in the bpf syscall may cause breakages. In addition,
some usages may increase the memlock limit for other reasons, which will remove
the protection for exhausting the module space.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch’s that implement module_alloc. These helpers
perform the checks and help with tracking. The intention is that they can be
added to the other arch’s as easily as possible.

For decrementing the module space usage when an area is free, there _is_ a
cross-platform place to do this, so its done there. The behavior is that if the
helpers to increment and check are not added into an arch’s module_alloc, then
the decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

Changes since v1 RFC
--------------------
Some feedback from Kees Cook was to try to plug this in for every architecture
and so this is done in this set for every architecture that has a BPF JIT
implementation. I have only done testing on x86.

There was also a suggestion from Daniel Borkmann to have default value for the
rlimit scale with the module size. This was complicated because the module space
size is not named the same accross architectures. It also is not always a
compile time constant and so the struct initilization would need to be changed.
So instead for this version a default value is added that can be overridden for
each architecture. For this set it is just defined for x86, all others get the
default.

Questions
---------
 - Should there be any special behavior for root or users with superuser
   capabilities?

Rick Edgecombe (7):
  modules: Create rlimit for module space
  x86/modules: Add rlimit checking for x86 modules
  arm/modules: Add rlimit checking for arm modules
  arm64/modules: Add rlimit checking for arm64 modules
  mips/modules: Add rlimit checking for mips modules
  sparc/modules: Add rlimit for sparc modules
  s390/modules: Add rlimit checking for s390 modules

 arch/arm/kernel/module.c                |  12 +-
 arch/arm64/kernel/module.c              |   5 +
 arch/mips/kernel/module.c               |  11 +-
 arch/s390/kernel/module.c               |  12 +-
 arch/sparc/kernel/module.c              |   5 +
 arch/x86/include/asm/pgtable_32_types.h |   3 +
 arch/x86/include/asm/pgtable_64_types.h |   2 +
 arch/x86/kernel/module.c                |   7 +-
 fs/proc/base.c                          |   1 +
 include/asm-generic/resource.h          |   8 ++
 include/linux/moduleloader.h            |   3 +
 include/linux/sched/user.h              |   4 +
 include/uapi/asm-generic/resource.h     |   3 +-
 kernel/module.c                         | 141 +++++++++++++++++++++++-
 14 files changed, 210 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 0/7] Rlimit for module space

[Rick Edgecombe]

Hi,

This is v2 of a patch series that was first sent to security@kernel.org. The
recommendation was to pursue the fix on public lists. First I’ll describe the
issue that this is trying to solve, and then the general solution being
proposed, and lastly summarize the feedback so far. At a high level, this is
coming from a local DOS on eBPF, a KASLR module offset leak and desire for
general hardening of module space usage.

Problem
-------
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 this is for the bpf syscall. The cBPF gets converted
to eBPF and then handled by the JIT depending on if JIT is enabled. There is a
low enough default limit for open file descriptors per process, but this can be
worked around easily by forking before inserting. If the memlock rlimit is set
high for some other reason, eBPF programs inserted with the bpf syscall can also
exhaust the space.

This can cause problems not limited to:
 - Filling the entire module space with filters so that kernel modules cannot
   be loaded.
 - If CONFIG_BPF_JIT_ALWAYS_ON is configured, then if the module space is full,
   other BPF insertions will fail. This could cause filters that apps are
   relying on for security to fail to insert.
 - Counting the allocations until failure, since the module space is
   allocated linearly, the number of allocations can be used to de-randomize
   modules, with KASLR module base randomization. (This has been POCed with some
   assumptions)

Thanks to Daniel Borkmann for helping me understand what was happening with the
classic BPF JIT compilation and CONFIG_BPF_JIT_ALWAYS_ON config.

Proposed solution
-----------------
The solution being proposed here is to add a per user rlimit for module space,
similar to memlock rlimit. For the case of fds with attached filters being sent
over domain sockets, there is tracking for the uid of each module allocation.

Hopefully this could also be used for helping prevent BPF JIT spray type attacks
if a lower, more locked down setting is used.

The default memlock rlimit is pretty low, so just adding a check to classic BPF
similar to what happens in the bpf syscall may cause breakages. In addition,
some usages may increase the memlock limit for other reasons, which will remove
the protection for exhausting the module space.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch’s that implement module_alloc. These helpers
perform the checks and help with tracking. The intention is that they can be
added to the other arch’s as easily as possible.

For decrementing the module space usage when an area is free, there _is_ a
cross-platform place to do this, so its done there. The behavior is that if the
helpers to increment and check are not added into an arch’s module_alloc, then
the decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

Changes since v1 RFC
--------------------
Some feedback from Kees Cook was to try to plug this in for every architecture
and so this is done in this set for every architecture that has a BPF JIT
implementation. I have only done testing on x86.

There was also a suggestion from Daniel Borkmann to have default value for the
rlimit scale with the module size. This was complicated because the module space
size is not named the same accross architectures. It also is not always a
compile time constant and so the struct initilization would need to be changed.
So instead for this version a default value is added that can be overridden for
each architecture. For this set it is just defined for x86, all others get the
default.

Questions
---------
 - Should there be any special behavior for root or users with superuser
   capabilities?

Rick Edgecombe (7):
  modules: Create rlimit for module space
  x86/modules: Add rlimit checking for x86 modules
  arm/modules: Add rlimit checking for arm modules
  arm64/modules: Add rlimit checking for arm64 modules
  mips/modules: Add rlimit checking for mips modules
  sparc/modules: Add rlimit for sparc modules
  s390/modules: Add rlimit checking for s390 modules

 arch/arm/kernel/module.c                |  12 +-
 arch/arm64/kernel/module.c              |   5 +
 arch/mips/kernel/module.c               |  11 +-
 arch/s390/kernel/module.c               |  12 +-
 arch/sparc/kernel/module.c              |   5 +
 arch/x86/include/asm/pgtable_32_types.h |   3 +
 arch/x86/include/asm/pgtable_64_types.h |   2 +
 arch/x86/kernel/module.c                |   7 +-
 fs/proc/base.c                          |   1 +
 include/asm-generic/resource.h          |   8 ++
 include/linux/moduleloader.h            |   3 +
 include/linux/sched/user.h              |   4 +
 include/uapi/asm-generic/resource.h     |   3 +-
 kernel/module.c                         | 141 +++++++++++++++++++++++-
 14 files changed, 210 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 0/7] Rlimit for module space

[Rick Edgecombe]

Hi,

This is v2 of a patch series that was first sent to security at kernel.org. The
recommendation was to pursue the fix on public lists. First I?ll describe the
issue that this is trying to solve, and then the general solution being
proposed, and lastly summarize the feedback so far. At a high level, this is
coming from a local DOS on eBPF, a KASLR module offset leak and desire for
general hardening of module space usage.

Problem
-------
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 this is for the bpf syscall. The cBPF gets converted
to eBPF and then handled by the JIT depending on if JIT is enabled. There is a
low enough default limit for open file descriptors per process, but this can be
worked around easily by forking before inserting. If the memlock rlimit is set
high for some other reason, eBPF programs inserted with the bpf syscall can also
exhaust the space.

This can cause problems not limited to:
 - Filling the entire module space with filters so that kernel modules cannot
   be loaded.
 - If CONFIG_BPF_JIT_ALWAYS_ON is configured, then if the module space is full,
   other BPF insertions will fail. This could cause filters that apps are
   relying on for security to fail to insert.
 - Counting the allocations until failure, since the module space is
   allocated linearly, the number of allocations can be used to de-randomize
   modules, with KASLR module base randomization. (This has been POCed with some
   assumptions)

Thanks to Daniel Borkmann for helping me understand what was happening with the
classic BPF JIT compilation and CONFIG_BPF_JIT_ALWAYS_ON config.

Proposed solution
-----------------
The solution being proposed here is to add a per user rlimit for module space,
similar to memlock rlimit. For the case of fds with attached filters being sent
over domain sockets, there is tracking for the uid of each module allocation.

Hopefully this could also be used for helping prevent BPF JIT spray type attacks
if a lower, more locked down setting is used.

The default memlock rlimit is pretty low, so just adding a check to classic BPF
similar to what happens in the bpf syscall may cause breakages. In addition,
some usages may increase the memlock limit for other reasons, which will remove
the protection for exhausting the module space.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch?s that implement module_alloc. These helpers
perform the checks and help with tracking. The intention is that they can be
added to the other arch?s as easily as possible.

For decrementing the module space usage when an area is free, there _is_ a
cross-platform place to do this, so its done there. The behavior is that if the
helpers to increment and check are not added into an arch?s module_alloc, then
the decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

Changes since v1 RFC
--------------------
Some feedback from Kees Cook was to try to plug this in for every architecture
and so this is done in this set for every architecture that has a BPF JIT
implementation. I have only done testing on x86.

There was also a suggestion from Daniel Borkmann to have default value for the
rlimit scale with the module size. This was complicated because the module space
size is not named the same accross architectures. It also is not always a
compile time constant and so the struct initilization would need to be changed.
So instead for this version a default value is added that can be overridden for
each architecture. For this set it is just defined for x86, all others get the
default.

Questions
---------
 - Should there be any special behavior for root or users with superuser
   capabilities?

Rick Edgecombe (7):
  modules: Create rlimit for module space
  x86/modules: Add rlimit checking for x86 modules
  arm/modules: Add rlimit checking for arm modules
  arm64/modules: Add rlimit checking for arm64 modules
  mips/modules: Add rlimit checking for mips modules
  sparc/modules: Add rlimit for sparc modules
  s390/modules: Add rlimit checking for s390 modules

 arch/arm/kernel/module.c                |  12 +-
 arch/arm64/kernel/module.c              |   5 +
 arch/mips/kernel/module.c               |  11 +-
 arch/s390/kernel/module.c               |  12 +-
 arch/sparc/kernel/module.c              |   5 +
 arch/x86/include/asm/pgtable_32_types.h |   3 +
 arch/x86/include/asm/pgtable_64_types.h |   2 +
 arch/x86/kernel/module.c                |   7 +-
 fs/proc/base.c                          |   1 +
 include/asm-generic/resource.h          |   8 ++
 include/linux/moduleloader.h            |   3 +
 include/linux/sched/user.h              |   4 +
 include/uapi/asm-generic/resource.h     |   3 +-
 kernel/module.c                         | 141 +++++++++++++++++++++++-
 14 files changed, 210 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 1/7] 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.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch’s that implement module_alloc. These
helpers perform the checks and help with tracking. The intention is that they
an be added to the various arch’s as easily as possible.

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.

For decrementing the module space usage when an area is free, there is a
cross-platform place to do this. The behavior is that if the helpers to
increment and check are not added into an arch’s module_alloc, then the
decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

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

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/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..206539e97579 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+int check_inc_mod_rlimit(unsigned long size);
+void update_mod_rlimit(void *addr, unsigned long size);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
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 6746c85511fe..2ef9ed95bf60 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2110,9 +2110,139 @@ 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;
+	kuid_t uid;
+};
+
+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);
+
+	/*
+	 * 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) {
+		struct user_struct *user = get_current_user();
+
+		atomic_long_sub(pages, &user->module_vm);
+		free_uid(user);
+		return;
+	}
+
+	/* Now, add tracking for the uid that allocated this */
+	track->uid = current_uid();
+	track->addr = addrl;
+	track->pages = pages;
+
+	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 = find_user(cur_alloc_user->uid);
+	*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 module_memfree(void *module_region)
 {
 	vfree(module_region);
+	dec_mod_rlimit(module_region);
 }
 
 void __weak module_arch_cleanup(struct module *mod)
@@ -2730,7 +2860,16 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
 
 void * __weak module_alloc(unsigned long size)
 {
-	return vmalloc_exec(size);
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = vmalloc_exec(size);
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 #ifdef CONFIG_DEBUG_KMEMLEAK
-- 
2.17.1

[PATCH v2 1/7] 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.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch’s that implement module_alloc. These
helpers perform the checks and help with tracking. The intention is that they
an be added to the various arch’s as easily as possible.

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.

For decrementing the module space usage when an area is free, there is a
cross-platform place to do this. The behavior is that if the helpers to
increment and check are not added into an arch’s module_alloc, then the
decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

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

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/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..206539e97579 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+int check_inc_mod_rlimit(unsigned long size);
+void update_mod_rlimit(void *addr, unsigned long size);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
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 6746c85511fe..2ef9ed95bf60 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2110,9 +2110,139 @@ 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;
+	kuid_t uid;
+};
+
+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);
+
+	/*
+	 * 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) {
+		struct user_struct *user = get_current_user();
+
+		atomic_long_sub(pages, &user->module_vm);
+		free_uid(user);
+		return;
+	}
+
+	/* Now, add tracking for the uid that allocated this */
+	track->uid = current_uid();
+	track->addr = addrl;
+	track->pages = pages;
+
+	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 = find_user(cur_alloc_user->uid);
+	*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 module_memfree(void *module_region)
 {
 	vfree(module_region);
+	dec_mod_rlimit(module_region);
 }
 
 void __weak module_arch_cleanup(struct module *mod)
@@ -2730,7 +2860,16 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
 
 void * __weak module_alloc(unsigned long size)
 {
-	return vmalloc_exec(size);
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = vmalloc_exec(size);
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 #ifdef CONFIG_DEBUG_KMEMLEAK
-- 
2.17.1

[PATCH v2 1/7] 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.

There is unfortunately no cross platform place to perform this accounting
during allocation in the module space, so instead two helpers are created to be
inserted into the various arch?s that implement module_alloc. These
helpers perform the checks and help with tracking. The intention is that they
an be added to the various arch?s as easily as possible.

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.

For decrementing the module space usage when an area is free, there is a
cross-platform place to do this. The behavior is that if the helpers to
increment and check are not added into an arch?s module_alloc, then the
decrement should have no effect. This is due to the allocation being missing
from the allocation-uid tracking.

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

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/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..206539e97579 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+int check_inc_mod_rlimit(unsigned long size);
+void update_mod_rlimit(void *addr, unsigned long size);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
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 6746c85511fe..2ef9ed95bf60 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2110,9 +2110,139 @@ 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;
+	kuid_t uid;
+};
+
+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);
+
+	/*
+	 * 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) {
+		struct user_struct *user = get_current_user();
+
+		atomic_long_sub(pages, &user->module_vm);
+		free_uid(user);
+		return;
+	}
+
+	/* Now, add tracking for the uid that allocated this */
+	track->uid = current_uid();
+	track->addr = addrl;
+	track->pages = pages;
+
+	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 = find_user(cur_alloc_user->uid);
+	*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 module_memfree(void *module_region)
 {
 	vfree(module_region);
+	dec_mod_rlimit(module_region);
 }
 
 void __weak module_arch_cleanup(struct module *mod)
@@ -2730,7 +2860,16 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
 
 void * __weak module_alloc(unsigned long size)
 {
-	return vmalloc_exec(size);
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = vmalloc_exec(size);
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 #ifdef CONFIG_DEBUG_KMEMLEAK
-- 
2.17.1

[PATCH v2 2/7] x86/modules: Add rlimit checking for x86 modules

[Rick Edgecombe]

This adds in the rlimit checking for the x86 module allocator.

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 ++
 arch/x86/kernel/module.c                | 7 ++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

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..c256931f4667 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 / 2)
+
 #define ESPFIX_PGD_ENTRY	_AC(-2, UL)
 #define ESPFIX_BASE_ADDR	(ESPFIX_PGD_ENTRY << P4D_SHIFT)
 
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index f58336af095c..5eb3f9c5a976 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -84,16 +84,21 @@ void *module_alloc(unsigned long size)
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN,
 				    MODULES_VADDR + get_module_load_offset(),
 				    MODULES_END, GFP_KERNEL,
 				    PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
 	if (p && (kasan_module_alloc(p, size) < 0)) {
-		vfree(p);
+		module_memfree(p);
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 2/7] x86/modules: Add rlimit checking for x86 modules

[Rick Edgecombe]

This adds in the rlimit checking for the x86 module allocator.

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 ++
 arch/x86/kernel/module.c                | 7 ++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

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..c256931f4667 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 / 2)
+
 #define ESPFIX_PGD_ENTRY	_AC(-2, UL)
 #define ESPFIX_BASE_ADDR	(ESPFIX_PGD_ENTRY << P4D_SHIFT)
 
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index f58336af095c..5eb3f9c5a976 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -84,16 +84,21 @@ void *module_alloc(unsigned long size)
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN,
 				    MODULES_VADDR + get_module_load_offset(),
 				    MODULES_END, GFP_KERNEL,
 				    PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
 	if (p && (kasan_module_alloc(p, size) < 0)) {
-		vfree(p);
+		module_memfree(p);
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 2/7] x86/modules: Add rlimit checking for x86 modules

[Rick Edgecombe]

This adds in the rlimit checking for the x86 module allocator.

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 ++
 arch/x86/kernel/module.c                | 7 ++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

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..c256931f4667 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 / 2)
+
 #define ESPFIX_PGD_ENTRY	_AC(-2, UL)
 #define ESPFIX_BASE_ADDR	(ESPFIX_PGD_ENTRY << P4D_SHIFT)
 
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index f58336af095c..5eb3f9c5a976 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -84,16 +84,21 @@ void *module_alloc(unsigned long size)
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN,
 				    MODULES_VADDR + get_module_load_offset(),
 				    MODULES_END, GFP_KERNEL,
 				    PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
 	if (p && (kasan_module_alloc(p, size) < 0)) {
-		vfree(p);
+		module_memfree(p);
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 3/7] arm/modules: Add rlimit checking for arm modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm/kernel/module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..e331863553d2 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -43,6 +43,9 @@ void *module_alloc(unsigned long size)
 	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	/* Silence the initial allocation */
 	if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
@@ -51,10 +54,15 @@ void *module_alloc(unsigned long size)
 				gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
 	if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
-		return p;
-	return __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
+		goto done;
+	p = __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
 				GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+done:
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 3/7] arm/modules: Add rlimit checking for arm modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm/kernel/module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..e331863553d2 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -43,6 +43,9 @@ void *module_alloc(unsigned long size)
 	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	/* Silence the initial allocation */
 	if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
@@ -51,10 +54,15 @@ void *module_alloc(unsigned long size)
 				gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
 	if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
-		return p;
-	return __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
+		goto done;
+	p = __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
 				GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+done:
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 3/7] arm/modules: Add rlimit checking for arm modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm/kernel/module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..e331863553d2 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -43,6 +43,9 @@ void *module_alloc(unsigned long size)
 	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	/* Silence the initial allocation */
 	if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
@@ -51,10 +54,15 @@ void *module_alloc(unsigned long size)
 				gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
 	if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
-		return p;
-	return __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
+		goto done;
+	p = __vmalloc_node_range(size, 1,  VMALLOC_START, VMALLOC_END,
 				GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+done:
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm64 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm64/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f0f27aeefb73..ea9794f2f571 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -39,6 +39,9 @@ void *module_alloc(unsigned long size)
 	if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
 				module_alloc_base + MODULES_VSIZE,
 				gfp_mask, PAGE_KERNEL_EXEC, 0,
@@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm64 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm64/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f0f27aeefb73..ea9794f2f571 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -39,6 +39,9 @@ void *module_alloc(unsigned long size)
 	if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
 				module_alloc_base + MODULES_VSIZE,
 				gfp_mask, PAGE_KERNEL_EXEC, 0,
@@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Rick Edgecombe]

This adds in the rlimit checking for the arm64 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/arm64/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index f0f27aeefb73..ea9794f2f571 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -39,6 +39,9 @@ void *module_alloc(unsigned long size)
 	if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
 		gfp_mask |= __GFP_NOWARN;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
 				module_alloc_base + MODULES_VSIZE,
 				gfp_mask, PAGE_KERNEL_EXEC, 0,
@@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
 		return NULL;
 	}
 
+	update_mod_rlimit(p, size);
+
 	return p;
 }
 
-- 
2.17.1

[PATCH v2 5/7] mips/modules: Add rlimit checking for mips modules

[Rick Edgecombe]

This adds in the rlimit checking for the mips module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/mips/kernel/module.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 491605137b03..7a23392512d1 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -47,9 +47,18 @@ static DEFINE_SPINLOCK(dbe_lock);
 #ifdef MODULE_START
 void *module_alloc(unsigned long size)
 {
-	return __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
 				GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 5/7] mips/modules: Add rlimit checking for mips modules

[Rick Edgecombe]

This adds in the rlimit checking for the mips module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/mips/kernel/module.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 491605137b03..7a23392512d1 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -47,9 +47,18 @@ static DEFINE_SPINLOCK(dbe_lock);
 #ifdef MODULE_START
 void *module_alloc(unsigned long size)
 {
-	return __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
 				GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 5/7] mips/modules: Add rlimit checking for mips modules

[Rick Edgecombe]

This adds in the rlimit checking for the mips module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/mips/kernel/module.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/module.c b/arch/mips/kernel/module.c
index 491605137b03..7a23392512d1 100644
--- a/arch/mips/kernel/module.c
+++ b/arch/mips/kernel/module.c
@@ -47,9 +47,18 @@ static DEFINE_SPINLOCK(dbe_lock);
 #ifdef MODULE_START
 void *module_alloc(unsigned long size)
 {
-	return __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
+	void *p;
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULE_START, MODULE_END,
 				GFP_KERNEL, PAGE_KERNEL, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 #endif
 
-- 
2.17.1

[PATCH v2 6/7] sparc/modules: Add rlimit for sparc modules

[Rick Edgecombe]

This adds in the rlimit checking for the sparc module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/sparc/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sparc/kernel/module.c b/arch/sparc/kernel/module.c
index df39580f398d..24854fdfa7c3 100644
--- a/arch/sparc/kernel/module.c
+++ b/arch/sparc/kernel/module.c
@@ -44,10 +44,15 @@ void *module_alloc(unsigned long size)
 {
 	void *ret;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	ret = module_map(size);
 	if (ret)
 		memset(ret, 0, size);
 
+	update_mod_rlimit(ret, size);
+
 	return ret;
 }
 
-- 
2.17.1

[PATCH v2 6/7] sparc/modules: Add rlimit for sparc modules

[Rick Edgecombe]

This adds in the rlimit checking for the sparc module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/sparc/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sparc/kernel/module.c b/arch/sparc/kernel/module.c
index df39580f398d..24854fdfa7c3 100644
--- a/arch/sparc/kernel/module.c
+++ b/arch/sparc/kernel/module.c
@@ -44,10 +44,15 @@ void *module_alloc(unsigned long size)
 {
 	void *ret;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	ret = module_map(size);
 	if (ret)
 		memset(ret, 0, size);
 
+	update_mod_rlimit(ret, size);
+
 	return ret;
 }
 
-- 
2.17.1

[PATCH v2 6/7] sparc/modules: Add rlimit for sparc modules

[Rick Edgecombe]

This adds in the rlimit checking for the sparc module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/sparc/kernel/module.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sparc/kernel/module.c b/arch/sparc/kernel/module.c
index df39580f398d..24854fdfa7c3 100644
--- a/arch/sparc/kernel/module.c
+++ b/arch/sparc/kernel/module.c
@@ -44,10 +44,15 @@ void *module_alloc(unsigned long size)
 {
 	void *ret;
 
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
 	ret = module_map(size);
 	if (ret)
 		memset(ret, 0, size);
 
+	update_mod_rlimit(ret, size);
+
 	return ret;
 }
 
-- 
2.17.1

[PATCH v2 7/7] s390/modules: Add rlimit checking for s390 modules

[Rick Edgecombe]

This adds in the rlimit checking for the s390 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/s390/kernel/module.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index d298d3cb46d0..6c2356a72b63 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -32,12 +32,22 @@
 
 void *module_alloc(unsigned long size)
 {
+	void *p;
+
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
-	return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
 				    GFP_KERNEL, PAGE_KERNEL_EXEC,
 				    0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 void module_arch_freeing_init(struct module *mod)
-- 
2.17.1

[PATCH v2 7/7] s390/modules: Add rlimit checking for s390 modules

[Rick Edgecombe]

This adds in the rlimit checking for the s390 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/s390/kernel/module.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index d298d3cb46d0..6c2356a72b63 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -32,12 +32,22 @@
 
 void *module_alloc(unsigned long size)
 {
+	void *p;
+
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
-	return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
 				    GFP_KERNEL, PAGE_KERNEL_EXEC,
 				    0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 void module_arch_freeing_init(struct module *mod)
-- 
2.17.1

[PATCH v2 7/7] s390/modules: Add rlimit checking for s390 modules

[Rick Edgecombe]

This adds in the rlimit checking for the s390 module allocator.

This has not been tested.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/s390/kernel/module.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index d298d3cb46d0..6c2356a72b63 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -32,12 +32,22 @@
 
 void *module_alloc(unsigned long size)
 {
+	void *p;
+
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
-	return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
+
+	if (check_inc_mod_rlimit(size))
+		return NULL;
+
+	p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
 				    GFP_KERNEL, PAGE_KERNEL_EXEC,
 				    0, NUMA_NO_NODE,
 				    __builtin_return_address(0));
+
+	update_mod_rlimit(p, size);
+
+	return p;
 }
 
 void module_arch_freeing_init(struct module *mod)
-- 
2.17.1

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Dave Hansen]

On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> +	if (check_inc_mod_rlimit(size))
> +		return NULL;
> +
>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>  				module_alloc_base + MODULES_VSIZE,
>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>  		return NULL;
>  	}
>  
> +	update_mod_rlimit(p, size);

Is there a reason we couldn't just rename all of the existing per-arch
module_alloc() calls to be called, say, "arch_module_alloc()", then put
this new rlimit code in a generic helper that does:


	if (check_inc_mod_rlimit(size))
		return NULL;

	p = arch_module_alloc(...);

	...

	update_mod_rlimit(p, size);

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Dave Hansen]

On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> +	if (check_inc_mod_rlimit(size))
> +		return NULL;
> +
>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>  				module_alloc_base + MODULES_VSIZE,
>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>  		return NULL;
>  	}
>  
> +	update_mod_rlimit(p, size);

Is there a reason we couldn't just rename all of the existing per-arch
module_alloc() calls to be called, say, "arch_module_alloc()", then put
this new rlimit code in a generic helper that does:


	if (check_inc_mod_rlimit(size))
		return NULL;

	p = arch_module_alloc(...);

	...

	update_mod_rlimit(p, size);

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Dave Hansen]

On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> +	if (check_inc_mod_rlimit(size))
> +		return NULL;
> +
>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>  				module_alloc_base + MODULES_VSIZE,
>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>  		return NULL;
>  	}
>  
> +	update_mod_rlimit(p, size);

Is there a reason we couldn't just rename all of the existing per-arch
module_alloc() calls to be called, say, "arch_module_alloc()", then put
this new rlimit code in a generic helper that does:


	if (check_inc_mod_rlimit(size))
		return NULL;

	p = arch_module_alloc(...);

	...

	update_mod_rlimit(p, size);

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
> 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.

Note that in some configurations (iirc e.g. the default Ubuntu
config), normal users can use the subuid mechanism (the /etc/subuid
config file and the /usr/bin/newuidmap setuid helper) to gain access
to 65536 UIDs, which means that in such a configuration,
RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
applies to RLIMIT_MEMLOCK.)

Also, it is probably possible to waste a few times as much virtual
memory as permitted by the limit by deliberately fragmenting virtual
memory?

> There is unfortunately no cross platform place to perform this accounting
> during allocation in the module space, so instead two helpers are created to be
> inserted into the various arch’s that implement module_alloc. These
> helpers perform the checks and help with tracking. The intention is that they
> an be added to the various arch’s as easily as possible.

nit: s/an/can/

[...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 6746c85511fe..2ef9ed95bf60 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2110,9 +2110,139 @@ 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;
> +       kuid_t uid;
> +};
> +
> +static struct rb_root alloc_users = RB_ROOT;
> +static DEFINE_SPINLOCK(alloc_users_lock);

Why all the rbtree stuff instead of stashing a pointer in struct
vmap_area, or something like that?

[...]
> +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);

If you drop the reference on the user_struct, an attacker with two
UIDs can charge module allocations to UID A, keep the associated
sockets alive as UID B, and then log out and back in again as UID A.
At that point, nobody is charged for the module space anymore. If you
look at the eBPF implementation, you'll see that
bpf_prog_charge_memlock() actually stores a refcounted pointer to the
user_struct.

> +       return 0;
> +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
> 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.

Note that in some configurations (iirc e.g. the default Ubuntu
config), normal users can use the subuid mechanism (the /etc/subuid
config file and the /usr/bin/newuidmap setuid helper) to gain access
to 65536 UIDs, which means that in such a configuration,
RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
applies to RLIMIT_MEMLOCK.)

Also, it is probably possible to waste a few times as much virtual
memory as permitted by the limit by deliberately fragmenting virtual
memory?

> There is unfortunately no cross platform place to perform this accounting
> during allocation in the module space, so instead two helpers are created to be
> inserted into the various arch’s that implement module_alloc. These
> helpers perform the checks and help with tracking. The intention is that they
> an be added to the various arch’s as easily as possible.

nit: s/an/can/

[...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 6746c85511fe..2ef9ed95bf60 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2110,9 +2110,139 @@ 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;
> +       kuid_t uid;
> +};
> +
> +static struct rb_root alloc_users = RB_ROOT;
> +static DEFINE_SPINLOCK(alloc_users_lock);

Why all the rbtree stuff instead of stashing a pointer in struct
vmap_area, or something like that?

[...]
> +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);

If you drop the reference on the user_struct, an attacker with two
UIDs can charge module allocations to UID A, keep the associated
sockets alive as UID B, and then log out and back in again as UID A.
At that point, nobody is charged for the module space anymore. If you
look at the eBPF implementation, you'll see that
bpf_prog_charge_memlock() actually stores a refcounted pointer to the
user_struct.

> +       return 0;
> +}

[PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
> 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.

Note that in some configurations (iirc e.g. the default Ubuntu
config), normal users can use the subuid mechanism (the /etc/subuid
config file and the /usr/bin/newuidmap setuid helper) to gain access
to 65536 UIDs, which means that in such a configuration,
RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
applies to RLIMIT_MEMLOCK.)

Also, it is probably possible to waste a few times as much virtual
memory as permitted by the limit by deliberately fragmenting virtual
memory?

> There is unfortunately no cross platform place to perform this accounting
> during allocation in the module space, so instead two helpers are created to be
> inserted into the various arch?s that implement module_alloc. These
> helpers perform the checks and help with tracking. The intention is that they
> an be added to the various arch?s as easily as possible.

nit: s/an/can/

[...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 6746c85511fe..2ef9ed95bf60 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2110,9 +2110,139 @@ 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;
> +       kuid_t uid;
> +};
> +
> +static struct rb_root alloc_users = RB_ROOT;
> +static DEFINE_SPINLOCK(alloc_users_lock);

Why all the rbtree stuff instead of stashing a pointer in struct
vmap_area, or something like that?

[...]
> +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);

If you drop the reference on the user_struct, an attacker with two
UIDs can charge module allocations to UID A, keep the associated
sockets alive as UID B, and then log out and back in again as UID A.
At that point, nobody is charged for the module space anymore. If you
look at the eBPF implementation, you'll see that
bpf_prog_charge_memlock() actually stores a refcounted pointer to the
user_struct.

> +       return 0;
> +}

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Jessica Yu]

+++ Dave Hansen [11/10/18 16:47 -0700]:
>On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
>> +	if (check_inc_mod_rlimit(size))
>> +		return NULL;
>> +
>>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>>  				module_alloc_base + MODULES_VSIZE,
>>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
>> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>>  		return NULL;
>>  	}
>>
>> +	update_mod_rlimit(p, size);
>
>Is there a reason we couldn't just rename all of the existing per-arch
>module_alloc() calls to be called, say, "arch_module_alloc()", then put
>this new rlimit code in a generic helper that does:
>
>
>	if (check_inc_mod_rlimit(size))
>		return NULL;
>
>	p = arch_module_alloc(...);
>
>	...
>
>	update_mod_rlimit(p, size);
>

I second this suggestion. Just make module_{alloc,memfree} generic,
non-weak functions that call the rlimit helpers in addition to the
arch-specific arch_module_{alloc,memfree} functions.

Jessica

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Jessica Yu]

+++ Dave Hansen [11/10/18 16:47 -0700]:
>On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
>> +	if (check_inc_mod_rlimit(size))
>> +		return NULL;
>> +
>>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>>  				module_alloc_base + MODULES_VSIZE,
>>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
>> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>>  		return NULL;
>>  	}
>>
>> +	update_mod_rlimit(p, size);
>
>Is there a reason we couldn't just rename all of the existing per-arch
>module_alloc() calls to be called, say, "arch_module_alloc()", then put
>this new rlimit code in a generic helper that does:
>
>
>	if (check_inc_mod_rlimit(size))
>		return NULL;
>
>	p = arch_module_alloc(...);
>
>	...
>
>	update_mod_rlimit(p, size);
>

I second this suggestion. Just make module_{alloc,memfree} generic,
non-weak functions that call the rlimit helpers in addition to the
arch-specific arch_module_{alloc,memfree} functions.

Jessica

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Jessica Yu]

+++ Dave Hansen [11/10/18 16:47 -0700]:
>On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
>> +	if (check_inc_mod_rlimit(size))
>> +		return NULL;
>> +
>>  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>>  				module_alloc_base + MODULES_VSIZE,
>>  				gfp_mask, PAGE_KERNEL_EXEC, 0,
>> @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
>>  		return NULL;
>>  	}
>>
>> +	update_mod_rlimit(p, size);
>
>Is there a reason we couldn't just rename all of the existing per-arch
>module_alloc() calls to be called, say, "arch_module_alloc()", then put
>this new rlimit code in a generic helper that does:
>
>
>	if (check_inc_mod_rlimit(size))
>		return NULL;
>
>	p = arch_module_alloc(...);
>
>	...
>
>	update_mod_rlimit(p, size);
>

I second this suggestion. Just make module_{alloc,memfree} generic,
non-weak functions that call the rlimit helpers in addition to the
arch-specific arch_module_{alloc,memfree} functions.

Jessica

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
> 
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)
Ah, that is a problem. There is only room for about 130,000 filters on x86 with
KASLR, so it couldn't really be set small enough.

I'll have to look into what this is. Thanks for pointing it out.

> Also, it is probably possible to waste a few times as much virtual
> memory as permitted by the limit by deliberately fragmenting virtual
> memory?
Good point. I guess if the first point can be addressed somehow, this one could
maybe be solved by just picking a lower limit.

Any thoughts on if instead of all this there was just a system wide limit on BPF
JIT module space usage?

> > There is unfortunately no cross platform place to perform this accounting
> > during allocation in the module space, so instead two helpers are created to
> > be
> > inserted into the various arch’s that implement module_alloc. These
> > helpers perform the checks and help with tracking. The intention is that
> > they
> > an be added to the various arch’s as easily as possible.
> 
> nit: s/an/can/
> 
> [...]
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 6746c85511fe..2ef9ed95bf60 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> >  }
> >  #endif /* CONFIG_LIVEPATCH */it 
> > 
> > +struct mod_alloc_user {
> > +       struct rb_node node;
> > +       unsigned long addr;
> > +       unsigned long pages;
> > +       kuid_t uid;
> > +};
> > +
> > +static struct rb_root alloc_users = RB_ROOT;
> > +static DEFINE_SPINLOCK(alloc_users_lock);
> 
> Why all the rbtree stuff instead of stashing a pointer in struct
> vmap_area, or something like that?

Since the tracking was not for all vmalloc usage, the intention was to not bloat
the structure for other usages likes stacks. I thought usually there wouldn't be
nearly as much module space allocations as there would be kernel stacks, but I
didn't do any actual measurements on the tradeoffs.

> [...]
> > +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);
> 
> If you drop the reference on the user_struct, an attacker with two
> UIDs can charge module allocations to UID A, keep the associated
> sockets alive as UID B, and then log out and back in again as UID A.
> At that point, nobody is charged for the module space anymore. If you
> look at the eBPF implementation, you'll see that
> bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> user_struct.
Ok, I'll take a look. Thanks Jann.
> > +       return 0;
> > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
> 
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)
Ah, that is a problem. There is only room for about 130,000 filters on x86 with
KASLR, so it couldn't really be set small enough.

I'll have to look into what this is. Thanks for pointing it out.

> Also, it is probably possible to waste a few times as much virtual
> memory as permitted by the limit by deliberately fragmenting virtual
> memory?
Good point. I guess if the first point can be addressed somehow, this one could
maybe be solved by just picking a lower limit.

Any thoughts on if instead of all this there was just a system wide limit on BPF
JIT module space usage?

> > There is unfortunately no cross platform place to perform this accounting
> > during allocation in the module space, so instead two helpers are created to
> > be
> > inserted into the various arch’s that implement module_alloc. These
> > helpers perform the checks and help with tracking. The intention is that
> > they
> > an be added to the various arch’s as easily as possible.
> 
> nit: s/an/can/
> 
> [...]
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 6746c85511fe..2ef9ed95bf60 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> >  }
> >  #endif /* CONFIG_LIVEPATCH */it 
> > 
> > +struct mod_alloc_user {
> > +       struct rb_node node;
> > +       unsigned long addr;
> > +       unsigned long pages;
> > +       kuid_t uid;
> > +};
> > +
> > +static struct rb_root alloc_users = RB_ROOT;
> > +static DEFINE_SPINLOCK(alloc_users_lock);
> 
> Why all the rbtree stuff instead of stashing a pointer in struct
> vmap_area, or something like that?

Since the tracking was not for all vmalloc usage, the intention was to not bloat
the structure for other usages likes stacks. I thought usually there wouldn't be
nearly as much module space allocations as there would be kernel stacks, but I
didn't do any actual measurements on the tradeoffs.

> [...]
> > +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);
> 
> If you drop the reference on the user_struct, an attacker with two
> UIDs can charge module allocations to UID A, keep the associated
> sockets alive as UID B, and then log out and back in again as UID A.
> At that point, nobody is charged for the module space anymore. If you
> look at the eBPF implementation, you'll see that
> bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> user_struct.
Ok, I'll take a look. Thanks Jann.
> > +       return 0;
> > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
> 
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)
Ah, that is a problem. There is only room for about 130,000 filters on x86 with
KASLR, so it couldn't really be set small enough.

I'll have to look into what this is. Thanks for pointing it out.

> Also, it is probably possible to waste a few times as much virtual
> memory as permitted by the limit by deliberately fragmenting virtual
> memory?
Good point. I guess if the first point can be addressed somehow, this one could
maybe be solved by just picking a lower limit.

Any thoughts on if instead of all this there was just a system wide limit on BPF
JIT module space usage?

> > There is unfortunately no cross platform place to perform this accounting
> > during allocation in the module space, so instead two helpers are created to
> > be
> > inserted into the various arch’s that implement module_alloc. These
> > helpers perform the checks and help with tracking. The intention is that
> > they
> > an be added to the various arch’s as easily as possible.
> 
> nit: s/an/can/
> 
> [...]
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 6746c85511fe..2ef9ed95bf60 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> >  }
> >  #endif /* CONFIG_LIVEPATCH */it 
> > 
> > +struct mod_alloc_user {
> > +       struct rb_node node;
> > +       unsigned long addr;
> > +       unsigned long pages;
> > +       kuid_t uid;
> > +};
> > +
> > +static struct rb_root alloc_users = RB_ROOT;
> > +static DEFINE_SPINLOCK(alloc_users_lock);
> 
> Why all the rbtree stuff instead of stashing a pointer in struct
> vmap_area, or something like that?

Since the tracking was not for all vmalloc usage, the intention was to not bloat
the structure for other usages likes stacks. I thought usually there wouldn't be
nearly as much module space allocations as there would be kernel stacks, but I
didn't do any actual measurements on the tradeoffs.

> [...]
> > +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);
> 
> If you drop the reference on the user_struct, an attacker with two
> UIDs can charge module allocations to UID A, keep the associated
> sockets alive as UID B, and then log out and back in again as UID A.
> At that point, nobody is charged for the module space anymore. If you
> look at the eBPF implementation, you'll see that
> bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> user_struct.
Ok, I'll take a look. Thanks Jann.
> > +       return 0;
> > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

T24gRnJpLCAyMDE4LTEwLTEyIGF0IDAyOjM1ICswMjAwLCBKYW5uIEhvcm4gd3JvdGU6DQo+IE9u
IEZyaSwgT2N0IDEyLCAyMDE4IGF0IDE6NDAgQU0gUmljayBFZGdlY29tYmUNCj4gPHJpY2sucC5l
ZGdlY29tYmVAaW50ZWwuY29tPiB3cm90ZToNCj4gPiBUaGlzIGludHJvZHVjZXMgYSBuZXcgcmxp
bWl0LCBSTElNSVRfTU9EU1BBQ0UsIHdoaWNoIGxpbWl0cyB0aGUgYW1vdW50IG9mDQo+ID4gbW9k
dWxlIHNwYWNlIGEgdXNlciBjYW4gdXNlLiBUaGUgaW50ZW50aW9uIGlzIHRvIGJlIGFibGUgdG8g
bGltaXQgbW9kdWxlDQo+ID4gc3BhY2UNCj4gPiBhbGxvY2F0aW9ucyB0aGF0IG1heSBjb21lIGZy
b20gdW4tcHJpdmxpZGdlZCB1c2VycyBpbnNlcnRpbmcgZS9CUEYgZmlsdGVycy4NCj4gDQo+IE5v
dGUgdGhhdCBpbiBzb21lIGNvbmZpZ3VyYXRpb25zIChpaXJjIGUuZy4gdGhlIGRlZmF1bHQgVWJ1
bnR1DQo+IGNvbmZpZyksIG5vcm1hbCB1c2VycyBjYW4gdXNlIHRoZSBzdWJ1aWQgbWVjaGFuaXNt
ICh0aGUgL2V0Yy9zdWJ1aWQNCj4gY29uZmlnIGZpbGUgYW5kIHRoZSAvdXNyL2Jpbi9uZXd1aWRt
YXAgc2V0dWlkIGhlbHBlcikgdG8gZ2FpbiBhY2Nlc3MNCj4gdG8gNjU1MzYgVUlEcywgd2hpY2gg
bWVhbnMgdGhhdCBpbiBzdWNoIGEgY29uZmlndXJhdGlvbiwNCj4gUkxJTUlUX01PRFNQQUNFKjY1
NTM3IGlzIHRoZSBhY3R1YWwgbGltaXQgZm9yIG9uZSB1c2VyLiAoU2FtZSB0aGluZw0KPiBhcHBs
aWVzIHRvIFJMSU1JVF9NRU1MT0NLLikNCkFoLCB0aGF0IGlzIGEgcHJvYmxlbS4gVGhlcmUgaXMg
b25seSByb29tIGZvciBhYm91dCAxMzAsMDAwIGZpbHRlcnMgb24geDg2IHdpdGgNCktBU0xSLCBz
byBpdCBjb3VsZG4ndCByZWFsbHkgYmUgc2V0IHNtYWxsIGVub3VnaC4NCg0KSSdsbCBoYXZlIHRv
IGxvb2sgaW50byB3aGF0IHRoaXMgaXMuIFRoYW5rcyBmb3IgcG9pbnRpbmcgaXQgb3V0Lg0KDQo+
IEFsc28sIGl0IGlzIHByb2JhYmx5IHBvc3NpYmxlIHRvIHdhc3RlIGEgZmV3IHRpbWVzIGFzIG11
Y2ggdmlydHVhbA0KPiBtZW1vcnkgYXMgcGVybWl0dGVkIGJ5IHRoZSBsaW1pdCBieSBkZWxpYmVy
YXRlbHkgZnJhZ21lbnRpbmcgdmlydHVhbA0KPiBtZW1vcnk/DQpHb29kIHBvaW50LiBJIGd1ZXNz
IGlmIHRoZSBmaXJzdCBwb2ludCBjYW4gYmUgYWRkcmVzc2VkIHNvbWVob3csIHRoaXMgb25lIGNv
dWxkDQptYXliZSBiZSBzb2x2ZWQgYnkganVzdCBwaWNraW5nIGEgbG93ZXIgbGltaXQuDQoNCkFu
eSB0aG91Z2h0cyBvbiBpZiBpbnN0ZWFkIG9mIGFsbCB0aGlzIHRoZXJlIHdhcyBqdXN0IGEgc3lz
dGVtIHdpZGUgbGltaXQgb24gQlBGDQpKSVQgbW9kdWxlIHNwYWNlIHVzYWdlPw0KDQo+ID4gVGhl
cmUgaXMgdW5mb3J0dW5hdGVseSBubyBjcm9zcyBwbGF0Zm9ybSBwbGFjZSB0byBwZXJmb3JtIHRo
aXMgYWNjb3VudGluZw0KPiA+IGR1cmluZyBhbGxvY2F0aW9uIGluIHRoZSBtb2R1bGUgc3BhY2Us
IHNvIGluc3RlYWQgdHdvIGhlbHBlcnMgYXJlIGNyZWF0ZWQgdG8NCj4gPiBiZQ0KPiA+IGluc2Vy
dGVkIGludG8gdGhlIHZhcmlvdXMgYXJjaOKAmXMgdGhhdCBpbXBsZW1lbnQgbW9kdWxlX2FsbG9j
LiBUaGVzZQ0KPiA+IGhlbHBlcnMgcGVyZm9ybSB0aGUgY2hlY2tzIGFuZCBoZWxwIHdpdGggdHJh
Y2tpbmcuIFRoZSBpbnRlbnRpb24gaXMgdGhhdA0KPiA+IHRoZXkNCj4gPiBhbiBiZSBhZGRlZCB0
byB0aGUgdmFyaW91cyBhcmNo4oCZcyBhcyBlYXNpbHkgYXMgcG9zc2libGUuDQo+IA0KPiBuaXQ6
IHMvYW4vY2FuLw0KPiANCj4gWy4uLl0NCj4gPiBkaWZmIC0tZ2l0IGEva2VybmVsL21vZHVsZS5j
IGIva2VybmVsL21vZHVsZS5jDQo+ID4gaW5kZXggNjc0NmM4NTUxMWZlLi4yZWY5ZWQ5NWJmNjAg
MTAwNjQ0DQo+ID4gLS0tIGEva2VybmVsL21vZHVsZS5jDQo+ID4gKysrIGIva2VybmVsL21vZHVs
ZS5jDQo+ID4gQEAgLTIxMTAsOSArMjExMCwxMzkgQEAgc3RhdGljIHZvaWQgZnJlZV9tb2R1bGVf
ZWxmKHN0cnVjdCBtb2R1bGUgKm1vZCkNCj4gPiAgfQ0KPiA+ICAjZW5kaWYgLyogQ09ORklHX0xJ
VkVQQVRDSCAqL2l0IA0KPiA+IA0KPiA+ICtzdHJ1Y3QgbW9kX2FsbG9jX3VzZXIgew0KPiA+ICsg
ICAgICAgc3RydWN0IHJiX25vZGUgbm9kZTsNCj4gPiArICAgICAgIHVuc2lnbmVkIGxvbmcgYWRk
cjsNCj4gPiArICAgICAgIHVuc2lnbmVkIGxvbmcgcGFnZXM7DQo+ID4gKyAgICAgICBrdWlkX3Qg
dWlkOw0KPiA+ICt9Ow0KPiA+ICsNCj4gPiArc3RhdGljIHN0cnVjdCByYl9yb290IGFsbG9jX3Vz
ZXJzID0gUkJfUk9PVDsNCj4gPiArc3RhdGljIERFRklORV9TUElOTE9DSyhhbGxvY191c2Vyc19s
b2NrKTsNCj4gDQo+IFdoeSBhbGwgdGhlIHJidHJlZSBzdHVmZiBpbnN0ZWFkIG9mIHN0YXNoaW5n
IGEgcG9pbnRlciBpbiBzdHJ1Y3QNCj4gdm1hcF9hcmVhLCBvciBzb21ldGhpbmcgbGlrZSB0aGF0
Pw0KDQpTaW5jZSB0aGUgdHJhY2tpbmcgd2FzIG5vdCBmb3IgYWxsIHZtYWxsb2MgdXNhZ2UsIHRo
ZSBpbnRlbnRpb24gd2FzIHRvIG5vdCBibG9hdA0KdGhlIHN0cnVjdHVyZSBmb3Igb3RoZXIgdXNh
Z2VzIGxpa2VzIHN0YWNrcy4gSSB0aG91Z2h0IHVzdWFsbHkgdGhlcmUgd291bGRuJ3QgYmUNCm5l
YXJseSBhcyBtdWNoIG1vZHVsZSBzcGFjZSBhbGxvY2F0aW9ucyBhcyB0aGVyZSB3b3VsZCBiZSBr
ZXJuZWwgc3RhY2tzLCBidXQgSQ0KZGlkbid0IGRvIGFueSBhY3R1YWwgbWVhc3VyZW1lbnRzIG9u
IHRoZSB0cmFkZW9mZnMuDQoNCj4gWy4uLl0NCj4gPiAraW50IGNoZWNrX2luY19tb2RfcmxpbWl0
KHVuc2lnbmVkIGxvbmcgc2l6ZSkNCj4gPiArew0KPiA+ICsgICAgICAgc3RydWN0IHVzZXJfc3Ry
dWN0ICp1c2VyID0gZ2V0X2N1cnJlbnRfdXNlcigpOw0KPiA+ICsgICAgICAgdW5zaWduZWQgbG9u
ZyBtb2RzcGFjZV9wYWdlcyA9IHJsaW1pdChSTElNSVRfTU9EU1BBQ0UpID4+DQo+ID4gUEFHRV9T
SElGVDsNCj4gPiArICAgICAgIHVuc2lnbmVkIGxvbmcgY3VyX3BhZ2VzID0gYXRvbWljX2xvbmdf
cmVhZCgmdXNlci0+bW9kdWxlX3ZtKTsNCj4gPiArICAgICAgIHVuc2lnbmVkIGxvbmcgbmV3X3Bh
Z2VzID0gZ2V0X21vZF9wYWdlX2NudChzaXplKTsNCj4gPiArDQo+ID4gKyAgICAgICBpZiAocmxp
bWl0KFJMSU1JVF9NT0RTUEFDRSkgIT0gUkxJTV9JTkZJTklUWQ0KPiA+ICsgICAgICAgICAgICAg
ICAgICAgICAgICYmIGN1cl9wYWdlcyArIG5ld19wYWdlcyA+IG1vZHNwYWNlX3BhZ2VzKSB7DQo+
ID4gKyAgICAgICAgICAgICAgIGZyZWVfdWlkKHVzZXIpOw0KPiA+ICsgICAgICAgICAgICAgICBy
ZXR1cm4gMTsNCj4gPiArICAgICAgIH0NCj4gPiArDQo+ID4gKyAgICAgICBhdG9taWNfbG9uZ19h
ZGQobmV3X3BhZ2VzLCAmdXNlci0+bW9kdWxlX3ZtKTsNCj4gPiArDQo+ID4gKyAgICAgICBpZiAo
YXRvbWljX2xvbmdfcmVhZCgmdXNlci0+bW9kdWxlX3ZtKSA+IG1vZHNwYWNlX3BhZ2VzKSB7DQo+
ID4gKyAgICAgICAgICAgICAgIGF0b21pY19sb25nX3N1YihuZXdfcGFnZXMsICZ1c2VyLT5tb2R1
bGVfdm0pOw0KPiA+ICsgICAgICAgICAgICAgICBmcmVlX3VpZCh1c2VyKTsNCj4gPiArICAgICAg
ICAgICAgICAgcmV0dXJuIDE7DQo+ID4gKyAgICAgICB9DQo+ID4gKw0KPiA+ICsgICAgICAgZnJl
ZV91aWQodXNlcik7DQo+IA0KPiBJZiB5b3UgZHJvcCB0aGUgcmVmZXJlbmNlIG9uIHRoZSB1c2Vy
X3N0cnVjdCwgYW4gYXR0YWNrZXIgd2l0aCB0d28NCj4gVUlEcyBjYW4gY2hhcmdlIG1vZHVsZSBh
bGxvY2F0aW9ucyB0byBVSUQgQSwga2VlcCB0aGUgYXNzb2NpYXRlZA0KPiBzb2NrZXRzIGFsaXZl
IGFzIFVJRCBCLCBhbmQgdGhlbiBsb2cgb3V0IGFuZCBiYWNrIGluIGFnYWluIGFzIFVJRCBBLg0K
PiBBdCB0aGF0IHBvaW50LCBub2JvZHkgaXMgY2hhcmdlZCBmb3IgdGhlIG1vZHVsZSBzcGFjZSBh
bnltb3JlLiBJZiB5b3UNCj4gbG9vayBhdCB0aGUgZUJQRiBpbXBsZW1lbnRhdGlvbiwgeW91J2xs
IHNlZSB0aGF0DQo+IGJwZl9wcm9nX2NoYXJnZV9tZW1sb2NrKCkgYWN0dWFsbHkgc3RvcmVzIGEg
cmVmY291bnRlZCBwb2ludGVyIHRvIHRoZQ0KPiB1c2VyX3N0cnVjdC4NCk9rLCBJJ2xsIHRha2Ug
YSBsb29rLiBUaGFua3MgSmFubi4NCj4gPiArICAgICAgIHJldHVybiAwOw0KPiA+ICt9DQo

[PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
> 
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)
Ah, that is a problem. There is only room for about 130,000 filters on x86 with
KASLR, so it couldn't really be set small enough.

I'll have to look into what this is. Thanks for pointing it out.

> Also, it is probably possible to waste a few times as much virtual
> memory as permitted by the limit by deliberately fragmenting virtual
> memory?
Good point. I guess if the first point can be addressed somehow, this one could
maybe be solved by just picking a lower limit.

Any thoughts on if instead of all this there was just a system wide limit on BPF
JIT module space usage?

> > There is unfortunately no cross platform place to perform this accounting
> > during allocation in the module space, so instead two helpers are created to
> > be
> > inserted into the various arch?s that implement module_alloc. These
> > helpers perform the checks and help with tracking. The intention is that
> > they
> > an be added to the various arch?s as easily as possible.
> 
> nit: s/an/can/
> 
> [...]
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 6746c85511fe..2ef9ed95bf60 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> >  }
> >  #endif /* CONFIG_LIVEPATCH */it 
> > 
> > +struct mod_alloc_user {
> > +       struct rb_node node;
> > +       unsigned long addr;
> > +       unsigned long pages;
> > +       kuid_t uid;
> > +};
> > +
> > +static struct rb_root alloc_users = RB_ROOT;
> > +static DEFINE_SPINLOCK(alloc_users_lock);
> 
> Why all the rbtree stuff instead of stashing a pointer in struct
> vmap_area, or something like that?

Since the tracking was not for all vmalloc usage, the intention was to not bloat
the structure for other usages likes stacks. I thought usually there wouldn't be
nearly as much module space allocations as there would be kernel stacks, but I
didn't do any actual measurements on the tradeoffs.

> [...]
> > +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);
> 
> If you drop the reference on the user_struct, an attacker with two
> UIDs can charge module allocations to UID A, keep the associated
> sockets alive as UID B, and then log out and back in again as UID A.
> At that point, nobody is charged for the module space anymore. If you
> look at the eBPF implementation, you'll see that
> bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> user_struct.
Ok, I'll take a look. Thanks Jann.
> > +       return 0;
> > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> > <rick.p.edgecombe@intel.com> wrote:
> > > 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.
> >
> > Note that in some configurations (iirc e.g. the default Ubuntu
> > config), normal users can use the subuid mechanism (the /etc/subuid
> > config file and the /usr/bin/newuidmap setuid helper) to gain access
> > to 65536 UIDs, which means that in such a configuration,
> > RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> > applies to RLIMIT_MEMLOCK.)
> Ah, that is a problem. There is only room for about 130,000 filters on x86 with
> KASLR, so it couldn't really be set small enough.
>
> I'll have to look into what this is. Thanks for pointing it out.
>
> > Also, it is probably possible to waste a few times as much virtual
> > memory as permitted by the limit by deliberately fragmenting virtual
> > memory?
> Good point. I guess if the first point can be addressed somehow, this one could
> maybe be solved by just picking a lower limit.
>
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

That does sound more robust to me. And at least on systems that don't
compile out the BPF interpreter, everything should be more or less
fine then...

> > > There is unfortunately no cross platform place to perform this accounting
> > > during allocation in the module space, so instead two helpers are created to
> > > be
> > > inserted into the various arch’s that implement module_alloc. These
> > > helpers perform the checks and help with tracking. The intention is that
> > > they
> > > an be added to the various arch’s as easily as possible.
> >
> > nit: s/an/can/
> >
> > [...]
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 6746c85511fe..2ef9ed95bf60 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> > >  }
> > >  #endif /* CONFIG_LIVEPATCH */it
> > >
> > > +struct mod_alloc_user {
> > > +       struct rb_node node;
> > > +       unsigned long addr;
> > > +       unsigned long pages;
> > > +       kuid_t uid;
> > > +};
> > > +
> > > +static struct rb_root alloc_users = RB_ROOT;
> > > +static DEFINE_SPINLOCK(alloc_users_lock);
> >
> > Why all the rbtree stuff instead of stashing a pointer in struct
> > vmap_area, or something like that?
>
> Since the tracking was not for all vmalloc usage, the intention was to not bloat
> the structure for other usages likes stacks. I thought usually there wouldn't be
> nearly as much module space allocations as there would be kernel stacks, but I
> didn't do any actual measurements on the tradeoffs.

I imagine that one extra pointer in there - pointing to your struct
mod_alloc_user - would probably not be terrible. 8 bytes more per
kernel stack shouldn't be so bad?

> > [...]
> > > +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);
> >
> > If you drop the reference on the user_struct, an attacker with two
> > UIDs can charge module allocations to UID A, keep the associated
> > sockets alive as UID B, and then log out and back in again as UID A.
> > At that point, nobody is charged for the module space anymore. If you
> > look at the eBPF implementation, you'll see that
> > bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> > user_struct.
> Ok, I'll take a look. Thanks Jann.
> > > +       return 0;
> > > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> > <rick.p.edgecombe@intel.com> wrote:
> > > 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.
> >
> > Note that in some configurations (iirc e.g. the default Ubuntu
> > config), normal users can use the subuid mechanism (the /etc/subuid
> > config file and the /usr/bin/newuidmap setuid helper) to gain access
> > to 65536 UIDs, which means that in such a configuration,
> > RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> > applies to RLIMIT_MEMLOCK.)
> Ah, that is a problem. There is only room for about 130,000 filters on x86 with
> KASLR, so it couldn't really be set small enough.
>
> I'll have to look into what this is. Thanks for pointing it out.
>
> > Also, it is probably possible to waste a few times as much virtual
> > memory as permitted by the limit by deliberately fragmenting virtual
> > memory?
> Good point. I guess if the first point can be addressed somehow, this one could
> maybe be solved by just picking a lower limit.
>
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

That does sound more robust to me. And at least on systems that don't
compile out the BPF interpreter, everything should be more or less
fine then...

> > > There is unfortunately no cross platform place to perform this accounting
> > > during allocation in the module space, so instead two helpers are created to
> > > be
> > > inserted into the various arch’s that implement module_alloc. These
> > > helpers perform the checks and help with tracking. The intention is that
> > > they
> > > an be added to the various arch’s as easily as possible.
> >
> > nit: s/an/can/
> >
> > [...]
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 6746c85511fe..2ef9ed95bf60 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> > >  }
> > >  #endif /* CONFIG_LIVEPATCH */it
> > >
> > > +struct mod_alloc_user {
> > > +       struct rb_node node;
> > > +       unsigned long addr;
> > > +       unsigned long pages;
> > > +       kuid_t uid;
> > > +};
> > > +
> > > +static struct rb_root alloc_users = RB_ROOT;
> > > +static DEFINE_SPINLOCK(alloc_users_lock);
> >
> > Why all the rbtree stuff instead of stashing a pointer in struct
> > vmap_area, or something like that?
>
> Since the tracking was not for all vmalloc usage, the intention was to not bloat
> the structure for other usages likes stacks. I thought usually there wouldn't be
> nearly as much module space allocations as there would be kernel stacks, but I
> didn't do any actual measurements on the tradeoffs.

I imagine that one extra pointer in there - pointing to your struct
mod_alloc_user - would probably not be terrible. 8 bytes more per
kernel stack shouldn't be so bad?

> > [...]
> > > +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);
> >
> > If you drop the reference on the user_struct, an attacker with two
> > UIDs can charge module allocations to UID A, keep the associated
> > sockets alive as UID B, and then log out and back in again as UID A.
> > At that point, nobody is charged for the module space anymore. If you
> > look at the eBPF implementation, you'll see that
> > bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> > user_struct.
> Ok, I'll take a look. Thanks Jann.
> > > +       return 0;
> > > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> > <rick.p.edgecombe@intel.com> wrote:
> > > 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.
> >
> > Note that in some configurations (iirc e.g. the default Ubuntu
> > config), normal users can use the subuid mechanism (the /etc/subuid
> > config file and the /usr/bin/newuidmap setuid helper) to gain access
> > to 65536 UIDs, which means that in such a configuration,
> > RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> > applies to RLIMIT_MEMLOCK.)
> Ah, that is a problem. There is only room for about 130,000 filters on x86 with
> KASLR, so it couldn't really be set small enough.
>
> I'll have to look into what this is. Thanks for pointing it out.
>
> > Also, it is probably possible to waste a few times as much virtual
> > memory as permitted by the limit by deliberately fragmenting virtual
> > memory?
> Good point. I guess if the first point can be addressed somehow, this one could
> maybe be solved by just picking a lower limit.
>
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

That does sound more robust to me. And at least on systems that don't
compile out the BPF interpreter, everything should be more or less
fine then...

> > > There is unfortunately no cross platform place to perform this accounting
> > > during allocation in the module space, so instead two helpers are created to
> > > be
> > > inserted into the various arch’s that implement module_alloc. These
> > > helpers perform the checks and help with tracking. The intention is that
> > > they
> > > an be added to the various arch’s as easily as possible.
> >
> > nit: s/an/can/
> >
> > [...]
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 6746c85511fe..2ef9ed95bf60 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> > >  }
> > >  #endif /* CONFIG_LIVEPATCH */it
> > >
> > > +struct mod_alloc_user {
> > > +       struct rb_node node;
> > > +       unsigned long addr;
> > > +       unsigned long pages;
> > > +       kuid_t uid;
> > > +};
> > > +
> > > +static struct rb_root alloc_users = RB_ROOT;
> > > +static DEFINE_SPINLOCK(alloc_users_lock);
> >
> > Why all the rbtree stuff instead of stashing a pointer in struct
> > vmap_area, or something like that?
>
> Since the tracking was not for all vmalloc usage, the intention was to not bloat
> the structure for other usages likes stacks. I thought usually there wouldn't be
> nearly as much module space allocations as there would be kernel stacks, but I
> didn't do any actual measurements on the tradeoffs.

I imagine that one extra pointer in there - pointing to your struct
mod_alloc_user - would probably not be terrible. 8 bytes more per
kernel stack shouldn't be so bad?

> > [...]
> > > +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);
> >
> > If you drop the reference on the user_struct, an attacker with two
> > UIDs can charge module allocations to UID A, keep the associated
> > sockets alive as UID B, and then log out and back in again as UID A.
> > At that point, nobody is charged for the module space anymore. If you
> > look at the eBPF implementation, you'll see that
> > bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> > user_struct.
> Ok, I'll take a look. Thanks Jann.
> > > +       return 0;
> > > +}

[PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> > <rick.p.edgecombe@intel.com> wrote:
> > > 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.
> >
> > Note that in some configurations (iirc e.g. the default Ubuntu
> > config), normal users can use the subuid mechanism (the /etc/subuid
> > config file and the /usr/bin/newuidmap setuid helper) to gain access
> > to 65536 UIDs, which means that in such a configuration,
> > RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> > applies to RLIMIT_MEMLOCK.)
> Ah, that is a problem. There is only room for about 130,000 filters on x86 with
> KASLR, so it couldn't really be set small enough.
>
> I'll have to look into what this is. Thanks for pointing it out.
>
> > Also, it is probably possible to waste a few times as much virtual
> > memory as permitted by the limit by deliberately fragmenting virtual
> > memory?
> Good point. I guess if the first point can be addressed somehow, this one could
> maybe be solved by just picking a lower limit.
>
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

That does sound more robust to me. And at least on systems that don't
compile out the BPF interpreter, everything should be more or less
fine then...

> > > There is unfortunately no cross platform place to perform this accounting
> > > during allocation in the module space, so instead two helpers are created to
> > > be
> > > inserted into the various arch?s that implement module_alloc. These
> > > helpers perform the checks and help with tracking. The intention is that
> > > they
> > > an be added to the various arch?s as easily as possible.
> >
> > nit: s/an/can/
> >
> > [...]
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 6746c85511fe..2ef9ed95bf60 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2110,9 +2110,139 @@ static void free_module_elf(struct module *mod)
> > >  }
> > >  #endif /* CONFIG_LIVEPATCH */it
> > >
> > > +struct mod_alloc_user {
> > > +       struct rb_node node;
> > > +       unsigned long addr;
> > > +       unsigned long pages;
> > > +       kuid_t uid;
> > > +};
> > > +
> > > +static struct rb_root alloc_users = RB_ROOT;
> > > +static DEFINE_SPINLOCK(alloc_users_lock);
> >
> > Why all the rbtree stuff instead of stashing a pointer in struct
> > vmap_area, or something like that?
>
> Since the tracking was not for all vmalloc usage, the intention was to not bloat
> the structure for other usages likes stacks. I thought usually there wouldn't be
> nearly as much module space allocations as there would be kernel stacks, but I
> didn't do any actual measurements on the tradeoffs.

I imagine that one extra pointer in there - pointing to your struct
mod_alloc_user - would probably not be terrible. 8 bytes more per
kernel stack shouldn't be so bad?

> > [...]
> > > +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);
> >
> > If you drop the reference on the user_struct, an attacker with two
> > UIDs can charge module allocations to UID A, keep the associated
> > sockets alive as UID B, and then log out and back in again as UID A.
> > At that point, nobody is charged for the module space anymore. If you
> > look at the eBPF implementation, you'll see that
> > bpf_prog_charge_memlock() actually stores a refcounted pointer to the
> > user_struct.
> Ok, I'll take a look. Thanks Jann.
> > > +       return 0;
> > > +}

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 2:35 AM Jann Horn <jannh@google.com> wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
>
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)

Actually, I may have misremembered, perhaps it's not installed by
default - I just checked in a Ubuntu VM, and the newuidmap helper from
the uidmap package wasn't installed.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 2:35 AM Jann Horn <jannh@google.com> wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
>
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)

Actually, I may have misremembered, perhaps it's not installed by
default - I just checked in a Ubuntu VM, and the newuidmap helper from
the uidmap package wasn't installed.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 2:35 AM Jann Horn <jannh@google.com> wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
>
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)

Actually, I may have misremembered, perhaps it's not installed by
default - I just checked in a Ubuntu VM, and the newuidmap helper from
the uidmap package wasn't installed.

[PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Fri, Oct 12, 2018 at 2:35 AM Jann Horn <jannh@google.com> wrote:
> On Fri, Oct 12, 2018 at 1:40 AM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > 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.
>
> Note that in some configurations (iirc e.g. the default Ubuntu
> config), normal users can use the subuid mechanism (the /etc/subuid
> config file and the /usr/bin/newuidmap setuid helper) to gain access
> to 65536 UIDs, which means that in such a configuration,
> RLIMIT_MODSPACE*65537 is the actual limit for one user. (Same thing
> applies to RLIMIT_MEMLOCK.)

Actually, I may have misremembered, perhaps it's not installed by
default - I just checked in a Ubuntu VM, and the newuidmap helper from
the uidmap package wasn't installed.

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> +++ Dave Hansen [11/10/18 16:47 -0700]:
> > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > +	if (check_inc_mod_rlimit(size))
> > > +		return NULL;
> > > +
> > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > >  				module_alloc_base + MODULES_VSIZE,
> > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > >  		return NULL;
> > >  	}
> > > 
> > > +	update_mod_rlimit(p, size);
> > 
> > Is there a reason we couldn't just rename all of the existing per-arch
> > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > this new rlimit code in a generic helper that does:
> > 
> > 
> > 	if (check_inc_mod_rlimit(size))
> > 		return NULL;
> > 
> > 	p = arch_module_alloc(...);
> > 
> > 	...
> > 
> > 	update_mod_rlimit(p, size);
> > 
> 
> I second this suggestion. Just make module_{alloc,memfree} generic,
> non-weak functions that call the rlimit helpers in addition to the
> arch-specific arch_module_{alloc,memfree} functions.
> 
> Jessica

Ok, thanks. I am going to try another version of this with just a system wide
BPF JIT limit based on the problems Jann brought up. I think it would be nice to
have a module space limit, but as far as I know the only way today un-privlidged 
users could fill the space is from BPF JIT. Unless you see another purpose long
term?

Rick

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> +++ Dave Hansen [11/10/18 16:47 -0700]:
> > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > +	if (check_inc_mod_rlimit(size))
> > > +		return NULL;
> > > +
> > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > >  				module_alloc_base + MODULES_VSIZE,
> > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > >  		return NULL;
> > >  	}
> > > 
> > > +	update_mod_rlimit(p, size);
> > 
> > Is there a reason we couldn't just rename all of the existing per-arch
> > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > this new rlimit code in a generic helper that does:
> > 
> > 
> > 	if (check_inc_mod_rlimit(size))
> > 		return NULL;
> > 
> > 	p = arch_module_alloc(...);
> > 
> > 	...
> > 
> > 	update_mod_rlimit(p, size);
> > 
> 
> I second this suggestion. Just make module_{alloc,memfree} generic,
> non-weak functions that call the rlimit helpers in addition to the
> arch-specific arch_module_{alloc,memfree} functions.
> 
> Jessica

Ok, thanks. I am going to try another version of this with just a system wide
BPF JIT limit based on the problems Jann brought up. I think it would be nice to
have a module space limit, but as far as I know the only way today un-privlidged 
users could fill the space is from BPF JIT. Unless you see another purpose long
term?

Rick

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> +++ Dave Hansen [11/10/18 16:47 -0700]:
> > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > +	if (check_inc_mod_rlimit(size))
> > > +		return NULL;
> > > +
> > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > >  				module_alloc_base + MODULES_VSIZE,
> > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > >  		return NULL;
> > >  	}
> > > 
> > > +	update_mod_rlimit(p, size);
> > 
> > Is there a reason we couldn't just rename all of the existing per-arch
> > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > this new rlimit code in a generic helper that does:
> > 
> > 
> > 	if (check_inc_mod_rlimit(size))
> > 		return NULL;
> > 
> > 	p = arch_module_alloc(...);
> > 
> > 	...
> > 
> > 	update_mod_rlimit(p, size);
> > 
> 
> I second this suggestion. Just make module_{alloc,memfree} generic,
> non-weak functions that call the rlimit helpers in addition to the
> arch-specific arch_module_{alloc,memfree} functions.
> 
> Jessica

Ok, thanks. I am going to try another version of this with just a system wide
BPF JIT limit based on the problems Jann brought up. I think it would be nice to
have a module space limit, but as far as I know the only way today un-privlidged 
users could fill the space is from BPF JIT. Unless you see another purpose long
term?

Rick

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

T24gRnJpLCAyMDE4LTEwLTEyIGF0IDE2OjMyICswMjAwLCBKZXNzaWNhIFl1IHdyb3RlOg0KPiAr
KysgRGF2ZSBIYW5zZW4gWzExLzEwLzE4IDE2OjQ3IC0wNzAwXToNCj4gPiBPbiAxMC8xMS8yMDE4
IDA0OjMxIFBNLCBSaWNrIEVkZ2Vjb21iZSB3cm90ZToNCj4gPiA+ICsJaWYgKGNoZWNrX2luY19t
b2RfcmxpbWl0KHNpemUpKQ0KPiA+ID4gKwkJcmV0dXJuIE5VTEw7DQo+ID4gPiArDQo+ID4gPiAg
CXAgPSBfX3ZtYWxsb2Nfbm9kZV9yYW5nZShzaXplLCBNT0RVTEVfQUxJR04sIG1vZHVsZV9hbGxv
Y19iYXNlLA0KPiA+ID4gIAkJCQltb2R1bGVfYWxsb2NfYmFzZSArIE1PRFVMRVNfVlNJWkUsDQo+
ID4gPiAgCQkJCWdmcF9tYXNrLCBQQUdFX0tFUk5FTF9FWEVDLCAwLA0KPiA+ID4gQEAgLTY1LDYg
KzY4LDggQEAgdm9pZCAqbW9kdWxlX2FsbG9jKHVuc2lnbmVkIGxvbmcgc2l6ZSkNCj4gPiA+ICAJ
CXJldHVybiBOVUxMOw0KPiA+ID4gIAl9DQo+ID4gPiANCj4gPiA+ICsJdXBkYXRlX21vZF9ybGlt
aXQocCwgc2l6ZSk7DQo+ID4gDQo+ID4gSXMgdGhlcmUgYSByZWFzb24gd2UgY291bGRuJ3QganVz
dCByZW5hbWUgYWxsIG9mIHRoZSBleGlzdGluZyBwZXItYXJjaA0KPiA+IG1vZHVsZV9hbGxvYygp
IGNhbGxzIHRvIGJlIGNhbGxlZCwgc2F5LCAiYXJjaF9tb2R1bGVfYWxsb2MoKSIsIHRoZW4gcHV0
DQo+ID4gdGhpcyBuZXcgcmxpbWl0IGNvZGUgaW4gYSBnZW5lcmljIGhlbHBlciB0aGF0IGRvZXM6
DQo+ID4gDQo+ID4gDQo+ID4gCWlmIChjaGVja19pbmNfbW9kX3JsaW1pdChzaXplKSkNCj4gPiAJ
CXJldHVybiBOVUxMOw0KPiA+IA0KPiA+IAlwID0gYXJjaF9tb2R1bGVfYWxsb2MoLi4uKTsNCj4g
PiANCj4gPiAJLi4uDQo+ID4gDQo+ID4gCXVwZGF0ZV9tb2RfcmxpbWl0KHAsIHNpemUpOw0KPiA+
IA0KPiANCj4gSSBzZWNvbmQgdGhpcyBzdWdnZXN0aW9uLiBKdXN0IG1ha2UgbW9kdWxlX3thbGxv
YyxtZW1mcmVlfSBnZW5lcmljLA0KPiBub24td2VhayBmdW5jdGlvbnMgdGhhdCBjYWxsIHRoZSBy
bGltaXQgaGVscGVycyBpbiBhZGRpdGlvbiB0byB0aGUNCj4gYXJjaC1zcGVjaWZpYyBhcmNoX21v
ZHVsZV97YWxsb2MsbWVtZnJlZX0gZnVuY3Rpb25zLg0KPiANCj4gSmVzc2ljYQ0KDQpPaywgdGhh
bmtzLiBJIGFtIGdvaW5nIHRvIHRyeSBhbm90aGVyIHZlcnNpb24gb2YgdGhpcyB3aXRoIGp1c3Qg
YSBzeXN0ZW0gd2lkZQ0KQlBGIEpJVCBsaW1pdCBiYXNlZCBvbiB0aGUgcHJvYmxlbXMgSmFubiBi
cm91Z2h0IHVwLiBJIHRoaW5rIGl0IHdvdWxkIGJlIG5pY2UgdG8NCmhhdmUgYSBtb2R1bGUgc3Bh
Y2UgbGltaXQsIGJ1dCBhcyBmYXIgYXMgSSBrbm93IHRoZSBvbmx5IHdheSB0b2RheSB1bi1wcml2
bGlkZ2VkIA0KdXNlcnMgY291bGQgZmlsbCB0aGUgc3BhY2UgaXMgZnJvbSBCUEYgSklULiBVbmxl
c3MgeW91IHNlZSBhbm90aGVyIHB1cnBvc2UgbG9uZw0KdGVybT8NCg0KUmljaw0K

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> +++ Dave Hansen [11/10/18 16:47 -0700]:
> > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > +	if (check_inc_mod_rlimit(size))
> > > +		return NULL;
> > > +
> > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > >  				module_alloc_base + MODULES_VSIZE,
> > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > >  		return NULL;
> > >  	}
> > > 
> > > +	update_mod_rlimit(p, size);
> > 
> > Is there a reason we couldn't just rename all of the existing per-arch
> > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > this new rlimit code in a generic helper that does:
> > 
> > 
> > 	if (check_inc_mod_rlimit(size))
> > 		return NULL;
> > 
> > 	p = arch_module_alloc(...);
> > 
> > 	...
> > 
> > 	update_mod_rlimit(p, size);
> > 
> 
> I second this suggestion. Just make module_{alloc,memfree} generic,
> non-weak functions that call the rlimit helpers in addition to the
> arch-specific arch_module_{alloc,memfree} functions.
> 
> Jessica

Ok, thanks. I am going to try another version of this with just a system wide
BPF JIT limit based on the problems Jann brought up. I think it would be nice to
have a module space limit, but as far as I know the only way today un-privlidged 
users could fill the space is from BPF JIT. Unless you see another purpose long
term?

Rick

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 22:01 +0000, Edgecombe, Rick P wrote:
> On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> > +++ Dave Hansen [11/10/18 16:47 -0700]:
> > > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > > +	if (check_inc_mod_rlimit(size))
> > > > +		return NULL;
> > > > +
> > > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > > >  				module_alloc_base + MODULES_VSIZE,
> > > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > > >  		return NULL;
> > > >  	}
> > > > 
> > > > +	update_mod_rlimit(p, size);
> > > 
> > > Is there a reason we couldn't just rename all of the existing per-arch
> > > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > > this new rlimit code in a generic helper that does:
> > > 
> > > 
> > > 	if (check_inc_mod_rlimit(size))
> > > 		return NULL;
> > > 
> > > 	p = arch_module_alloc(...);
> > > 
> > > 	...
> > > 
> > > 	update_mod_rlimit(p, size);
> > > 
> > 
> > I second this suggestion. Just make module_{alloc,memfree} generic,
> > non-weak functions that call the rlimit helpers in addition to the
> > arch-specific arch_module_{alloc,memfree} functions.
> > 
> > Jessica
> 
> Ok, thanks. I am going to try another version of this with just a system wide
> BPF JIT limit based on the problems Jann brought up. I think it would be nice
> to
> have a module space limit, but as far as I know the only way today un-
> privlidged 
> users could fill the space is from BPF JIT. Unless you see another purpose
> long
> term?

Err, nevermind. Going to try to include both limits. I'll incorporate this
refactor into the next version.

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 22:01 +0000, Edgecombe, Rick P wrote:
> On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> > +++ Dave Hansen [11/10/18 16:47 -0700]:
> > > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > > +	if (check_inc_mod_rlimit(size))
> > > > +		return NULL;
> > > > +
> > > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > > >  				module_alloc_base + MODULES_VSIZE,
> > > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > > >  		return NULL;
> > > >  	}
> > > > 
> > > > +	update_mod_rlimit(p, size);
> > > 
> > > Is there a reason we couldn't just rename all of the existing per-arch
> > > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > > this new rlimit code in a generic helper that does:
> > > 
> > > 
> > > 	if (check_inc_mod_rlimit(size))
> > > 		return NULL;
> > > 
> > > 	p = arch_module_alloc(...);
> > > 
> > > 	...
> > > 
> > > 	update_mod_rlimit(p, size);
> > > 
> > 
> > I second this suggestion. Just make module_{alloc,memfree} generic,
> > non-weak functions that call the rlimit helpers in addition to the
> > arch-specific arch_module_{alloc,memfree} functions.
> > 
> > Jessica
> 
> Ok, thanks. I am going to try another version of this with just a system wide
> BPF JIT limit based on the problems Jann brought up. I think it would be nice
> to
> have a module space limit, but as far as I know the only way today un-
> privlidged 
> users could fill the space is from BPF JIT. Unless you see another purpose
> long
> term?

Err, nevermind. Going to try to include both limits. I'll incorporate this
refactor into the next version.

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 22:01 +0000, Edgecombe, Rick P wrote:
> On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> > +++ Dave Hansen [11/10/18 16:47 -0700]:
> > > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > > +	if (check_inc_mod_rlimit(size))
> > > > +		return NULL;
> > > > +
> > > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > > >  				module_alloc_base + MODULES_VSIZE,
> > > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > > >  		return NULL;
> > > >  	}
> > > > 
> > > > +	update_mod_rlimit(p, size);
> > > 
> > > Is there a reason we couldn't just rename all of the existing per-arch
> > > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > > this new rlimit code in a generic helper that does:
> > > 
> > > 
> > > 	if (check_inc_mod_rlimit(size))
> > > 		return NULL;
> > > 
> > > 	p = arch_module_alloc(...);
> > > 
> > > 	...
> > > 
> > > 	update_mod_rlimit(p, size);
> > > 
> > 
> > I second this suggestion. Just make module_{alloc,memfree} generic,
> > non-weak functions that call the rlimit helpers in addition to the
> > arch-specific arch_module_{alloc,memfree} functions.
> > 
> > Jessica
> 
> Ok, thanks. I am going to try another version of this with just a system wide
> BPF JIT limit based on the problems Jann brought up. I think it would be nice
> to
> have a module space limit, but as far as I know the only way today un-
> privlidged 
> users could fill the space is from BPF JIT. Unless you see another purpose
> long
> term?

Err, nevermind. Going to try to include both limits. I'll incorporate this
refactor into the next version.

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

T24gRnJpLCAyMDE4LTEwLTEyIGF0IDIyOjAxICswMDAwLCBFZGdlY29tYmUsIFJpY2sgUCB3cm90
ZToNCj4gT24gRnJpLCAyMDE4LTEwLTEyIGF0IDE2OjMyICswMjAwLCBKZXNzaWNhIFl1IHdyb3Rl
Og0KPiA+ICsrKyBEYXZlIEhhbnNlbiBbMTEvMTAvMTggMTY6NDcgLTA3MDBdOg0KPiA+ID4gT24g
MTAvMTEvMjAxOCAwNDozMSBQTSwgUmljayBFZGdlY29tYmUgd3JvdGU6DQo+ID4gPiA+ICsJaWYg
KGNoZWNrX2luY19tb2RfcmxpbWl0KHNpemUpKQ0KPiA+ID4gPiArCQlyZXR1cm4gTlVMTDsNCj4g
PiA+ID4gKw0KPiA+ID4gPiAgCXAgPSBfX3ZtYWxsb2Nfbm9kZV9yYW5nZShzaXplLCBNT0RVTEVf
QUxJR04sIG1vZHVsZV9hbGxvY19iYXNlLA0KPiA+ID4gPiAgCQkJCW1vZHVsZV9hbGxvY19iYXNl
ICsgTU9EVUxFU19WU0laRSwNCj4gPiA+ID4gIAkJCQlnZnBfbWFzaywgUEFHRV9LRVJORUxfRVhF
QywgMCwNCj4gPiA+ID4gQEAgLTY1LDYgKzY4LDggQEAgdm9pZCAqbW9kdWxlX2FsbG9jKHVuc2ln
bmVkIGxvbmcgc2l6ZSkNCj4gPiA+ID4gIAkJcmV0dXJuIE5VTEw7DQo+ID4gPiA+ICAJfQ0KPiA+
ID4gPiANCj4gPiA+ID4gKwl1cGRhdGVfbW9kX3JsaW1pdChwLCBzaXplKTsNCj4gPiA+IA0KPiA+
ID4gSXMgdGhlcmUgYSByZWFzb24gd2UgY291bGRuJ3QganVzdCByZW5hbWUgYWxsIG9mIHRoZSBl
eGlzdGluZyBwZXItYXJjaA0KPiA+ID4gbW9kdWxlX2FsbG9jKCkgY2FsbHMgdG8gYmUgY2FsbGVk
LCBzYXksICJhcmNoX21vZHVsZV9hbGxvYygpIiwgdGhlbiBwdXQNCj4gPiA+IHRoaXMgbmV3IHJs
aW1pdCBjb2RlIGluIGEgZ2VuZXJpYyBoZWxwZXIgdGhhdCBkb2VzOg0KPiA+ID4gDQo+ID4gPiAN
Cj4gPiA+IAlpZiAoY2hlY2tfaW5jX21vZF9ybGltaXQoc2l6ZSkpDQo+ID4gPiAJCXJldHVybiBO
VUxMOw0KPiA+ID4gDQo+ID4gPiAJcCA9IGFyY2hfbW9kdWxlX2FsbG9jKC4uLik7DQo+ID4gPiAN
Cj4gPiA+IAkuLi4NCj4gPiA+IA0KPiA+ID4gCXVwZGF0ZV9tb2RfcmxpbWl0KHAsIHNpemUpOw0K
PiA+ID4gDQo+ID4gDQo+ID4gSSBzZWNvbmQgdGhpcyBzdWdnZXN0aW9uLiBKdXN0IG1ha2UgbW9k
dWxlX3thbGxvYyxtZW1mcmVlfSBnZW5lcmljLA0KPiA+IG5vbi13ZWFrIGZ1bmN0aW9ucyB0aGF0
IGNhbGwgdGhlIHJsaW1pdCBoZWxwZXJzIGluIGFkZGl0aW9uIHRvIHRoZQ0KPiA+IGFyY2gtc3Bl
Y2lmaWMgYXJjaF9tb2R1bGVfe2FsbG9jLG1lbWZyZWV9IGZ1bmN0aW9ucy4NCj4gPiANCj4gPiBK
ZXNzaWNhDQo+IA0KPiBPaywgdGhhbmtzLiBJIGFtIGdvaW5nIHRvIHRyeSBhbm90aGVyIHZlcnNp
b24gb2YgdGhpcyB3aXRoIGp1c3QgYSBzeXN0ZW0gd2lkZQ0KPiBCUEYgSklUIGxpbWl0IGJhc2Vk
IG9uIHRoZSBwcm9ibGVtcyBKYW5uIGJyb3VnaHQgdXAuIEkgdGhpbmsgaXQgd291bGQgYmUgbmlj
ZQ0KPiB0bw0KPiBoYXZlIGEgbW9kdWxlIHNwYWNlIGxpbWl0LCBidXQgYXMgZmFyIGFzIEkga25v
dyB0aGUgb25seSB3YXkgdG9kYXkgdW4tDQo+IHByaXZsaWRnZWQgDQo+IHVzZXJzIGNvdWxkIGZp
bGwgdGhlIHNwYWNlIGlzIGZyb20gQlBGIEpJVC4gVW5sZXNzIHlvdSBzZWUgYW5vdGhlciBwdXJw
b3NlDQo+IGxvbmcNCj4gdGVybT8NCg0KRXJyLCBuZXZlcm1pbmQuIEdvaW5nIHRvIHRyeSB0byBp
bmNsdWRlIGJvdGggbGltaXRzLiBJJ2xsIGluY29ycG9yYXRlIHRoaXMNCnJlZmFjdG9yIGludG8g
dGhlIG5leHQgdmVyc2lvbi4NCg=

[PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 22:01 +0000, Edgecombe, Rick P wrote:
> On Fri, 2018-10-12 at 16:32 +0200, Jessica Yu wrote:
> > +++ Dave Hansen [11/10/18 16:47 -0700]:
> > > On 10/11/2018 04:31 PM, Rick Edgecombe wrote:
> > > > +	if (check_inc_mod_rlimit(size))
> > > > +		return NULL;
> > > > +
> > > >  	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> > > >  				module_alloc_base + MODULES_VSIZE,
> > > >  				gfp_mask, PAGE_KERNEL_EXEC, 0,
> > > > @@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
> > > >  		return NULL;
> > > >  	}
> > > > 
> > > > +	update_mod_rlimit(p, size);
> > > 
> > > Is there a reason we couldn't just rename all of the existing per-arch
> > > module_alloc() calls to be called, say, "arch_module_alloc()", then put
> > > this new rlimit code in a generic helper that does:
> > > 
> > > 
> > > 	if (check_inc_mod_rlimit(size))
> > > 		return NULL;
> > > 
> > > 	p = arch_module_alloc(...);
> > > 
> > > 	...
> > > 
> > > 	update_mod_rlimit(p, size);
> > > 
> > 
> > I second this suggestion. Just make module_{alloc,memfree} generic,
> > non-weak functions that call the rlimit helpers in addition to the
> > arch-specific arch_module_{alloc,memfree} functions.
> > 
> > Jessica
> 
> Ok, thanks. I am going to try another version of this with just a system wide
> BPF JIT limit based on the problems Jann brought up. I think it would be nice
> to
> have a module space limit, but as far as I know the only way today un-
> privlidged 
> users could fill the space is from BPF JIT. Unless you see another purpose
> long
> term?

Err, nevermind. Going to try to include both limits. I'll incorporate this
refactor into the next version.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > vmap_area, or something like that?
> > 
> > Since the tracking was not for all vmalloc usage, the intention was to not
> > bloat
> > the structure for other usages likes stacks. I thought usually there
> > wouldn't be
> > nearly as much module space allocations as there would be kernel stacks, but
> > I
> > didn't do any actual measurements on the tradeoffs.
> 
> I imagine that one extra pointer in there - pointing to your struct
> mod_alloc_user - would probably not be terrible. 8 bytes more per
> kernel stack shouldn't be so bad?

I looked into this and it starts to look a little messy. The nommu.c version of
vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
stand in that maintains pretend vm struct's in nommu.c. I had actually
previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.

Thought I would check back and see. How important do you think this is?

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > vmap_area, or something like that?
> > 
> > Since the tracking was not for all vmalloc usage, the intention was to not
> > bloat
> > the structure for other usages likes stacks. I thought usually there
> > wouldn't be
> > nearly as much module space allocations as there would be kernel stacks, but
> > I
> > didn't do any actual measurements on the tradeoffs.
> 
> I imagine that one extra pointer in there - pointing to your struct
> mod_alloc_user - would probably not be terrible. 8 bytes more per
> kernel stack shouldn't be so bad?

I looked into this and it starts to look a little messy. The nommu.c version of
vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
stand in that maintains pretend vm struct's in nommu.c. I had actually
previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.

Thought I would check back and see. How important do you think this is?

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > vmap_area, or something like that?
> > 
> > Since the tracking was not for all vmalloc usage, the intention was to not
> > bloat
> > the structure for other usages likes stacks. I thought usually there
> > wouldn't be
> > nearly as much module space allocations as there would be kernel stacks, but
> > I
> > didn't do any actual measurements on the tradeoffs.
> 
> I imagine that one extra pointer in there - pointing to your struct
> mod_alloc_user - would probably not be terrible. 8 bytes more per
> kernel stack shouldn't be so bad?

I looked into this and it starts to look a little messy. The nommu.c version of
vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
stand in that maintains pretend vm struct's in nommu.c. I had actually
previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.

Thought I would check back and see. How important do you think this is?

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

T24gRnJpLCAyMDE4LTEwLTEyIGF0IDE5OjIyICswMjAwLCBKYW5uIEhvcm4gd3JvdGU6DQo+IE9u
IEZyaSwgT2N0IDEyLCAyMDE4IGF0IDc6MDQgUE0gRWRnZWNvbWJlLCBSaWNrIFANCj4gPHJpY2su
cC5lZGdlY29tYmVAaW50ZWwuY29tPiB3cm90ZToNCj4gPiBPbiBGcmksIDIwMTgtMTAtMTIgYXQg
MDI6MzUgKzAyMDAsIEphbm4gSG9ybiB3cm90ZToNCj4gPiA+IFdoeSBhbGwgdGhlIHJidHJlZSBz
dHVmZiBpbnN0ZWFkIG9mIHN0YXNoaW5nIGEgcG9pbnRlciBpbiBzdHJ1Y3QNCj4gPiA+IHZtYXBf
YXJlYSwgb3Igc29tZXRoaW5nIGxpa2UgdGhhdD8NCj4gPiANCj4gPiBTaW5jZSB0aGUgdHJhY2tp
bmcgd2FzIG5vdCBmb3IgYWxsIHZtYWxsb2MgdXNhZ2UsIHRoZSBpbnRlbnRpb24gd2FzIHRvIG5v
dA0KPiA+IGJsb2F0DQo+ID4gdGhlIHN0cnVjdHVyZSBmb3Igb3RoZXIgdXNhZ2VzIGxpa2VzIHN0
YWNrcy4gSSB0aG91Z2h0IHVzdWFsbHkgdGhlcmUNCj4gPiB3b3VsZG4ndCBiZQ0KPiA+IG5lYXJs
eSBhcyBtdWNoIG1vZHVsZSBzcGFjZSBhbGxvY2F0aW9ucyBhcyB0aGVyZSB3b3VsZCBiZSBrZXJu
ZWwgc3RhY2tzLCBidXQNCj4gPiBJDQo+ID4gZGlkbid0IGRvIGFueSBhY3R1YWwgbWVhc3VyZW1l
bnRzIG9uIHRoZSB0cmFkZW9mZnMuDQo+IA0KPiBJIGltYWdpbmUgdGhhdCBvbmUgZXh0cmEgcG9p
bnRlciBpbiB0aGVyZSAtIHBvaW50aW5nIHRvIHlvdXIgc3RydWN0DQo+IG1vZF9hbGxvY191c2Vy
IC0gd291bGQgcHJvYmFibHkgbm90IGJlIHRlcnJpYmxlLiA4IGJ5dGVzIG1vcmUgcGVyDQo+IGtl
cm5lbCBzdGFjayBzaG91bGRuJ3QgYmUgc28gYmFkPw0KDQpJIGxvb2tlZCBpbnRvIHRoaXMgYW5k
IGl0IHN0YXJ0cyB0byBsb29rIGEgbGl0dGxlIG1lc3N5LiBUaGUgbm9tbXUuYyB2ZXJzaW9uIG9m
DQp2bWFsbG9jIGRvZXNuJ3QgdXNlIG9yIGV4cG9zZSBhY2Nlc3MgdG8gdm1hcF9hcmVhIG9yIHZt
X3N0cnVjdC4gU28gaXQgc3RhcnRzIHRvDQpsb29rIGxpa2UgYSBidW5jaCBvZiBJRkRFRnMgdG8g
cmVtb3ZlIHRoZSBybGltaXQgaW4gdGhlIG5vbW11IGNhc2Ugb3IgbWFraW5nIGENCnN0YW5kIGlu
IHRoYXQgbWFpbnRhaW5zIHByZXRlbmQgdm0gc3RydWN0J3MgaW4gbm9tbXUuYy4gSSBoYWQgYWN0
dWFsbHkNCnByZXZpb3VzbHkgdHJpZWQgdG8gYXQgbGVhc3QgcHVsbCB0aGUgYWxsb2NhdGlvbnMg
c2l6ZSBmcm9tIHZtYWxsb2Mgc3RydWN0cywgYnV0IGl0IGJyb2tlIG9uIG5vbW11Lg0KDQpUaG91
Z2h0IEkgd291bGQgY2hlY2sgYmFjayBhbmQgc2VlLiBIb3cgaW1wb3J0YW50IGRvIHlvdSB0aGlu
ayB0aGlzIGlzPw0KDQoNCg=

[PATCH v2 1/7] modules: Create rlimit for module space

[Edgecombe, Rick P]

On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > vmap_area, or something like that?
> > 
> > Since the tracking was not for all vmalloc usage, the intention was to not
> > bloat
> > the structure for other usages likes stacks. I thought usually there
> > wouldn't be
> > nearly as much module space allocations as there would be kernel stacks, but
> > I
> > didn't do any actual measurements on the tradeoffs.
> 
> I imagine that one extra pointer in there - pointing to your struct
> mod_alloc_user - would probably not be terrible. 8 bytes more per
> kernel stack shouldn't be so bad?

I looked into this and it starts to look a little messy. The nommu.c version of
vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
stand in that maintains pretend vm struct's in nommu.c. I had actually
previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.

Thought I would check back and see. How important do you think this is?

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Sat, Oct 13, 2018 at 2:04 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> > <rick.p.edgecombe@intel.com> wrote:
> > > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > > vmap_area, or something like that?
> > >
> > > Since the tracking was not for all vmalloc usage, the intention was to not
> > > bloat
> > > the structure for other usages likes stacks. I thought usually there
> > > wouldn't be
> > > nearly as much module space allocations as there would be kernel stacks, but
> > > I
> > > didn't do any actual measurements on the tradeoffs.
> >
> > I imagine that one extra pointer in there - pointing to your struct
> > mod_alloc_user - would probably not be terrible. 8 bytes more per
> > kernel stack shouldn't be so bad?
>
> I looked into this and it starts to look a little messy. The nommu.c version of
> vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
> look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
> stand in that maintains pretend vm struct's in nommu.c. I had actually
> previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.
>
> Thought I would check back and see. How important do you think this is?

I don't think it's important - I just thought that it would be nice to
avoid the extra complexity if it is easily avoidable.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Sat, Oct 13, 2018 at 2:04 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> > <rick.p.edgecombe@intel.com> wrote:
> > > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > > vmap_area, or something like that?
> > >
> > > Since the tracking was not for all vmalloc usage, the intention was to not
> > > bloat
> > > the structure for other usages likes stacks. I thought usually there
> > > wouldn't be
> > > nearly as much module space allocations as there would be kernel stacks, but
> > > I
> > > didn't do any actual measurements on the tradeoffs.
> >
> > I imagine that one extra pointer in there - pointing to your struct
> > mod_alloc_user - would probably not be terrible. 8 bytes more per
> > kernel stack shouldn't be so bad?
>
> I looked into this and it starts to look a little messy. The nommu.c version of
> vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
> look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
> stand in that maintains pretend vm struct's in nommu.c. I had actually
> previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.
>
> Thought I would check back and see. How important do you think this is?

I don't think it's important - I just thought that it would be nice to
avoid the extra complexity if it is easily avoidable.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Sat, Oct 13, 2018 at 2:04 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> > <rick.p.edgecombe@intel.com> wrote:
> > > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > > vmap_area, or something like that?
> > >
> > > Since the tracking was not for all vmalloc usage, the intention was to not
> > > bloat
> > > the structure for other usages likes stacks. I thought usually there
> > > wouldn't be
> > > nearly as much module space allocations as there would be kernel stacks, but
> > > I
> > > didn't do any actual measurements on the tradeoffs.
> >
> > I imagine that one extra pointer in there - pointing to your struct
> > mod_alloc_user - would probably not be terrible. 8 bytes more per
> > kernel stack shouldn't be so bad?
>
> I looked into this and it starts to look a little messy. The nommu.c version of
> vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
> look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
> stand in that maintains pretend vm struct's in nommu.c. I had actually
> previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.
>
> Thought I would check back and see. How important do you think this is?

I don't think it's important - I just thought that it would be nice to
avoid the extra complexity if it is easily avoidable.

[PATCH v2 1/7] modules: Create rlimit for module space

[Jann Horn]

On Sat, Oct 13, 2018 at 2:04 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> On Fri, 2018-10-12 at 19:22 +0200, Jann Horn wrote:
> > On Fri, Oct 12, 2018 at 7:04 PM Edgecombe, Rick P
> > <rick.p.edgecombe@intel.com> wrote:
> > > On Fri, 2018-10-12 at 02:35 +0200, Jann Horn wrote:
> > > > Why all the rbtree stuff instead of stashing a pointer in struct
> > > > vmap_area, or something like that?
> > >
> > > Since the tracking was not for all vmalloc usage, the intention was to not
> > > bloat
> > > the structure for other usages likes stacks. I thought usually there
> > > wouldn't be
> > > nearly as much module space allocations as there would be kernel stacks, but
> > > I
> > > didn't do any actual measurements on the tradeoffs.
> >
> > I imagine that one extra pointer in there - pointing to your struct
> > mod_alloc_user - would probably not be terrible. 8 bytes more per
> > kernel stack shouldn't be so bad?
>
> I looked into this and it starts to look a little messy. The nommu.c version of
> vmalloc doesn't use or expose access to vmap_area or vm_struct. So it starts to
> look like a bunch of IFDEFs to remove the rlimit in the nommu case or making a
> stand in that maintains pretend vm struct's in nommu.c. I had actually
> previously tried to at least pull the allocations size from vmalloc structs, but it broke on nommu.
>
> Thought I would check back and see. How important do you think this is?

I don't think it's important - I just thought that it would be nice to
avoid the extra complexity if it is easily avoidable.

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Michal Hocko]

On Fri 12-10-18 17:04:46, Edgecombe, Rick P wrote:
[...]
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

We do allow to charge vmalloc memory to a memory cgroup. Isn't that a
way forward?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Michal Hocko]

On Fri 12-10-18 17:04:46, Edgecombe, Rick P wrote:
[...]
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

We do allow to charge vmalloc memory to a memory cgroup. Isn't that a
way forward?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Michal Hocko]

On Fri 12-10-18 17:04:46, Edgecombe, Rick P wrote:
[...]
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

We do allow to charge vmalloc memory to a memory cgroup. Isn't that a
way forward?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/7] modules: Create rlimit for module space

[Michal Hocko]

On Fri 12-10-18 17:04:46, Edgecombe, Rick P wrote:
[...]
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

We do allow to charge vmalloc memory to a memory cgroup. Isn't that a
way forward?
-- 
Michal Hocko
SUSE Labs

[PATCH v2 1/7] modules: Create rlimit for module space

[Michal Hocko]

On Fri 12-10-18 17:04:46, Edgecombe, Rick P wrote:
[...]
> Any thoughts on if instead of all this there was just a system wide limit on BPF
> JIT module space usage?

We do allow to charge vmalloc memory to a memory cgroup. Isn't that a
way forward?
-- 
Michal Hocko
SUSE Labs