Message-Id: <20221006224212.569555-6-gpiccoli@igalia.com>
Date:   Thu,  6 Oct 2022 19:42:09 -0300
From:   "Guilherme G. Piccoli" <gpiccoli@...lia.com>
To:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Cc:     kernel-dev@...lia.com, kernel@...ccoli.net, keescook@...omium.org,
        anton@...msg.org, ccross@...roid.com, tony.luck@...el.com,
        "Guilherme G. Piccoli" <gpiccoli@...lia.com>,
        Ard Biesheuvel <ardb@...nel.org>
Subject: [PATCH 5/8] pstore: Fix long-term implicit conversions in the compression routines

The pstore infrastructure is capable of compressing collected logs,
relying for that in many compression "libraries" present on kernel.
Happens that the (de)compression code in pstore performs many
implicit conversions from unsigned int/size_t to int, and vice-versa.
Specially in the compress buffer size calculation, we notice that
even the libs are not consistent, some of them return int, most of
them unsigned int and others rely on preprocessor calculation.

Here is an attempt to make it consistent: since we're talking
about buffer sizes, let's go with unsigned types, since negative
sizes don't make sense.

While at it, improve pstore_compress() return semantic too. This
function returns either some potential error or the size of the
compressed buffer. Such output size is a parameter, so changing it
to a pointer makes sense, even follows the compression libraries
API (that does modify its parameter with the output size).

In order to remove such ambiguity in the return of the function,
was required to validate that all compression libraries return
either 0 on success or some negative value on error - our analysis
showed that all compress libraries potentially used by pstore
do respect that [kudos to sw842_compress() as the most convoluted
return semantic among them all!].

Cc: Ard Biesheuvel <ardb@...nel.org>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@...lia.com>
---
 fs/pstore/platform.c | 46 ++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index ee50812fdd2e..c10bfb8346fe 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -98,7 +98,7 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
 static struct crypto_comp *tfm;
 
 struct pstore_zbackend {
-	int (*zbufsize)(size_t size);
+	unsigned int (*zbufsize)(size_t size);
 	const char *name;
 };
 
@@ -169,9 +169,9 @@ 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)
+static unsigned int zbufsize_deflate(size_t size)
 {
-	size_t cmpr;
+	unsigned int cmpr;
 
 	switch (size) {
 	/* buffer range for efivars */
@@ -198,28 +198,28 @@ static int zbufsize_deflate(size_t size)
 #endif
 
 #if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-static int zbufsize_lzo(size_t size)
+static unsigned 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)
+static unsigned int zbufsize_lz4(size_t size)
 {
-	return LZ4_compressBound(size);
+	return (unsigned int)LZ4_compressBound(size);
 }
 #endif
 
 #if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-static int zbufsize_842(size_t size)
+static unsigned int zbufsize_842(size_t size)
 {
 	return size;
 }
 #endif
 
 #if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-static int zbufsize_zstd(size_t size)
+static unsigned int zbufsize_zstd(size_t size)
 {
 	return zstd_compress_bound(size);
 }
@@ -267,27 +267,27 @@ static const struct pstore_zbackend zbackends[] = {
 	{ }
 };
 
-static int pstore_compress(const void *in, void *out,
-			   unsigned int inlen, unsigned int outlen)
+static bool pstore_compress(const void *in, void *out,
+			   unsigned int inlen, unsigned int *outlen)
 {
 	int ret;
 
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS))
 		return -EINVAL;
 
-	ret = crypto_comp_compress(tfm, in, inlen, out, &outlen);
+	ret = crypto_comp_compress(tfm, in, inlen, out, outlen);
 	if (ret) {
 		pr_err("crypto_comp_compress failed, ret = %d!\n", ret);
-		return ret;
+		return false;
 	}
 
-	return outlen;
+	return true;
 }
 
 static void allocate_buf_for_compression(void)
 {
 	struct crypto_comp *ctx;
-	int size;
+	unsigned int size;
 	char *buf;
 
 	/* Skip if not built-in or compression backend not selected yet. */
@@ -304,15 +304,15 @@ static void allocate_buf_for_compression(void)
 	}
 
 	size = zbackend->zbufsize(psinfo->bufsize);
-	if (size <= 0) {
-		pr_err("Invalid compression size for %s: %d\n",
+	if (!size) {
+		pr_err("Invalid compression size for %s: %u\n",
 		       zbackend->name, size);
 		return;
 	}
 
 	buf = kmalloc(size, GFP_KERNEL);
 	if (!buf) {
-		pr_err("Failed %d byte compression buffer allocation for: %s\n",
+		pr_err("Failed %u byte compression buffer allocation for: %s\n",
 		       size, zbackend->name);
 		return;
 	}
@@ -414,7 +414,6 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		char *dst;
 		size_t dst_size;
 		int header_size;
-		int zipped_len = -1;
 		size_t dump_size;
 		struct pstore_record record;
 
@@ -444,13 +443,10 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 			break;
 
 		if (big_oops_buf) {
-			zipped_len = pstore_compress(dst, psinfo->buf,
-						header_size + dump_size,
-						psinfo->bufsize);
-
-			if (zipped_len > 0) {
+			record.size = psinfo->bufsize;
+			if (pstore_compress(dst, psinfo->buf,
+			    header_size + dump_size, (unsigned int *)&record.size)) {
 				record.compressed = true;
-				record.size = zipped_len;
 			} else {
 				record.size = copy_kmsg_to_buffer(header_size,
 								  dump_size);
@@ -677,7 +673,7 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
 static void decompress_record(struct pstore_record *record)
 {
 	int ret;
-	int unzipped_len;
+	unsigned int unzipped_len;
 	char *unzipped, *workspace;
 
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !record->compressed)
-- 
2.38.0

Powered by blists - more mailing lists