Skip to contentSkip to conversation

btrfs: Fix wild memory access in compression level parser

PATCH In review Qu Wenruo 4 participants 6 messages Nov 6, 2017 – Nov 21, 2017 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/2 btrfs: Fix wild memory access in compression level parser

[BUG]

Kernel panic when mounting with "-o compress" mount option.
KASAN will report like:
------
==================================================================
BUG: KASAN: wild-memory-access in strncmp+0x31/0xc0
Read of size 1 at addr d86735fce994f800 by task mount/662
...
Call Trace:
 dump_stack+0xe3/0x175
 kasan_report+0x163/0x370
 __asan_load1+0x47/0x50
 strncmp+0x31/0xc0
 btrfs_compress_str2level+0x20/0x70 [btrfs]
 btrfs_parse_options+0xff4/0x1870 [btrfs]
 open_ctree+0x2679/0x49f0 [btrfs]
 btrfs_mount+0x1b7f/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 vfs_kern_mount+0x13/0x20
 btrfs_mount+0x31e/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 do_mount+0xaad/0x1a00
 SyS_mount+0x98/0xe0
 entry_SYSCALL_64_fastpath+0x1f/0xbe

------

[Cause]
For 'compress' and 'compress_force' options, its token doesn't expect any parameter so its args[0] contains uninitialized data. Accessing args[0] will cause above wild memory access.

[Fix]
For Opt_compress and Opt_compress_force, set compression level to Z_DEFAULT_COMPRESSION manually.

NOTE: Don't set zlib compression level to 0 by default, which means no compression.

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/super.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
fs/btrfs/super.c +13 -1
index 65af029559b5..14258671da84 100644
@@ -42,6 +42,7 @@
 #include <linux/cleancache.h>
 #include <linux/ratelimit.h>
 #include <linux/btrfs.h>
+#include <linux/zlib.h>
 #include "delayed-inode.h"
 #include "ctree.h"
 #include "disk-io.h"

@@ -507,8 +508,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			    token == Opt_compress_force ||
 			    strncmp(args[0].from, "zlib", 4) == 0) {
 				compress_type = "zlib";
+
 				info->compress_type = BTRFS_COMPRESS_ZLIB;
-				info->compress_level =
+				/*
+				 * args[0] contains uninitialized data since
+				 * for these tokens we don't expect any
+				 * parameter.
+				 */
+				if (token == Opt_compress ||
+				    token == Opt_compress_force)
+					info->compress_level =
+						Z_DEFAULT_COMPRESSION;
+				else
+					info->compress_level =
 					btrfs_compress_str2level(args[0].from);
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH 2/2 btrfs: compression: Return correct default zlib compression level

0 means no compression at all for zlib. Z_DEFAULT_COMPRESSION is the correct value, which is -1 and will be converted to 6 at runtime.

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/compression.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
fs/btrfs/compression.c +6 -1
index b35ce16b3df3..29a41e6c9e28 100644
@@ -35,6 +35,7 @@
 #include <linux/sched/mm.h>
 #include <linux/sort.h>
 #include <linux/log2.h>
+#include <linux/zlib.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "transaction.h"

@@ -1528,5 +1529,9 @@ unsigned int btrfs_compress_str2level(const char *str)
 	if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0)
 		return str[5] - '0';
 
-	return 0;
+	/*
+	 * NOTE: Default compression level is not 0!!
+	 * 0 means no compression at all
+	 */
+	return Z_DEFAULT_COMPRESSION;
 }
Signature · 1 line
2.14.3
Reviewed-by on v1 1/2
#2 lore raw
In reply to Qu Wenruo
Quoted 33 lines · Qu Wenruo

[BUG]

Kernel panic when mounting with "-o compress" mount option.
KASAN will report like:
------
==================================================================
BUG: KASAN: wild-memory-access in strncmp+0x31/0xc0
Read of size 1 at addr d86735fce994f800 by task mount/662
...
Call Trace:
dump_stack+0xe3/0x175
kasan_report+0x163/0x370
__asan_load1+0x47/0x50
strncmp+0x31/0xc0
btrfs_compress_str2level+0x20/0x70 [btrfs]
btrfs_parse_options+0xff4/0x1870 [btrfs]
open_ctree+0x2679/0x49f0 [btrfs]
btrfs_mount+0x1b7f/0x1d30 [btrfs]
mount_fs+0x49/0x190
vfs_kern_mount.part.29+0xba/0x280
vfs_kern_mount+0x13/0x20
btrfs_mount+0x31e/0x1d30 [btrfs]
mount_fs+0x49/0x190
vfs_kern_mount.part.29+0xba/0x280
do_mount+0xaad/0x1a00
SyS_mount+0x98/0xe0
entry_SYSCALL_64_fastpath+0x1f/0xbe

