* [PATCH] btrfs: fix out-of-bounds access in property handling
@ 2019-06-06 7:49 Naohiro Aota
2019-06-06 8:04 ` Nikolay Borisov
0 siblings, 1 reply; 3+ messages in thread
From: Naohiro Aota @ 2019-06-06 7:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba, Naohiro Aota, stable
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@vger.kernel.org # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion
Cc: stable@vger.kernel.org # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c
Cc: stable@vger.kernel.org # 4.14+
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
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 {
--
2.21.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: fix out-of-bounds access in property handling
2019-06-06 7:49 [PATCH] btrfs: fix out-of-bounds access in property handling Naohiro Aota
@ 2019-06-06 8:04 ` Nikolay Borisov
2019-06-06 8:37 ` Naohiro Aota
0 siblings, 1 reply; 3+ messages in thread
From: Nikolay Borisov @ 2019-06-06 8:04 UTC (permalink / raw)
To: Naohiro Aota, linux-btrfs; +Cc: David Sterba, stable
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@vger.kernel.org # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion
> Cc: stable@vger.kernel.org # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c
> Cc: stable@vger.kernel.org # 4.14+
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
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.
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: fix out-of-bounds access in property handling
2019-06-06 8:04 ` Nikolay Borisov
@ 2019-06-06 8:37 ` Naohiro Aota
0 siblings, 0 replies; 3+ messages in thread
From: Naohiro Aota @ 2019-06-06 8:37 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs; +Cc: David Sterba, stable
On 2019/06/06 17:05, Nikolay Borisov wrote:
>
>
> 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.
>>
>>(snip)
>>
>> Fixes: 272e5326c783 ("btrfs: prop: fix vanished compression property after failed set")
>> Fixes: 50398fde997f ("btrfs: prop: fix zstd compression parameter validation")
>> Cc: stable@vger.kernel.org # 4.14+: 802a5c69584a: btrfs: prop: use common helper for type to string conversion
>> Cc: stable@vger.kernel.org # 4.14+: 3dcf96c7b9fe: btrfs: drop redundant forward declaration in props.c
>> Cc: stable@vger.kernel.org # 4.14+
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>
> 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.
>
yeah, that looks good to me. It's much more easy to add new compression
type. Please pick that one.
>
>> ---
>> 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.
>
Indeed.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-06 8:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-06 7:49 [PATCH] btrfs: fix out-of-bounds access in property handling Naohiro Aota
2019-06-06 8:04 ` Nikolay Borisov
2019-06-06 8:37 ` Naohiro Aota
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).