Re: [PATCH] module: add in-kernel support for decompressing
flat view
From: Kees Cook <hidden>
Date: 2021-12-01 19:25:31
Also in:
lkml
On Sat, Nov 27, 2021 at 09:48:22PM -0800, Dmitry Torokhov wrote:
Current scheme of having userspace decompress kernel modules before loading them into the kernel runs afoul of LoadPin security policy, as it loses link between the source of kernel module on the disk and binary blob that is being loaded into the kernel. To solve this issue let's implement decompression in kernel, so that we can pass a file descriptor of compressed module file into finit_module() which will keep LoadPin happy.
Yeah, adding module signing for this case seems like needless overhead when there is an existing fd association back down to a dm-verity device, etc.
To let userspace know what compression/decompression scheme kernel supports it will create /sys/module/compression attribute. kmod can read this attribute and decide if it can pass compressed file to finit_module(). New MODULE_INIT_COMPRESSED_DATA flag indicates that the kernel should attempt to decompress the data read from file descriptor prior to trying load the module.
Cool; this seems reasonable.
quoted hunk ↗ jump to hunk
To simplify things kernel will only implement single decompression method matching compression method selected when generating modules. This patch implements gzip and xz; more can be added later, Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- I am also attaching a patch to kmod to make use of this new facility. Thanks! include/uapi/linux/module.h | 1 + init/Kconfig | 12 ++ kernel/Makefile | 1 + kernel/module-internal.h | 18 +++ kernel/module.c | 35 +++-- kernel/module_decompress.c | 271 ++++++++++++++++++++++++++++++++++++ 6 files changed, 327 insertions(+), 11 deletions(-)diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h index 50d98ec5e866..becab4a1c5db 100644 --- a/include/uapi/linux/module.h +++ b/include/uapi/linux/module.h@@ -5,5 +5,6 @@ /* Flags for sys_finit_module: */ #define MODULE_INIT_IGNORE_MODVERSIONS 1 #define MODULE_INIT_IGNORE_VERMAGIC 2 +#define MODULE_INIT_COMPRESSED_DATA 4
bikeshedding: adding "_DATA" seems redundant/misleading? The entire module is compressed, so maybe call it just MODULE_INIT_COMPRESSED ?
quoted hunk ↗ jump to hunk
#endif /* _UAPI_LINUX_MODULE_H */diff --git a/init/Kconfig b/init/Kconfig index 03d3c21e28a3..a3f37ae7436d 100644 --- a/init/Kconfig +++ b/init/Kconfig@@ -2309,6 +2309,18 @@ config MODULE_COMPRESS_ZSTD endchoice +config MODULE_DECOMPRESS + bool "Support in-kernel module decompression" + select ZLIB_INFLATE if MODULE_COMPRESS_GZIP + select XZ_DEC if MODULE_COMPRESS_XZ + help + + Support for decompressing kernel modules by the kernel itself + instead of relying on userspace to perform this task. Useful when + load pinning security policy is enabled. + + If unsure, say N. + config MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS bool "Allow loading of modules with missing namespace imports" helpdiff --git a/kernel/Makefile b/kernel/Makefile index 186c49582f45..56f4ee97f328 100644 --- a/kernel/Makefile +++ b/kernel/Makefile@@ -67,6 +67,7 @@ obj-y += up.o endif obj-$(CONFIG_UID16) += uid16.o obj-$(CONFIG_MODULES) += module.o +obj-$(CONFIG_MODULE_DECOMPRESS) += module_decompress.o obj-$(CONFIG_MODULE_SIG) += module_signing.o obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o obj-$(CONFIG_KALLSYMS) += kallsyms.odiff --git a/kernel/module-internal.h b/kernel/module-internal.h index 33783abc377b..3c1143d2c8c7 100644 --- a/kernel/module-internal.h +++ b/kernel/module-internal.h@@ -22,6 +22,11 @@ struct load_info { bool sig_ok; #ifdef CONFIG_KALLSYMS unsigned long mod_kallsyms_init_off; +#endif +#ifdef CONFIG_MODULE_DECOMPRESS + struct page **pages; + unsigned int max_pages; + unsigned int used_pages; #endif struct { unsigned int sym, str, mod, vers, info, pcpu;@@ -29,3 +34,16 @@ struct load_info { }; extern int mod_verify_sig(const void *mod, struct load_info *info); + +#ifdef CONFIG_MODULE_DECOMPRESS +int module_decompress(struct load_info *info, const void *buf, size_t size); +void module_decompress_cleanup(struct load_info *info); +#else +int module_decompress(struct load_info *info, const void *buf, size_t size) +{ + return -EOPNOTSUPP; +} +void module_decompress_cleanup(struct load_info *info) +{ +} +#endifdiff --git a/kernel/module.c b/kernel/module.c index 84a9141a5e15..eeab85ea1627 100644 --- a/kernel/module.c +++ b/kernel/module.c@@ -3174,9 +3174,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len, return err; } -static void free_copy(struct load_info *info) +static void free_copy(struct load_info *info, int flags)
Since struct load_info is already being modified, how about adding flags there instead, then it doesn't need to be plumbed down into each of these functions?
quoted hunk ↗ jump to hunk
{ - vfree(info->hdr); + if (flags & MODULE_INIT_COMPRESSED_DATA) + module_decompress_cleanup(info); + else + vfree(info->hdr); } static int rewrite_section_headers(struct load_info *info, int flags)@@ -4125,7 +4128,7 @@ static int load_module(struct load_info *info, const char __user *uargs, } /* Get rid of temporary copy. */ - free_copy(info); + free_copy(info, flags); /* Done! */ trace_module_load(mod);@@ -4174,7 +4177,7 @@ static int load_module(struct load_info *info, const char __user *uargs, module_deallocate(mod, info); free_copy: - free_copy(info); + free_copy(info, flags); return err; }@@ -4201,7 +4204,8 @@ SYSCALL_DEFINE3(init_module, void __user *, umod, SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) { struct load_info info = { }; - void *hdr = NULL; + void *buf = NULL; + int len; int err; err = may_init_module();@@ -4211,15 +4215,24 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags); if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS - |MODULE_INIT_IGNORE_VERMAGIC)) + |MODULE_INIT_IGNORE_VERMAGIC + |MODULE_INIT_COMPRESSED_DATA)) return -EINVAL; - err = kernel_read_file_from_fd(fd, 0, &hdr, INT_MAX, NULL, + len = kernel_read_file_from_fd(fd, 0, &buf, INT_MAX, NULL, READING_MODULE); - if (err < 0) - return err; - info.hdr = hdr; - info.len = err; + if (len < 0) + return len; + + if (flags & MODULE_INIT_COMPRESSED_DATA) { + err = module_decompress(&info, buf, len); + vfree(buf); /* compressed data is no longer needed */ + if (err) + return err; + } else { + info.hdr = buf; + info.len = len; + } return load_module(&info, uargs, flags); }diff --git a/kernel/module_decompress.c b/kernel/module_decompress.c new file mode 100644 index 000000000000..590ca00aa098 --- /dev/null +++ b/kernel/module_decompress.c
I think most of this can be dropped. I think using crypto_comp_decompress() would make much more sense. -- Kees Cook