Skip to content

Fix buffer overflow in dsl_dataset_name()#6374

Merged
behlendorf merged 1 commit intoopenzfs:masterfrom
loli10K:fix-poolname-buffer-overflow
Jul 24, 2017
Merged

Fix buffer overflow in dsl_dataset_name()#6374
behlendorf merged 1 commit intoopenzfs:masterfrom
loli10K:fix-poolname-buffer-overflow

Conversation

@loli10K
Copy link
Contributor

@loli10K loli10K commented Jul 19, 2017 •
edited
Loading

Description

If we're creating a pool with version >= SPA_VERSION_DSL_SCRUB (v11) we need to account for additional space needed by the origin dataset which will also be snapshotted: "poolname"+"/"+"$ORIGIN"+"@"+"$ORIGIN".

Enforce this limit in pool_namecheck().

EDIT: related to eca7b76

Motivation and Context

ZoL:

root@linux:~# function is_linux() {
>    if [[ "$(uname)" == "Linux" ]]; then
>       return 0
>    else
>       return 1
>    fi
> }
root@linux:~# #
root@linux:~# POOLNAME="$(printf 'A%.s' `seq 1 240`)"
root@linux:~# if is_linux; then
>    TMPDIR='/var/tmp'
>    mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
>    zpool destroy $POOLNAME
>    rm -f $TMPDIR/zpool.dat
>    fallocate -l 64m $TMPDIR/zpool.dat
>    zpool create -O mountpoint=none $POOLNAME $TMPDIR/zpool.dat
> else
>    TMPDIR='/tmp'
>    zpool destroy $POOLNAME
>    rm -f $TMPDIR/zpool.dat
>    mkfile 64m $TMPDIR/zpool.dat
>    zpool create -O mountpoint=none $POOLNAME $TMPDIR/zpool.dat
> fi
cannot open 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA': no such pool
[  148.800464] VERIFY3(strlcat(name, ds->ds_snapname, 256) < 256) failed (262 < 256)
[  148.803733] PANIC at dsl_dataset.c:687:dsl_dataset_name()
[  148.805649] Showing stack for process 2253
[  148.806111] CPU: 2 PID: 2253 Comm: zpool Tainted: P           OE   4.6.4 #2
[  148.807946] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  148.809657]  0000000000000286 00000000c773538c ffff8800db31f838 ffffffff81495313
[  148.811855]  ffffffffc03ab320 00000000000002af ffff8800db31f848 ffffffffc0103d04
[  148.813821]  ffff8800db31f9d0 ffffffffc0103dcf 0000000002404200 ffff880200000030
[  148.815723] Call Trace:
[  148.816020]  [<ffffffff81495313>] dump_stack+0x63/0x90
[  148.817681]  [<ffffffffc0103d04>] spl_dumpstack+0x44/0x50 [spl]
[  148.818333]  [<ffffffffc0103dcf>] spl_panic+0xbf/0xf0 [spl]
[  148.820152]  [<ffffffff811e5f5d>] ? kmem_cache_alloc_node_trace+0xfd/0x4f0
[  148.821998]  [<ffffffffc01f1661>] ? arc_cksum_compute+0xa1/0x190 [zfs]
[  148.823785]  [<ffffffff811e6541>] ? __kmalloc_node+0x31/0x40
[  148.824359]  [<ffffffff811e6541>] ? __kmalloc_node+0x31/0x40
[  148.826011]  [<ffffffff8149e43a>] ? strlcat+0x5a/0x80
[  148.827569]  [<ffffffff8149e43a>] ? strlcat+0x5a/0x80
[  148.828162]  [<ffffffffc0239bcf>] dsl_dataset_name+0x26f/0x360 [zfs]
[  148.829923]  [<ffffffffc012081c>] ? fnvlist_alloc+0x2c/0x90 [znvpair]
[  148.831675]  [<ffffffffc027f0e6>] spa_history_log_internal_ds+0x66/0x130 [zfs]
[  148.832449]  [<ffffffff81832622>] ? mutex_lock+0x12/0x30
[  148.834104]  [<ffffffffc023ebe7>] dsl_dataset_snapshot_sync_impl+0x6c7/0x9b0 [zfs]
[  148.835967]  [<ffffffffc024c891>] dsl_pool_create_origin+0xb1/0x220 [zfs]
[  148.837751]  [<ffffffffc024ccee>] dsl_pool_create+0x2ee/0x4f0 [zfs]
[  148.838392]  [<ffffffffc011dba9>] ? nvlist_lookup_common+0x59/0x90 [znvpair]
[  148.840293]  [<ffffffffc0277d32>] spa_create+0x512/0xaf0 [zfs]
[  148.841908]  [<ffffffffc02c55d2>] zfs_ioc_pool_create+0x1f2/0x260 [zfs]
[  148.843607]  [<ffffffffc02c63b4>] zfsdev_ioctl+0x574/0x670 [zfs]
[  148.844215]  [<ffffffff8182a126>] ? kmemleak_free+0x36/0x80
[  148.845825]  [<ffffffff8121b44d>] do_vfs_ioctl+0x9d/0x5b0
[  148.846448]  [<ffffffff81068e50>] ? __do_page_fault+0x210/0x4d0
[  148.848224]  [<ffffffff8121b9d9>] SyS_ioctl+0x79/0x90
[  148.849771]  [<ffffffff81834536>] system_call_fast_compare_end+0xc/0x96

