Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double-free of encryption wrapping key due to invalid pool properties #8791

Merged
merged 1 commit into from
May 28, 2019

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented May 21, 2019

Motivation and Context

This commits fixes a double-free in zfs_ioc_pool_create() triggered by specifying an unsupported combination of properties when creating a pool with encryption enabled.

root@linux:/usr/src/zfs# truncate -s 1024m /var/tmp/1
root@linux:/usr/src/zfs# dd if=/dev/urandom of=/var/tmp/key bs=32 count=1
1+0 records in
1+0 records out
32 bytes (32 B) copied, 0.0017121 s, 18.7 kB/s
root@linux:/usr/src/zfs# zpool create -f -O encryption=on -O compression=lz4 -O keyformat=raw -O keylocation=file:///var/tmp/key -o feature@encryption=enabled -d rpool /var/tmp/1
[ 2493.031066] general protection fault: 0000 [#1] SMP 
[ 2493.031914] Modules linked in: zfs(PO) zunicode(PO) zlua(PO) zcommon(PO) znvpair(PO) icp(PO) zavl(PO) spl(O) joydev crc32_pclmul ppdev evdev aesni_intel aes_x86_64 lrw gf128mul glue_helper psmouse serio_raw ablk_helper cryptd pcspkr virtio_balloon i2c_piix4 i2c_core parport_pc parport pvpanic virtio_console processor thermal_sys button autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid hid sg sr_mod cdrom ata_generic virtio_scsi virtio_blk virtio_net crct10dif_pclmul crct10dif_common crc32c_intel ahci libahci ata_piix floppy xhci_hcd usbcore usb_common virtio_pci virtio_ring virtio libata scsi_mod
[ 2493.034969] CPU: 0 PID: 17613 Comm: zpool Tainted: P           O  3.16.0-4-amd64 #1 Debian 3.16.51-2
[ 2493.034969] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 2493.034969] task: ffff880039bcab30 ti: ffff880038218000 task.ti: ffff880038218000
[ 2493.034969] RIP: 0010:[<ffffffffa056cb18>]  [<ffffffffa056cb18>] list_del+0x11/0x51 [zfs]
[ 2493.034969] RSP: 0018:ffff88003821bcf0  EFLAGS: 00010286
[ 2493.034969] RAX: dead000000000100 RBX: ffff880037b89400 RCX: ffff8800b7865180
[ 2493.034969] RDX: 0000000000000000 RSI: dead000000000100 RDI: dead000000000100
[ 2493.034969] RBP: ffff88003821bd00 R08: ffff880039a82600 R09: ffff880037865180
[ 2493.034969] R10: ffff880039a82800 R11: ffff880039a82c00 R12: 00007ffd490e5f90
[ 2493.034969] R13: 0000000000005a00 R14: 00007ffd490e5f90 R15: 0000000000000000
[ 2493.034969] FS:  00007f58be0b1780(0000) GS:ffff88003e400000(0000) knlGS:0000000000000000
[ 2493.034969] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 2493.034969] CR2: 00007f58be0bf000 CR3: 0000000039335000 CR4: 00000000000006f0
[ 2493.034969] Stack:
[ 2493.034969]  dead000000000100 ffff880037b89400 ffff88003821bd20 ffffffffa056ce7b
[ 2493.034969]  dead000000000100 ffff880037998b88 ffff88003821bd78 ffffffffa056d1bd
[ 2493.034969]  0000000000000000 ffff880037998b48 00007ffd490e5f90 0000000000005a00
[ 2493.034969] Call Trace:
[ 2493.034969]  [<ffffffffa056ce7b>] ? list_remove+0x27/0x29 [zfs]
[ 2493.034969]  [<ffffffffa056d1bd>] ? zfs_refcount_destroy_many+0x79/0x20d [zfs]
[ 2493.034969]  [<ffffffffa0309876>] ? spl_kmem_free_impl+0x3a/0x3c [spl]
[ 2493.034969]  [<ffffffffa056d373>] ? zfs_refcount_destroy+0x22/0x24 [zfs]
[ 2493.034969]  [<ffffffffa05267f9>] ? dsl_wrapping_key_free+0xf2/0x105 [zfs]
[ 2493.034969]  [<ffffffffa0526d96>] ? dsl_crypto_params_free+0x5d/0x70 [zfs]
[ 2493.034969]  [<ffffffffa0636785>] ? zfs_ioc_pool_create+0x394/0x399 [zfs]
[ 2493.034969]  [<ffffffffa0643660>] ? zfsdev_ioctl+0x69b/0x799 [zfs]
[ 2493.034969]  [<ffffffff811c1baf>] ? do_vfs_ioctl+0x2cf/0x4b0
[ 2493.034969]  [<ffffffff811760a9>] ? do_munmap+0x299/0x3a0
[ 2493.034969]  [<ffffffff811c1e11>] ? SyS_ioctl+0x81/0xa0
[ 2493.034969]  [<ffffffff81525d0d>] ? system_call_fast_compare_end+0x10/0x15
[ 2493.034969] Code: f0 48 8b 45 f0 48 8b 55 f8 48 89 50 08 48 8b 45 f8 48 8b 55 f0 48 89 10 c9 c3 55 48 89 e5 53 48 83 ec 08 48 89 7d f0 48 8b 45 f0 <48> 8b 10 48 8b 45 f0 48 8b 40 08 48 89 d6 48 89 c7 e8 b0 ff ff 
[ 2493.034969] RIP  [<ffffffffa056cb18>] list_del+0x11/0x51 [zfs]
[ 2493.034969]  RSP <ffff88003821bcf0>
[ 2493.107718] ---[ end trace 67e0fd68c393fe39 ]---
root@linux:/usr/src/zfs# 

