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

2017-11-21 Thread David Sterba 
On Thu, Nov 16, 2017 at 08:49:47AM +0800, Qu Wenruo wrote:
> > 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-11-15 Thread Qu Wenruo 


On 2017年11月15日 23:11, David Sterba wrote:
> On Mon, Nov 06, 2017 at 10:43:18AM +0800, Qu Wenruo wrote:
>> [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.
> 
> 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

> 
>> @@ -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);
> 
> At least this will not screw up the levels, anything that's not
> recognized will become the default.
> 
>>  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



signature.asc
Description: OpenPGP digital signature

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

2017-11-15 Thread David Sterba 
On Mon, Nov 06, 2017 at 10:43:18AM +0800, Qu Wenruo wrote:
> [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.

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.

> @@ -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);

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

>   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2017-11-06 Thread Lu Fengqi 
On Mon, Nov 06, 2017 at 10:43:18AM +0800, Qu Wenruo wrote:
>[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 

Reviewed-by: Lu Fengqi 

-- 
Thanks,
Lu

>---
> fs/btrfs/super.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>index 65af029559b5..14258671da84 100644
>--- a/fs/btrfs/super.c
>+++ b/fs/btrfs/super.c
>@@ -42,6 +42,7 @@
> #include 
> #include 
> #include 
>+#include 
> #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);
>-- 
>2.14.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>


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

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

2017-11-05 Thread 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.

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

Signed-off-by: Qu Wenruo 
---
 fs/btrfs/super.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 65af029559b5..14258671da84 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 #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);
-- 
2.14.3

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