Illumos:

> ::status
debugging crash dump vmcore.1 (64-bit) from 52-54-00-d3-7a-01
operating system: 5.11 joyent_20170622T212149Z (i86pc)
image uuid: (not set)
panic message: assertion failed: strlcat(name, ds->ds_snapname, 256) < 256 (0x100 < 0x100), file: ../../common/fs/zfs/dsl_dataset.c, line: 670
dump content: kernel pages only
> ::stack
vpanic()
0xfffffffffba7bcad()
dsl_dataset_name+0xe3(ffffff0255767080, ffffff00088ff7b0)
spa_history_log_internal_ds+0x74(ffffff0255767080, fffffffff7e055a5, ffffff024ca04c00, fffffffff7e04fd7)
dsl_dataset_snapshot_sync_impl+0x455(ffffff025078f000, fffffffff7e05598, ffffff024ca04c00)
dsl_pool_create_origin+0x6d(ffffff0252e8ab00, ffffff024ca04c00)
dsl_pool_create+0x243(ffffff025561d000, ffffff024f3aa8e0, 4)
spa_create+0x391(ffffff024f1a5000, ffffff02508645c0, ffffff024e82b538, ffffff024f3aa8e0)
zfs_ioc_pool_create+0x181(ffffff024f1a5000)
zfsdev_ioctl+0x4b7(5a00000000, 5a00, 8042578, 100003, ffffff02557c3578, ffffff00088ffe58)
cdev_ioctl+0x39(5a00000000, 5a00, 8042578, 100003, ffffff02557c3578, ffffff00088ffe58)
spec_ioctl+0x60(ffffff024e8f1600, 5a00, 8042578, 100003, ffffff02557c3578, ffffff00088ffe58)
fop_ioctl+0x55(ffffff024e8f1600, 5a00, 8042578, 100003, ffffff02557c3578, ffffff00088ffe58)
ioctl+0x9b(3, 5a00, 8042578)
_sys_sysenter_post_swapgs+0x153()
> ffffff0255767080::print dsl_dataset_t ds_snapname ds_is_snapshot
ds_snapname = [ "$ORIGIN" ]
ds_is_snapshot = 0x1 (B_TRUE)
> ffffff00088ff7b0,0x10f::dump
                   \/ 1 2 3  4 5 6 7  8 9 a b  c d e f  v123456789abcdef
ffffff00088ff7b0:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff7c0:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff7d0:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff7e0:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff7f0:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff800:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff810:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff820:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff830:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff840:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff850:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff860:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff870:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff880:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff890:  41414141 41414141 41414141 41414141  AAAAAAAAAAAAAAAA
ffffff00088ff8a0:  2f244f52 4947494e 40244f52 49474900  /$ORIGIN@$ORIGI.
ffffff00088ff8b0:  40931c5c 02ffffff 933bd65a 361136c0  @..\.....;.Z6.6.
>

How Has This Been Tested?

Manually tested (gdb)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@loli10K loli10K requested a review from ahrens July 20, 2017 04:28
ahrens
ahrens reviewed Jul 20, 2017
View reviewed changes
module/zcommon/zfs_namecheck.c
*/
if (strlen(pool) >= ZFS_MAX_DATASET_NAME_LEN) {
if (strlen(pool) >= (ZFS_MAX_DATASET_NAME_LEN + 2 +
strlen(ORIGIN_DIR_NAME) * 2)) {
Copy link
Member

@ahrens ahrens Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the comment but not the code. Did you mean to do - instead of +?

Copy link
Contributor Author

@loli10K loli10K Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, i think i was already half asleep when i pushed it, will fix.

@loli10K
Fix buffer overflow in dsl_dataset_name()
52983a4 
If we're creating a pool with version >= SPA_VERSION_DSL_SCRUB (v11)
we need to account for additional space needed by the origin dataset
which will also be snapshotted: "poolname"+"/"+"$ORIGIN"+"@"+"$ORIGIN".

Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
@loli10K loli10K force-pushed the fix-poolname-buffer-overflow branch from 9f42459 to 52983a4 Compare July 20, 2017 17:16
@behlendorf behlendorf merged commit 871e073 into openzfs:master Jul 24, 2017
@loli10K loli10K deleted the fix-poolname-buffer-overflow branch July 25, 2017 14:34
@loli10K loli10K mentioned this pull request Oct 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahrens ahrens ahrens left review comments

@behlendorf behlendorf behlendorf approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@loli10K @behlendorf @ahrens