Date:   Fri, 10 Dec 2021 16:11:21 -0800
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     Jessica Yu <jeyu@kernel.org>, Kees Cook <keescook@chromium.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] module: add in-kernel support for decompressing
Message-ID: <YbPsqR5ZyiFwJul3@bombadil.infradead.org>
References: <YbLvDWdyFivlj7pP@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbLvDWdyFivlj7pP@google.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Thu, Dec 09, 2021 at 10:09:17PM -0800, Dmitry Torokhov wrote:
> diff --git a/init/Kconfig b/init/Kconfig
> index cd23faa163d1..d90774ff7610 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2305,6 +2305,19 @@ config MODULE_COMPRESS_ZSTD
>  
>  endchoice
>  
> +config MODULE_DECOMPRESS
> +	bool "Support in-kernel module decompression"
> +	depends on MODULE_COMPRESS_GZIP || MODULE_COMPRESS_XZ
> +	select ZLIB_INFLATE if MODULE_COMPRESS_GZIP
> +	select XZ_DEC if MODULE_COMPRESS_XZ

What if MODULE_COMPRESS_GZIP and MODULE_COMPRESS_XZ are enabled?
These are not mutually exclusive.

> +	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.

Shouldn't kernel decompression be faster too? If so, what's the
point of doing it in userspace?

> 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
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2021 Google LLC.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/highmem.h>
> +#include <linux/kobject.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/vmalloc.h>
> +
> +#include "module-internal.h"
> +
> +static int module_extend_max_pages(struct load_info *info, unsigned int extent)
> +{
> +	struct page **new_pages;
> +
> +	new_pages = kvmalloc_array(info->max_pages + extent,
> +				   sizeof(info->pages), GFP_KERNEL);
> +	if (!new_pages)
> +		return -ENOMEM;
> +
> +	memcpy(new_pages, info->pages, info->max_pages * sizeof(info->pages));
> +	kvfree(info->pages);
> +	info->pages = new_pages;
> +	info->max_pages += extent;
> +
> +	return 0;
> +}
> +
> +static struct page *module_get_next_page(struct load_info *info)
> +{
> +	struct page *page;
> +	int error;
> +
> +	if (info->max_pages == info->used_pages) {
> +		error = module_extend_max_pages(info, info->used_pages);
> +		if (error)
> +			return ERR_PTR(error);
> +	}
> +
> +	page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
> +	if (!page)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->pages[info->used_pages++] = page;
> +	return page;
> +}
> +
> +#ifdef CONFIG_MODULE_COMPRESS_GZIP
> +#include <linux/zlib.h>
> +#define MODULE_COMPRESSION	gzip
> +#define MODULE_DECOMPRESS_FN	module_gzip_decompress

So gzip is assumed if your kernel has both gzip and xz. That seems odd.

<-- snip -->

> +#elif CONFIG_MODULE_COMPRESS_XZ
> +#include <linux/xz.h>
> +#define MODULE_COMPRESSION	xz
> +#define MODULE_DECOMPRESS_FN	module_xz_decompress
> +#else

<-- snip -->

> +#error "Unexpected configuration for CONFIG_MODULE_DECOMPRESS"

Using "depends on" logic on the kconfig symbol would resolve this and
make this not needed.

Why can't we just inspect the module and determine? Or, why not just add
a new kconfig symbol under MODULE_DECOMPRESS which lets you specify the
MODULE_COMPRESSION_TYPE. This way this is explicit.

> +module_init(module_decompress_sysfs_init);

This seems odd, altough it works, can you use late_initcall instead()?

  Luis