| Message ID | 20181011233117.7883-5-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Rlimit for module space | expand |
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);
+++ 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
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
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.
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; }
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(+)