| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202112011112.83416FCA2C@keescook>
Date: Wed, 1 Dec 2021 11:24:35 -0800
From: Kees Cook <keescook@...omium.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Jessica Yu <jeyu@...nel.org>,
linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org
Subject: Re: [PATCH] module: add in-kernel support for decompressing
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.
> 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@...il.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 ?
>
> #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"
> help
> diff --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.o
> diff --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)
> +{
> +}
> +#endif
> diff --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?
> {
> - 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
Powered by blists - more mailing lists