------

[Cause]
For 'compress' and 'compress_force' options, its token doesn't expect any parameter so its args[0] contains uninitialized data. Accessing args[0] will cause above wild memory access.

Quoted 8 lines

[Fix]
For Opt_compress and Opt_compress_force, set compression level to Z_DEFAULT_COMPRESSION manually.

NOTE: Don't set zlib compression level to 0 by default, which means no compression.

Signed-off-by Qu Wenruo <[email protected]>
Reviewed-by Lu Fengqi <[email protected]>
Signature · 2 lines
Thanks,
Lu
Quoted 46 lines

---
fs/btrfs/super.c | 14 +++++++++++++-

1 file changed, 13 insertions(+), 1 deletion(-)
fs/btrfs/super.c +13 -1
index 65af029559b5..14258671da84 100644
@@ -42,6 +42,7 @@
 #include <linux/cleancache.h>
 #include <linux/ratelimit.h>
 #include <linux/btrfs.h>
+#include <linux/zlib.h>
 #include "delayed-inode.h"
 #include "ctree.h"
 #include "disk-io.h"

@@ -507,8 +508,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			    token == Opt_compress_force ||
 			    strncmp(args[0].from, "zlib", 4) == 0) {
 				compress_type = "zlib";
+
 				info->compress_type = BTRFS_COMPRESS_ZLIB;
-				info->compress_level =
+				/*
+				 * args[0] contains uninitialized data since
+				 * for these tokens we don't expect any
+				 * parameter.
+				 */
+				if (token == Opt_compress ||
+				    token == Opt_compress_force)
+					info->compress_level =
+						Z_DEFAULT_COMPRESSION;
+				else
+					info->compress_level =
 					btrfs_compress_str2level(args[0].from);
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);
Signature · 6 lines
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw
In reply to Qu Wenruo
Quoted 33 lines · Qu Wenruo

[BUG]

Kernel panic when mounting with "-o compress" mount option.
KASAN will report like:
------
==================================================================
BUG: KASAN: wild-memory-access in strncmp+0x31/0xc0
Read of size 1 at addr d86735fce994f800 by task mount/662
...
Call Trace:
 dump_stack+0xe3/0x175
 kasan_report+0x163/0x370
 __asan_load1+0x47/0x50
 strncmp+0x31/0xc0
 btrfs_compress_str2level+0x20/0x70 [btrfs]
 btrfs_parse_options+0xff4/0x1870 [btrfs]
 open_ctree+0x2679/0x49f0 [btrfs]
 btrfs_mount+0x1b7f/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 vfs_kern_mount+0x13/0x20
 btrfs_mount+0x31e/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 do_mount+0xaad/0x1a00
 SyS_mount+0x98/0xe0
 entry_SYSCALL_64_fastpath+0x1f/0xbe

------

[Cause]
For 'compress' and 'compress_force' options, its token doesn't expect any parameter so its args[0] contains uninitialized data. Accessing args[0] will cause above wild memory access.

Quoted 6 lines

[Fix]
For Opt_compress and Opt_compress_force, set compression level to Z_DEFAULT_COMPRESSION manually.

NOTE: Don't set zlib compression level to 0 by default, which means no compression.

But we never set the level to 0 at the point the compression actually happens. See zlib.c:zlib_set_level, if level is 0 then the level passed to zlib is 3. Z_DEFAULT_COMPRESSION is upstream zlib level 6, which is slower, we need zlib to stay in the real-time numbers.

