Re: [PATCH] module: add in-kernel support for decompressing

[Kees Cook <keescook@chromium.org>]

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@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 ?

>  
>  #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