linux-kernel - Re: [PATCH] zram: don't copy invalid compression algorithms

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]

Date:	Tue, 8 Sep 2015 18:54:28 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Minchan Kim <minchan@...nel.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Luis Henriques <luis.henriques@...onical.com>,
	linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [PATCH] zram: don't copy invalid compression algorithms

On (09/08/15 17:16), Minchan Kim wrote:
> we should help them to *correct* it rather than keeping such weired
> thing.

A simple quiz

A)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
-bash: echo: write error: Invalid argument
echo $?
1

B)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
echo $?
0

which one *DOES* help finding and correcting an error and which one *DOES NOT*?
a million dolla question.

the difference between comp_algorithm store and any other store function - is
that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory,
free memory, register or unregister anything, change backends, etc., etc., etc.
it does none of those. its only purpose is to strcpy() given data. this data
will be used later by a completely different function as a result of additional
actions taken by user space.

Returning back to our quiz. I do suspect that the answer is... "B"!

So, I still NACK the patch. It introduces a goto label, etc. In fact all
we need to do is to move zcomp_available_algorithm() up, before we grab
the ->init_lock. zcomp_available_algorithm() does not depend on anything
that requires a ->init_lock protection.

Next, the patch lacks a reasoning/motivation in its commit message. What
we do in fact here is we introduce compression algorithm fallback to a
previously selected (knowingly supported, which has already passed
zcomp_available_algorithm()) or the `default_compressor'.

Summarizing, it's something like this:

---

From: Sergey SENOZHATSKY <sergey.senozhatsky@...il.com>
Subject: [PATCH] zram: introduce comp algorithm fallback functionality

When user supplies an unsupported compression algorithm, keep
a previously selected one (knowingly supported) or the default
one (in case if compression algorithm hasn't been changed before).
---
 drivers/block/zram/zram_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 55e09db..255d68b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	struct zram *zram = dev_to_zram(dev);
 	size_t sz;

+	if (!zcomp_available_algorithm(buf))
+		return -EINVAL;
+
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
@@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	if (sz > 0 && zram->compressor[sz - 1] == '\n')
 		zram->compressor[sz - 1] = 0x00;

-	if (!zcomp_available_algorithm(zram->compressor))
-		len = -EINVAL;
-
 	up_write(&zram->init_lock);
 	return len;
 }
-- 
2.5.1.454.g1616360

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/