linux-kernel - Re: [PATCH] module/decompress: use vmalloc() for zstd decompression workspace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZO4u+6atPevjudZm@righiandr-XPS-13-7390>
Date:   Tue, 29 Aug 2023 19:46:35 +0200
From:   Andrea Righi <andrea.righi@...onical.com>
To:     Lucas De Marchi <lucas.demarchi@...el.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Nick Terrell <terrelln@...com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Piotr Gorski <lucjan.lucjanov@...il.com>,
        linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] module/decompress: use vmalloc() for zstd decompression
 workspace

On Tue, Aug 29, 2023 at 10:30:55AM -0700, Lucas De Marchi wrote:
> On Tue, Aug 29, 2023 at 09:41:32AM -0700, Luis Chamberlain wrote:
> > On Tue, Aug 29, 2023 at 02:05:08PM +0200, Andrea Righi wrote:
> > > Using kmalloc() to allocate the decompression workspace for zstd may
> > > trigger the following warning when large modules are loaded (i.e., xfs):
> > > 
> > > [    2.961884] WARNING: CPU: 1 PID: 254 at mm/page_alloc.c:4453 __alloc_pages+0x2c3/0x350
> > > ...
> > > [    2.989033] Call Trace:
> > > [    2.989841]  <TASK>
> > > [    2.990614]  ? show_regs+0x6d/0x80
> > > [    2.991573]  ? __warn+0x89/0x160
> > > [    2.992485]  ? __alloc_pages+0x2c3/0x350
> > > [    2.993520]  ? report_bug+0x17e/0x1b0
> > > [    2.994506]  ? handle_bug+0x51/0xa0
> > > [    2.995474]  ? exc_invalid_op+0x18/0x80
> > > [    2.996469]  ? asm_exc_invalid_op+0x1b/0x20
> > > [    2.997530]  ? module_zstd_decompress+0xdc/0x2a0
> > > [    2.998665]  ? __alloc_pages+0x2c3/0x350
> > > [    2.999695]  ? module_zstd_decompress+0xdc/0x2a0
> > > [    3.000821]  __kmalloc_large_node+0x7a/0x150
> > > [    3.001920]  __kmalloc+0xdb/0x170
> > > [    3.002824]  module_zstd_decompress+0xdc/0x2a0
> > > [    3.003857]  module_decompress+0x37/0xc0
> > > [    3.004688]  init_module_from_file+0xd0/0x100
> > > [    3.005668]  idempotent_init_module+0x11c/0x2b0
> > > [    3.006632]  __x64_sys_finit_module+0x64/0xd0
> > > [    3.007568]  do_syscall_64+0x59/0x90
> > > [    3.008373]  ? ksys_read+0x73/0x100
> > > [    3.009395]  ? exit_to_user_mode_prepare+0x30/0xb0
> > > [    3.010531]  ? syscall_exit_to_user_mode+0x37/0x60
> > > [    3.011662]  ? do_syscall_64+0x68/0x90
> > > [    3.012511]  ? do_syscall_64+0x68/0x90
> > > [    3.013364]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> > > 
> > > However, continuous physical memory does not seem to be required in
> > > module_zstd_decompress(), so use vmalloc() instead, to prevent the
> > > warning and avoid potential failures at loading compressed modules.
> > > 
> > > Fixes: 169a58ad824d ("module/decompress: Support zstd in-kernel decompression")
> > > Signed-off-by: Andrea Righi <andrea.righi@...onical.com>
> > > ---
> > >  kernel/module/decompress.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/module/decompress.c b/kernel/module/decompress.c
> > > index 8a5d6d63b06c..87440f714c0c 100644
> > > --- a/kernel/module/decompress.c
> > > +++ b/kernel/module/decompress.c
> > > @@ -241,7 +241,7 @@ static ssize_t module_zstd_decompress(struct load_info *info,
> > >  	}
> > > 
> > >  	wksp_size = zstd_dstream_workspace_bound(header.windowSize);
> > > -	wksp = kmalloc(wksp_size, GFP_KERNEL);
> > > +	wksp = vmalloc(wksp_size);
> > >  	if (!wksp) {
> > >  		retval = -ENOMEM;
> > >  		goto out;
> > > @@ -284,7 +284,7 @@ static ssize_t module_zstd_decompress(struct load_info *info,
> > >  	retval = new_size;
> > > 
> > >   out:
> > > -	kfree(wksp);
> > > +	vfree(wksp);
> > >  	return retval;
> > 
> > Thanks! Applied and queued up.
> 
> I can see at least the gz decompress would need the same kind of change.
> Shouldn't we tackle them all at once?

gz decompress needs to allocate a struct inflate_workspace, that is not
too bad (11 pages on my system), but it also seems safer to just use
vmalloc():

struct inflate_workspace {
	struct inflate_state       inflate_state;        /*     0  9544 */
	/* --- cacheline 149 boundary (9536 bytes) was 8 bytes ago --- */
	unsigned char              working_window[32768]; /*  9544 32768 */

	/* size: 42312, cachelines: 662, members: 2 */
	/* last cacheline: 8 bytes */
};

xz is also using kmalloc() internally in xz_dec_init() to allocate
struct xz_dec that seems to be less than a page, so kmalloc() should be
fine in this case:

struct xz_dec {
...
	/* size: 1232, cachelines: 20, members: 16 */
...
}

In conclusion I think we should be pretty safe for now by just changing
gz and zstd.

Maybe having two separate patches is better (in case we need to revert
just one for any reason...)?

-Andrea