Description

How Has This Been Tested?

Tested on a Debian with updated test case

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:

This commits fixes a double-free in zfs_ioc_pool_create() triggered by
specifying an unsupported combination of properties when creating a pool
with encryption enabled.

Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
@loli10K loli10K added the Status: Code Review Needed Ready for review and testing label May 21, 2019
@codecov
Copy link

codecov bot commented May 22, 2019

Codecov Report

Merging #8791 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8791      +/-   ##
==========================================
+ Coverage   78.63%   78.79%   +0.15%     
==========================================
  Files         381      381              
  Lines      117798   117791       -7     
==========================================
+ Hits        92636    92812     +176     
+ Misses      25162    24979     -183
Flag Coverage Δ
#kernel 79.39% <100%> (+0.05%) ⬆️
#user 67.28% <ø> (+0.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dc41a7...0e7d22b. Read the comment docs.

@behlendorf behlendorf requested a review from tcaputi May 24, 2019 20:31
Copy link
Contributor

@tcaputi tcaputi left a comment

Choose a reason for hiding this comment

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

Fixes the issue. See my one comment.

(void) spa_destroy(spa_name);
unload_wkey = B_FALSE; /* spa_destroy() unloads wrapping keys */
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would move this up just beneath spa_create(). The issue here is that after spa_create() runs (without error), it "owns" the dsl crypto key so we should set this variable as soon as that is true (to prevent future problems). This code is equivalent though.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 25, 2019
@behlendorf behlendorf merged commit ab1a970 into openzfs:master May 28, 2019
behlendorf pushed a commit that referenced this pull request Jun 7, 2019
This commits fixes a double-free in zfs_ioc_pool_create() triggered by
specifying an unsupported combination of properties when creating a pool
with encryption enabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tom Caputi <tcaputi@datto.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes #8791
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 7, 2019
This commits fixes a double-free in zfs_ioc_pool_create() triggered by
specifying an unsupported combination of properties when creating a pool
with encryption enabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tom Caputi <tcaputi@datto.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#8791
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 15, 2019
This commits fixes a double-free in zfs_ioc_pool_create() triggered by
specifying an unsupported combination of properties when creating a pool
with encryption enabled.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tom Caputi <tcaputi@datto.com>
Signed-off-by: loli10K <ezomori.nozomu@gmail.com>
Closes openzfs#8791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants