[1/2] pstore: Remove worst-case compression size logic

Commit Message

Ard Biesheuvel July 4, 2023, 1:52 p.m. UTC

From: Kees Cook <[e-mail]>

The worst case compression size gives an upper bound for how much the
data might inadvertently *grow* due to encapsulation overhead if the
input is not compressible at all.

The kernel log is ASCII text so it should generally compress rather
well. This means that the probability that the kernel log grows beyond
the uncompressed size after compression is astronomically low, and in
such cases (i.e., dmesg filled with perfect entropy) we won't be able to
make sense of it anyway.

So let's just drop this logic, and use the uncompressed size as the
worst case instead.

Co-developed-by: Kees Cook <[e-mail]>
Signed-off-by: Kees Cook <[e-mail]>
Signed-off-by: Ard Biesheuvel <[e-mail]>
---
 fs/pstore/platform.c | 153 ++------------------
 1 file changed, 9 insertions(+), 144 deletions(-)

Comments

Eric Biggers July 4, 2023, 6:07 p.m. UTC | #1

On Tue, Jul 04, 2023 at 03:52:10PM +0200, Ard Biesheuvel wrote:
> From: Kees Cook <[e-mail]>
> 
> The worst case compression size gives an upper bound for how much the
> data might inadvertently *grow* due to encapsulation overhead if the
> input is not compressible at all.
> 
> The kernel log is ASCII text so it should generally compress rather
> well. This means that the probability that the kernel log grows beyond
> the uncompressed size after compression is astronomically low, and in
> such cases (i.e., dmesg filled with perfect entropy) we won't be able to
> make sense of it anyway.
> 
> So let's just drop this logic, and use the uncompressed size as the
> worst case instead.
> 
> Co-developed-by: Kees Cook <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>
> Signed-off-by: Ard Biesheuvel <[e-mail]>

I think the reasoning about the kernel log being low-entropy, which is a bit
hand-wavy, is entirely unnecessary.  pstore records include a flag that
indicates whether they are compressed or not.  Therefore, any record that would
compress to more than its original size can and should be stored uncompressed.
There's nothing more to it than that.

>  static int pstore_compress(const void *in, void *out,
>  			   unsigned int inlen, unsigned int outlen)
>  {
> @@ -291,36 +178,31 @@ static void allocate_buf_for_compression(void)
>  	char *buf;
>  
>  	/* Skip if not built-in or compression backend not selected yet. */
> -	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
> +	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
>  		return;
>  
>  	/* Skip if no pstore backend yet or compression init already done. */
>  	if (!psinfo || tfm)
>  		return;
>  
> -	if (!crypto_has_comp(zbackend->name, 0, 0)) {
> -		pr_err("Unknown compression: %s\n", zbackend->name);
> -		return;
> -	}
> -
> -	size = zbackend->zbufsize(psinfo->bufsize);
> -	if (size <= 0) {
> -		pr_err("Invalid compression size for %s: %d\n",
> -		       zbackend->name, size);
> +	if (!crypto_has_comp(compress, 0, 0)) {
> +		pr_err("Unknown compression: %s\n", compress);
>  		return;
>  	}
>  
> +	/* Worst-case compression should never be more than uncompressed. */
> +	size = psinfo->bufsize;
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf) {
>  		pr_err("Failed %d byte compression buffer allocation for: %s\n",
> -		       size, zbackend->name);
> +		       size, compress);
>  		return;
>  	}

The local variable 'size' should be removed, and psinfo->bufsize used directly.

> @@ -330,7 +212,7 @@ static void allocate_buf_for_compression(void)
>  	big_oops_buf_sz = size;

The static variable 'big_oops_buf_sz' should be removed, as it is redundant with
psinfo->bufsize.

>		if (big_oops_buf) {
>			dst = big_oops_buf;
>			dst_size = big_oops_buf_sz;
>		} else {
>			dst = psinfo->buf;
>			dst_size = psinfo->bufsize;
>		}

This can be simplified to:

		if (big_oops_buf)
			dst = big_oops_buf;
		else
			dst = psinfo->buf;
		dst_size = psinfo->bufsize;

>	unzipped_len = big_oops_buf_sz;
>	workspace = kmalloc(unzipped_len + record->ecc_notice_size,
>			    GFP_KERNEL);
>	if (!workspace)
>		return;

This can be simplified to:

	workspace = kmalloc(psinfo->bufsize + record->ecc_notice_size,
			    GFP_KERNEL);
	if (!workspace)
		return;

> /*
>  * Called when compression fails, since the printk buffer
>  * would be fetched for compression calling it again when
>  * compression fails would have moved the iterator of
>  * printk buffer which results in fetching old contents.
>  * Copy the recent messages from big_oops_buf to psinfo->buf
>  */
> static size_t copy_kmsg_to_buffer(int hsize, size_t len)
> {
> 	size_t total_len;
> 	size_t diff;
> 
> 	total_len = hsize + len;
> 
> 	if (total_len > psinfo->bufsize) {
> 		diff = total_len - psinfo->bufsize + hsize;
> 		memcpy(psinfo->buf, big_oops_buf, hsize);
> 		memcpy(psinfo->buf + hsize, big_oops_buf + diff,
> 					psinfo->bufsize - hsize);
> 		total_len = psinfo->bufsize;
> 	} else
> 		memcpy(psinfo->buf, big_oops_buf, total_len);
> 
> 	return total_len;
> }

