-----Original Message----- From: Ernesto A. Fernández [mailto:ernesto.mnd.fernandez@xxxxxxxxx] Sent: Monday, October 9, 2017 1:00 PM To: Viacheslav Dubeyko <slava@xxxxxxxxxxx>; linux-fsdevel@xxxxxxxxxxxxxxx Cc: Sergei Antonov <saproj@xxxxxxxxx>; Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>; Al Viro <viro@xxxxxxxxxxxxxxxxxx>; Christoph Hellwig <hch@xxxxxxxxxxxxx>; Slava Dubeyko <Vyacheslav.Dubeyko@xxxxxxx>; Ernesto A. Fernández <ernesto.mnd.fernandez@xxxxxxxxx> Subject: Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file > Here's the callstack, I hope it helps you: > > [ 3550.503259] general protection fault: 0000 [#1] SMP > [ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy > [ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16 > [ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014 > [ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000 > [ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] > [ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202 > [ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff > [ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780 > [ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000 > [ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002 > [ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56 > [ 3550.504013] FS: 00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 > [ 3550.504013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0 > [ 3550.504013] Call Trace: > [ 3550.504013] hfsplus_bnode_write_u16+0x27/0x30 [hfsplus] > [ 3550.504013] hfsplus_brec_remove+0x117/0x170 [hfsplus] > [ 3550.504013] __hfsplus_delete_attr+0x94/0xf0 [hfsplus] > [ 3550.504013] hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus] > [ 3550.504013] hfsplus_delete_cat+0x1f5/0x300 [hfsplus] > [ 3550.504013] hfsplus_unlink+0x82/0x1e0 [hfsplus] > [ 3550.504013] ? __inode_permission+0x44/0xc0 > [ 3550.504013] vfs_unlink+0xf1/0x180 > [ 3550.504013] do_unlinkat+0x25f/0x2e0 > [ 3550.504013] SyS_unlinkat+0x1b/0x30 > [ 3550.504013] entry_SYSCALL_64_fastpath+0x1e/0xa9 > [ 3550.504013] RIP: 0033:0x7fc27ebe632d > [ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107 > [ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d > [ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c > [ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000 > [ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030 > [ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000 > [ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48 > [ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08 > [ 3550.540759] ---[ end trace 142de398139577f1 ]--- > The whole picture is here: (1) hfsplus_find_attr() [ 458.039682] hfsplus: find_attr: (null),18 (2) hfs_brec_find(fd, hfs_find_1st_rec_by_cnid) [ 458.039683] hfsplus: put_node(8:1): 1 [ 458.039685] hfsplus: get_node(8:3): 82 [ 458.039686] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4 [ 458.039687] hfsplus: put_node(8:3): 82 [ 458.039688] hfsplus: get_node(8:1): 1 [ 458.039690] hfsplus: __hfs_brec_find: fd->record 0, fd->keyoffset 14, fd->keylength 30, fd->entryoffset 44, fd->entrylength 16 (3) __hfsplus_delete_attr() (4) hfs_brec_remove() [ 458.039691] hfsplus: bnode: 1 [ 458.039692] hfsplus: 2, 0, -1, 1, 1 [ 458.039693] hfsplus: 14 (28) [ 458.039695] hfsplus: 60 [ 458.039697] hfsplus: remove_rec: 0, 46 [ 458.039697] hfsplus: remove_rec: num_recs 1 [ 458.039700] hfsplus: new_node(8:2): 1 [ 458.040614] hfsplus: put_node(8:2): 1 [ 458.040618] hfsplus: get_node(8:3): 82 [ 458.040619] hfsplus: put_node(8:1): 1 [ 458.040620] hfsplus: remove_node(8:1): 0 [ 458.040622] hfsplus: btree_free_node: 1 [ 458.040623] hfsplus: new_node(8:0): 1 [ 458.041096] hfsplus: put_node(8:0): 1 [ 458.041102] hfsplus: __hfs_brec_find: fd->record -1, fd->keyoffset 14, fd->keylength 26, fd->entryoffset 40, fd->entrylength 4 [ 458.041103] hfsplus: bnode: 3 [ 458.041104] hfsplus: 0, 0, 0, 2, 2 [ 458.041105] hfsplus: 14 (26,1) [ 458.041107] hfsplus: 44 (30,2) [ 458.041110] hfsplus: 78 [ 458.041112] hfsplus: remove_rec: -1, 30 [ 458.041113] hfsplus: remove_rec: num_recs 2 [ 458.041114] hfsplus: remove_rec: num_recs 1 [ 458.041117] hfsplus: remove_rec: rec_off 8190, end_off 8186 [ 458.041118] hfsplus: remove_rec: rec_off 8190, end_off 8186 [ 458.041119] hfsplus: remove_rec: data_off 14, size 30 [ 458.041143] general protection fault: 0000 [#1] SMP The hfs_brec_find() found the pointer (#1) in index node #3 by means of hfs_find_1st_rec_by_cnid strategy. Then the hfs_brec_find() found the single record in the leaf node #1 on the basis of the same strategy. The most interesting is starting in hfs_brec_remove(): if (!--node->num_recs) { hfs_bnode_unlink(node); if (!node->parent) return 0; parent = hfs_bnode_find(tree, node->parent); if (IS_ERR(parent)) return PTR_ERR(parent); hfs_bnode_put(node); node = fd->bnode = parent; __hfs_brec_find(node, fd, hfs_find_rec_by_key); goto again; } The hfs_brec_remove() is trying to delete the single and latest record from the leaf node #1. Finally, the node->num_recs will be equal to zero after the increment operation and the condition directs the code inside of this execution path. So, first of all, the logic unlink the leaf node #2 from leaf node #1. Then the logic gets the parent node and to put the leaf node #1. And, finally, we are trying to find a record in the parent and index node #3 on the basis of hfs_find_rec_by_key strategy. We cannot find anything because the fd->search_key is not prepared for such search. It means that fd->record is equal to -1. The real issue takes place here: hfs_bnode_write_u16(node, rec_off + 2, data_off - size); because the data_off == 14 and size == 30. We have overflow here and it is the reason of the crash. But the data_off is lesser of size because of value of fd->record we selected improper position of the offset for the changing the value in the offsets table. What should be done for the issue fix? (1) We need to add the check condition here: http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L217 do { data_off = hfs_bnode_read_u16(node, rec_off); /* * TODO: check condition here * if (data_off < size) * return error here!!!!! */ hfs_bnode_write_u16(node, rec_off + 2, data_off - size); rec_off -= 2; } while (rec_off >= end_off); (2) The logic of hfs_brec_remove() should be reworked here: http://elixir.free-electrons.com/linux/latest/source/fs/hfsplus/brec.c#L197 /* * TODO: rework the logic here if (node->num_recs <= 0) BUG(); /* it could be error message here */ else if (node->num_recs == 1) __hfs_brec_find(node, fd, hfs_find_1st_rec_by_cnid); /* maybe check the error here */ memcpy(fd->search_key, fd->key, sizeof(hfsplus_btree_key)); --node->num_recs; */ hfs_bnode_unlink(node); if (!node->parent) return 0; parent = hfs_bnode_find(tree, node->parent); if (IS_ERR(parent)) return PTR_ERR(parent); hfs_bnode_put(node); node = fd->bnode = parent; __hfs_brec_find(node, fd, hfs_find_rec_by_key); goto again; } So, we need to extract the key of the latest record from the leaf node by using the hfs_find_1st_rec_by_cnid strategy, copy fd->key into the fd->search_key and then to go into logic of searching the record in the index node on the basis of hfs_find_rec_by_key strategy and the deletion of the record from the index node. What do you think? Could you rework the patch? Thanks, Vyacheslav Dubeyko.
