Re: [PATCH] module: Add LZ4 module compression

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] module: int retval overflow and truncation in
  module_lz4_decompress()
- [Low] module: 0-byte LZ4 frame causes vmap() -ENOMEM in
  module_lz4_decompress()
--

commit 5ec28e23c9b1ef7c279c61b63187aad310dbdc6c
Author: Paul Menzel <pmenzel@molgen.mpg.de>

module: Add LZ4 module compression

This commit adds build and in-kernel decompression support for LZ4-compressed
kernel modules (.ko.lz4).

Link: https://github.com/kmod-project/kmod/pull/505 [1]

> diff --git a/kernel/module/decompress.c b/kernel/module/decompress.c
> index cce098671be97..5828b7ec9d7b6 100644
> --- a/kernel/module/decompress.c
> +++ b/kernel/module/decompress.c

[ ... ]

> +static int module_lz4_verify_content(struct load_info *info, size_t total,
> +				     u32 expected)
> +{
> +	void *data;
> +	u32 csum;
> +
> +	data = vmap(info->pages, info->used_pages, VM_MAP, PAGE_KERNEL_RO);
> +	if (!data)
> +		return -ENOMEM;
> +	csum = xxh32(data, total, 0);
> +	vunmap(data);
> +
> +	if (csum != expected) {
> +		pr_err("LZ4-compressed data is corrupt (content checksum)\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
What happens here when an LZ4 frame decompresses to 0 bytes?

Unlike the gzip, xz and zstd decompressors, which allocate a page before
starting the loop, module_lz4_decompress() only calls module_get_next_page()
inside module_lz4_copy_out() while len is non-zero.  An empty LZ4 frame hits
the zero block_size end mark with info->used_pages == 0, so vmap() in
module_lz4_verify_content() (and in module_decompress() when content
checksums are disabled) returns NULL and fails with -ENOMEM instead of
rejecting the empty module.

> +
> +static ssize_t module_lz4_decompress(struct load_info *info,
> +				     const void *buf, size_t size)
> +{
> +	const u8 *in = buf, *end = in + size, *desc;
> +	size_t block_max, total = 0, dict_len = 0;
> +	u64 content_size = 0;
> +	u8 *scratch, *out;
> +	bool linked;
> +	u8 flg, bd;
> +	int retval;

[ ... ]

> +	if ((flg & LZ4F_FLG_CONTENT_SIZE) && content_size != total) {
> +		pr_err("LZ4-compressed data has a wrong content size\n");
> +		retval = -EINVAL;
> +		goto out;
> +	}
> +
> +	retval = total;
> +	goto out;

[Severity: Medium]
Can assigning total to int retval overflow or truncate the decompressed size?

module_lz4_decompress() returns ssize_t, but retval is declared as int.  If a
highly compressible LZ4 frame expands to 2 GiB or more, assigning size_t
total to retval either overflows into a negative non-errno value returned
from finit_module() or truncates the upper 32 bits on 64-bit systems, leaving
info->len smaller than the vmap()'d pages in info->hdr.  Declaring retval as
ssize_t (like module_gzip_decompress() and module_xz_decompress()) and
bounding total would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113144233.193782-1-pmenzel@molgen.mpg.de?part=1