This patch makes the 'total_len > psinfo->bufsize' case in the above function
unreachable.  That function should be removed, and its caller should just do:

			zipped_len = pstore_compress(dst, psinfo->buf,
						header_size + dump_size,
						psinfo->bufsize);

			if (zipped_len > 0) {
				record.compressed = true;
				record.size = zipped_len;
			} else {
				memcpy(psinfo->buf, dst,
				       header_size + dump_size);
				record.size = header_size + dump_size;
			}


- Eric

Patch

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab6ca1..6741ec4347a3eea9 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -16,15 +16,6 @@ 
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pstore.h>
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-#include <linux/lzo.h>
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS) || IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-#include <linux/lz4.h>
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-#include <linux/zstd.h>
-#endif
 #include <linux/crypto.h>
 #include <linux/string.h>
 #include <linux/timer.h>
@@ -97,11 +88,6 @@  MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
 /* Compression parameters */
 static struct crypto_comp *tfm;
 
-struct pstore_zbackend {
-	int (*zbufsize)(size_t size);
-	const char *name;
-};
-
 static char *big_oops_buf;
 static size_t big_oops_buf_sz;
 
@@ -168,105 +154,6 @@  static bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
 	}
 }
 
-#if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
-static int zbufsize_deflate(size_t size)
-{
-	size_t cmpr;
-
-	switch (size) {
-	/* buffer range for efivars */
-	case 1000 ... 2000:
-		cmpr = 56;
-		break;
-	case 2001 ... 3000:
-		cmpr = 54;
-		break;
-	case 3001 ... 3999:
-		cmpr = 52;
-		break;
-	/* buffer range for nvram, erst */
-	case 4000 ... 10000:
-		cmpr = 45;
-		break;
-	default:
-		cmpr = 60;
-		break;
-	}
-
-	return (size * 100) / cmpr;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-static int zbufsize_lzo(size_t size)
-{
-	return lzo1x_worst_compress(size);
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS) || IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-static int zbufsize_lz4(size_t size)
-{
-	return LZ4_compressBound(size);
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-static int zbufsize_842(size_t size)
-{
-	return size;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-static int zbufsize_zstd(size_t size)
-{
-	return zstd_compress_bound(size);
-}
-#endif
-
-static const struct pstore_zbackend *zbackend __ro_after_init;
-
-static const struct pstore_zbackend zbackends[] = {
-#if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
-	{
-		.zbufsize	= zbufsize_deflate,
-		.name		= "deflate",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lzo,
-		.name		= "lzo",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lz4,
-		.name		= "lz4",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-	{
-		.zbufsize	= zbufsize_lz4,
-		.name		= "lz4hc",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-	{
-		.zbufsize	= zbufsize_842,
-		.name		= "842",
-	},
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-	{
-		.zbufsize	= zbufsize_zstd,
-		.name		= "zstd",
-	},
-#endif
-	{ }
-};
-
 static int pstore_compress(const void *in, void *out,
 			   unsigned int inlen, unsigned int outlen)
 {
@@ -291,36 +178,31 @@  static void allocate_buf_for_compression(void)
 	char *buf;
 
 	/* Skip if not built-in or compression backend not selected yet. */
-	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
+	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
 		return;
 
 	/* Skip if no pstore backend yet or compression init already done. */
 	if (!psinfo || tfm)
 		return;
 
-	if (!crypto_has_comp(zbackend->name, 0, 0)) {
-		pr_err("Unknown compression: %s\n", zbackend->name);
-		return;
-	}
-
-	size = zbackend->zbufsize(psinfo->bufsize);
-	if (size <= 0) {
-		pr_err("Invalid compression size for %s: %d\n",
-		       zbackend->name, size);
+	if (!crypto_has_comp(compress, 0, 0)) {
+		pr_err("Unknown compression: %s\n", compress);
 		return;
 	}
 
+	/* Worst-case compression should never be more than uncompressed. */
+	size = psinfo->bufsize;
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf) {
 		pr_err("Failed %d byte compression buffer allocation for: %s\n",
-		       size, zbackend->name);
+		       size, compress);
 		return;
 	}
 
-	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+	ctx = crypto_alloc_comp(compress, 0, 0);
 	if (IS_ERR_OR_NULL(ctx)) {
 		kfree(buf);
-		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+		pr_err("crypto_alloc_comp('%s') failed: %ld\n", compress,
 		       PTR_ERR(ctx));
 		return;
 	}
@@ -330,7 +212,7 @@  static void allocate_buf_for_compression(void)
 	big_oops_buf_sz = size;
 	big_oops_buf = buf;
 
-	pr_info("Using crash dump compression: %s\n", zbackend->name);
+	pr_info("Using crash dump compression: %s\n", compress);
 }
 
 static void free_buf_for_compression(void)
@@ -818,27 +700,10 @@  static void pstore_timefunc(struct timer_list *unused)
 	pstore_timer_kick();
 }
 
-static void __init pstore_choose_compression(void)
-{
-	const struct pstore_zbackend *step;
-
-	if (!compress)
-		return;
-
-	for (step = zbackends; step->name; step++) {
-		if (!strcmp(compress, step->name)) {
-			zbackend = step;
-			return;
-		}
-	}
-}
-
 static int __init pstore_init(void)
 {
 	int ret;
 
-	pstore_choose_compression();
-
 	/*
 	 * Check if any pstore backends registered earlier but did not
 	 * initialize compression because crypto was not ready. If so,