On 6.06.19 г. 10:49 ч., Naohiro Aota wrote:
> xattr value is not NULL-terminated string. When you specify "lz" as the
> property value, strncmp("lzo", value, 3) will try to read one byte after
> the value buffer, causing the following OOB access. Fix this out-of-bound
> by explicitly check the required length.
>
> [ 1519.998589] ==================================================================
> [ 1520.007505] BUG: KASAN: slab-out-of-bounds in strncmp+0xab/0xc0
> [ 1520.015116] Read of size 1 at addr ffff8886daec16c2 by task btrfs/15317
>
> [ 1520.026628] CPU: 4 PID: 15317 Comm: btrfs Not tainted 5.1.0-rc7+ #3
> [ 1520.034635] Hardware name: Supermicro X10SLL-F/X10SLL-F, BIOS 3.0 04/24/2015
> [ 1520.043416] Call Trace:
> [ 1520.047562] dump_stack+0x71/0xa0
> [ 1520.052584] print_address_description+0x65/0x229
> [ 1520.058997] ? strncmp+0xab/0xc0
> [ 1520.063929] ? strncmp+0xab/0xc0
> [ 1520.068834] kasan_report.cold+0x1a/0x38
> [ 1520.074439] ? strncmp+0xab/0xc0
> [ 1520.079343] strncmp+0xab/0xc0
> [ 1520.084110] prop_compression_validate+0x22/0x70 [btrfs]
> [ 1520.091135] btrfs_xattr_handler_set_prop+0x6c/0x1f0 [btrfs]
> [ 1520.098452] __vfs_setxattr+0xd8/0x130
> [ 1520.103821] ? xattr_resolve_name+0x3e0/0x3e0
> [ 1520.109812] __vfs_setxattr_noperm+0xeb/0x3b0
> [ 1520.115790] vfs_setxattr+0xa3/0xd0
> [ 1520.120898] setxattr+0x17a/0x2c0
> [ 1520.125824] ? vfs_setxattr+0xd0/0xd0
> [ 1520.131102] ? __pmd_alloc+0x560/0x560
> [ 1520.136452] ? __do_sys_newfstat+0xd3/0xe0
> [ 1520.142123] ? __ia32_sys_newfstatat+0xf0/0xf0
> [ 1520.148140] ? __kasan_slab_free+0x141/0x170
> [ 1520.153955] ? handle_mm_fault+0x1ae/0x640
> [ 1520.159564] __x64_sys_fsetxattr+0x1a0/0x220
> [ 1520.165347] do_syscall_64+0xa0/0x2e0
> [ 1520.170515] ? page_fault+0x8/0x30
> [ 1520.175408] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1520.181975] RIP: 0033:0x7f84f257ae6e
> [ 1520.187068] Code: 48 8b 0d 1d 70 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 be 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ea 6f 0c 00 f7 d8 64 89 01 48
> [ 1520.209083] RSP: 002b:00007fff87996fa8 EFLAGS: 00000246 ORIG_RAX: 00000000000000be
> [ 1520.218336] RAX: ffffffffffffffda RBX: 000000000000000b RCX: 00007f84f257ae6e
> [ 1520.227132] RDX: 00007fff8799798b RSI: 00005561caf83270 RDI: 0000000000000003
> [ 1520.235912] RBP: 00007fff8799798b R08: 0000000000000000 R09: 00005561caf83290
> [ 1520.244691] R10: 0000000000000002 R11: 0000000000000246 R12: 00005561caf83270
> [ 1520.253462] R13: 0000000000000003 R14: 00007fff87997972 R15: 00007fff8799797f
>
> [ 1520.265443] Allocated by task 15317:
> [ 1520.270697] __kasan_kmalloc.constprop.0+0xc2/0xd0
> [ 1520.277677] setxattr+0xe8/0x2c0
> [ 1520.283084] __x64_sys_fsetxattr+0x1a0/0x220
> [ 1520.289540] do_syscall_64+0xa0/0x2e0
> [ 1520.295379] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 1520.306288] Freed by task 12227:
> [ 1520.311632] __kasan_slab_free+0x12c/0x170
> [ 1520.317828] kfree+0x90/0x1e0
> [ 1520.322891] btrfs_free_block_groups+0x8a1/0xd80 [btrfs]
> [ 1520.330313] close_ctree+0x37e/0x650 [btrfs]
> [ 1520.336627] generic_shutdown_super+0x12e/0x320
> [ 1520.343177] kill_anon_super+0x36/0x60
> [ 1520.348983] btrfs_kill_super+0x3d/0x2c0 [btrfs]
> [ 1520.355636] deactivate_locked_super+0x85/0xd0
> [ 1520.362108] deactivate_super+0x122/0x140
> [ 1520.368166] cleanup_mnt+0x9f/0x130
> [ 1520.373699] task_work_run+0x131/0x1c0
> [ 1520.379490] exit_to_usermode_loop+0x133/0x160
> [ 1520.386002] do_syscall_64+0x259/0x2e0
> [ 1520.391796] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> [ 1520.402415] The buggy address belongs to the object at ffff8886daec16c0
> which belongs to the cache kmalloc-8 of size 8
> [ 1520.418769] The buggy address is located 2 bytes inside of
> 8-byte region [ffff8886daec16c0, ffff8886daec16c8)
> [ 1520.434407] The buggy address belongs to the page:
> [ 1520.441358] page:ffffea001b6bb040 count:1 mapcount:0 mapping:ffff8886ff40fb80 index:0x0
> [ 1520.451601] flags: 0x17ffe000000200(slab)
> [ 1520.457868] raw: 0017ffe000000200 ffffea001bedcec0 0000000700000007 ffff8886ff40fb80
> [ 1520.467940] raw: 0000000000000000 0000000080aa00aa 00000001ffffffff 0000000000000000
> [ 1520.478024] page dumped because: kasan: bad access detected
>
> [ 1520.489795] Memory state around the buggy address:
> [ 1520.496963] ffff8886daec1580: fc 00 fc fc 00 fc fc 00 fc fc 00 fc fc 00 fc fc
> [ 1520.506621] ffff8886daec1600: 00 fc fc 04 fc fc fb fc fc fb fc fc fb fc fc fb
> [ 1520.516277] >ffff8886daec1680: fc fc 04 fc fc 00 fc fc 02 fc fc fb fc fc 00 fc
> [ 1520.525915] ^
> [ 1520.533584] ffff8886daec1700: fc 00 fc fc 00 fc fc 00 fc fc 00 fc fc 04 fc fc
> [ 1520.543137] ffff8886daec1780: fb fc fc 00 fc fc 00 fc fc 00 fc fc 00 fc fc 00
> [ 1520.552642] ==================================================================
>
> Fixes: 272e5326c783 ("btrfs: prop: fix vanished compression property after failed set")
> Fixes: 50398fde997f ("btrfs: prop: fix zstd compression parameter validation")
> Cc: stable@xxxxxxxxxxxxxxx # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion
> Cc: stable@xxxxxxxxxxxxxxx # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c
> Cc: stable@xxxxxxxxxxxxxxx # 4.14+
> Signed-off-by: Naohiro Aota <naohiro.aota@xxxxxxx>
We caught that one yesterday and were testing various fixes for it
Johannes just sent his version which IMO makes the code a bit more
maintainable.
> ---
> fs/btrfs/props.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index a9e2e66152ee..428141bf545d 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -257,11 +257,11 @@ static int prop_compression_validate(const char *value, size_t len)
> if (!value)
> return 0;
>
> - if (!strncmp("lzo", value, 3))
> + if (len >= 3 && !strncmp("lzo", value, 3))
> return 0;
> - else if (!strncmp("zlib", value, 4))
> + else if (len >= 4 && !strncmp("zlib", value, 4))
> return 0;
> - else if (!strncmp("zstd", value, 4))
> + else if (len >= 4 && !strncmp("zstd", value, 4))
> return 0;
>
> return -EINVAL;
> @@ -281,12 +281,12 @@ static int prop_compression_apply(struct inode *inode, const char *value,
> return 0;
> }
>
> - if (!strncmp("lzo", value, 3)) {
> + if (len >= 3 && !strncmp("lzo", value, 3)) {
> type = BTRFS_COMPRESS_LZO;
> btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
> - } else if (!strncmp("zlib", value, 4)) {
> + } else if (len >= 4 && !strncmp("zlib", value, 4)) {
> type = BTRFS_COMPRESS_ZLIB;
> - } else if (!strncmp("zstd", value, 4)) {
> + } else if (len >= 4 && !strncmp("zstd", value, 4)) {
> type = BTRFS_COMPRESS_ZSTD;
> btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
> } else {
This seems redundant as ->validates is supposed to always be called
before calling ->apply.
>
