Date:   Fri, 10 Dec 2021 17:09:23 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
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: <YbP6Q9J++OVKqPfn@google.com>
References: <YbLvDWdyFivlj7pP@google.com>
 <YbPsqR5ZyiFwJul3@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbPsqR5ZyiFwJul3@bombadil.infradead.org>
Precedence: bulk

On Fri, Dec 10, 2021 at 04:11:21PM -0800, Luis Chamberlain wrote:
> 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.

They are mutually exclusive, the kernel uses the same (one) compression
method for all kernel modules that it generates (i.e we do not compress
drivers/usb/... with gzip while drivers/net/... with xz).

The idea here is to allow the kernel consume the same format that was
used when generating modules. Supporting multiple formats at once is
overkill IMO.

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

Make the kernel smaller? Have more flexibility with exotic compression
formats?

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

No, gzuo is used when CONFIG_MODULE_COMPRESS_GZIP is enabled. That means
that CONFIG_MODULE_COMPRESS_XZ is not enabled.

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

It is a possibility, I just wanted to decompression scheme match
compression one so there is less potential for misconfiguration.

> 
> > +module_init(module_decompress_sysfs_init);
> 
> This seems odd, altough it works, can you use late_initcall instead()?

Yes, I can change this.

Thanks.

-- 
Dmitry