Quoted 9 lines
fs/btrfs/super.c +3 -1
@@ -507,8 +508,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			    token == Opt_compress_force ||
 			    strncmp(args[0].from, "zlib", 4) == 0) {
 				compress_type = "zlib";
+
 				info->compress_type = BTRFS_COMPRESS_ZLIB;
-				info->compress_level =
+				/*
+				 * args[0] contains uninitialized data since

fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 524

Quoted 10 lines
fs/btrfs/super.c +9 -0
+				 * for these tokens we don't expect any
+				 * parameter.
+				 */
+				if (token == Opt_compress ||
+				    token == Opt_compress_force)
+					info->compress_level =
+						Z_DEFAULT_COMPRESSION;
+				else
+					info->compress_level =
 					btrfs_compress_str2level(args[0].from);

At least this will not screw up the levels, anything that's not recognized will become the default.

Quoted 2 lines
fs/btrfs/super.c +0 -0
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);
#4 lore raw
Quoted 38 lines · David Sterba
Quoted 37 lines · Qu Wenruo

[BUG]

Kernel panic when mounting with "-o compress" mount option.
KASAN will report like:
------
==================================================================
BUG: KASAN: wild-memory-access in strncmp+0x31/0xc0
Read of size 1 at addr d86735fce994f800 by task mount/662
...
Call Trace:
 dump_stack+0xe3/0x175
 kasan_report+0x163/0x370
 __asan_load1+0x47/0x50
 strncmp+0x31/0xc0
 btrfs_compress_str2level+0x20/0x70 [btrfs]
 btrfs_parse_options+0xff4/0x1870 [btrfs]
 open_ctree+0x2679/0x49f0 [btrfs]
 btrfs_mount+0x1b7f/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 vfs_kern_mount+0x13/0x20
 btrfs_mount+0x31e/0x1d30 [btrfs]
 mount_fs+0x49/0x190
 vfs_kern_mount.part.29+0xba/0x280
 do_mount+0xaad/0x1a00
 SyS_mount+0x98/0xe0
 entry_SYSCALL_64_fastpath+0x1f/0xbe

------

[Cause]
For 'compress' and 'compress_force' options, its token doesn't expect any parameter so its args[0] contains uninitialized data. Accessing args[0] will cause above wild memory access.

[Fix]
For Opt_compress and Opt_compress_force, set compression level to Z_DEFAULT_COMPRESSION manually.

Quoted 7 lines
Quoted 2 lines

NOTE: Don't set zlib compression level to 0 by default, which means no compression.

But we never set the level to 0 at the point the compression actually happens. See zlib.c:zlib_set_level, if level is 0 then the level passed to zlib is 3. Z_DEFAULT_COMPRESSION is upstream zlib level 6, which is slower, we need zlib to stay in the real-time numbers.

Right, I missed that.

So should I still use 0, or use separate macro like BTRFS_DEFAULT_ZLIB_LEVEL?

Thanks,
Qu

Quoted 31 lines
Quoted 10 lines
fs/btrfs/super.c +3 -1
@@ -507,8 +508,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			    token == Opt_compress_force ||
 			    strncmp(args[0].from, "zlib", 4) == 0) {
 				compress_type = "zlib";
+
 				info->compress_type = BTRFS_COMPRESS_ZLIB;
-				info->compress_level =
+				/*
+				 * args[0] contains uninitialized data since

fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 524

Quoted 10 lines
fs/btrfs/super.c +9 -0
+				 * for these tokens we don't expect any
+				 * parameter.
+				 */
+				if (token == Opt_compress ||
+				    token == Opt_compress_force)
+					info->compress_level =
+						Z_DEFAULT_COMPRESSION;
+				else
+					info->compress_level =
 					btrfs_compress_str2level(args[0].from);

At least this will not screw up the levels, anything that's not recognized will become the default.

fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 526

Quoted 2 lines
fs/btrfs/super.c +0 -0
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 9 lines · Qu Wenruo
Quoted 4 lines

But we never set the level to 0 at the point the compression actually happens. See zlib.c:zlib_set_level, if level is 0 then the level passed to zlib is 3. Z_DEFAULT_COMPRESSION is upstream zlib level 6, which is slower, we need zlib to stay in the real-time numbers.

Right, I missed that.

So should I still use 0, or use separate macro like BTRFS_DEFAULT_ZLIB_LEVEL?

BTRFS_DEFAULT_ZLIB_LEVEL would be better, as this would address
https://patchwork.kernel.org/patch/10021441/