| Message ID | 20200204070636.25572-1-zlang@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: fix invalid pointer dereference in xfs_attr3_node_inactive | expand |
On Tuesday, February 4, 2020 12:36 PM Zorro Lang wrote: > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > gets 'child_bp' at there: > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > child_blkno, > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > &child_bp); > if (error) > return error; > error = bp->b_error; > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > xfs_trans_brelse(*trans, bp); May be add a Fixes tag. The bug was introduced by the commit 2911edb653b9c64e0aad461f308cae8ce030eb7b (xfs: remove the mappedbno argument to xfs_da_get_buf). Apart from that, I don't see any other issue with your patch. Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > [75626.212549] ================================================================== > [75626.245606] BUG: KASAN: use-after-free in xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > [75626.280164] Read of size 4 at addr ffff88881ffab004 by task rm/30390 > > [75626.315595] CPU: 13 PID: 30390 Comm: rm Tainted: G W 5.5.0+ #1 > [75626.347856] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 08/02/2014 > [75626.377864] Call Trace: > [75626.388868] dump_stack+0x96/0xe0 > [75626.403778] print_address_description.constprop.4+0x1f/0x300 > [75626.429656] __kasan_report.cold.8+0x76/0xb0 > [75626.448950] ? xfs_trans_ordered_buf+0x410/0x440 [xfs] > [75626.472393] ? xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > [75626.496705] kasan_report+0xe/0x20 > [75626.512134] xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > [75626.535328] ? xfs_da_read_buf+0x235/0x2c0 [xfs] > [75626.557270] ? xfs_attr3_leaf_inactive+0x470/0x470 [xfs] > [75626.583199] ? xfs_da3_root_split+0x1050/0x1050 [xfs] > [75626.607952] ? lock_contended+0xd20/0xd20 > [75626.626615] ? xfs_ilock+0x149/0x4c0 [xfs] > [75626.644661] ? down_write_nested+0x187/0x3c0 > [75626.663892] ? down_write_trylock+0x2f0/0x2f0 > [75626.683496] ? __sb_start_write+0x1c4/0x310 > [75626.702389] ? down_read_trylock+0x360/0x360 > [75626.721669] ? xfs_trans_buf_set_type+0x90/0x1e0 [xfs] > [75626.745171] xfs_attr_inactive+0x3e5/0x7b0 [xfs] > [75626.766097] ? xfs_attr3_node_inactive+0x8a0/0x8a0 [xfs] > [75626.790101] ? lock_downgrade+0x6d0/0x6d0 > [75626.808122] ? do_raw_spin_trylock+0xb2/0x180 > [75626.827859] ? lock_contended+0xd20/0xd20 > [75626.846154] xfs_inactive+0x4b8/0x5b0 [xfs] > [75626.865504] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > [75626.887615] destroy_inode+0xbc/0x1a0 > [75626.904172] do_unlinkat+0x451/0x5d0 > [75626.920325] ? __ia32_sys_rmdir+0x40/0x40 > [75626.938485] ? __check_object_size+0x275/0x324 > [75626.958819] ? strncpy_from_user+0x7d/0x350 > [75626.977848] do_syscall_64+0x9f/0x4f0 > [75626.994333] entry_SYSCALL_64_after_hwframe+0x49/0xbe > [75627.017173] RIP: 0033:0x7f968239567b > [75627.033260] Code: 73 01 c3 48 8b 0d 0d d8 2c 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 b8 07 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d dd d7 2c 00 f7 d8 64 89 01 48 > [75627.123796] RSP: 002b:00007ffcdf66ad38 EFLAGS: 00000246 ORIG_RAX: 0000000000000107 > [75627.158521] RAX: ffffffffffffffda RBX: 0000562cd8b5d5b0 RCX: 00007f968239567b > [75627.190764] RDX: 0000000000000000 RSI: 0000562cd8b5c380 RDI: 00000000ffffff9c > [75627.222921] RBP: 0000562cd8b5c2f0 R08: 0000000000000003 R09: 0000000000000000 > [75627.255236] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffcdf66af20 > [75627.287435] R13: 0000000000000000 R14: 0000562cd8b5d5b0 R15: 0000000000000000 > > [75627.326616] Allocated by task 30390: > [75627.342780] save_stack+0x19/0x80 > [75627.357980] __kasan_kmalloc.constprop.7+0xc1/0xd0 > [75627.379553] kmem_cache_alloc+0xc8/0x300 > [75627.397288] kmem_zone_alloc+0x10a/0x3f0 [xfs] > [75627.417376] _xfs_buf_alloc+0x56/0x1140 [xfs] > [75627.437051] xfs_buf_get_map+0x126/0x7c0 [xfs] > [75627.457103] xfs_buf_read_map+0xb2/0xaa0 [xfs] > [75627.477180] xfs_trans_read_buf_map+0x6c8/0x12d0 [xfs] > [75627.500420] xfs_da_read_buf+0x1d9/0x2c0 [xfs] > [75627.520579] xfs_da3_node_read+0x23/0x80 [xfs] > [75627.540620] xfs_attr_inactive+0x5c5/0x7b0 [xfs] > [75627.561609] xfs_inactive+0x4b8/0x5b0 [xfs] > [75627.581541] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > [75627.605628] destroy_inode+0xbc/0x1a0 > [75627.624025] do_unlinkat+0x451/0x5d0 > [75627.641629] do_syscall_64+0x9f/0x4f0 > [75627.658156] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [75627.687232] Freed by task 30390: > [75627.701882] save_stack+0x19/0x80 > [75627.716821] __kasan_slab_free+0x125/0x170 > [75627.735329] kmem_cache_free+0xcd/0x400 > [75627.752745] xfs_buf_rele+0x30a/0xcb0 [xfs] > [75627.772731] xfs_attr3_node_inactive+0x1c7/0x8a0 [xfs] > [75627.797384] xfs_attr_inactive+0x3e5/0x7b0 [xfs] > [75627.818450] xfs_inactive+0x4b8/0x5b0 [xfs] > [75627.837455] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > [75627.859765] destroy_inode+0xbc/0x1a0 > [75627.876296] do_unlinkat+0x451/0x5d0 > [75627.892466] do_syscall_64+0x9f/0x4f0 > [75627.909015] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [75627.938572] The buggy address belongs to the object at ffff88881ffaad80 > which belongs to the cache xfs_buf of size 680 > [75627.994075] The buggy address is located 644 bytes inside of > 680-byte region [ffff88881ffaad80, ffff88881ffab028) > [75628.047015] The buggy address belongs to the page: > [75628.069056] page:ffffea00207fea00 refcount:1 mapcount:0 mapping:ffff888098515400 index:0xffff88881ffa9d40 compound_mapcount: 0 > [75628.124539] raw: 0057ffffc0010200 dead000000000100 dead000000000122 ffff888098515400 > [75628.162598] raw: ffff88881ffa9d40 0000000080270025 00000001ffffffff 0000000000000000 > [75628.197491] page dumped because: kasan: bad access detected > > [75628.230389] Memory state around the buggy address: > [75628.252072] ffff88881ffaaf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [75628.284801] ffff88881ffaaf80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [75628.317587] >ffff88881ffab000: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > [75628.350592] ^ > [75628.364746] ffff88881ffab080: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb > [75628.397289] ffff88881ffab100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [75628.429955] ================================================================== > > Signed-off-by: Zorro Lang <zlang@redhat.com> > --- > fs/xfs/xfs_attr_inactive.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > index bbfa6ba84dcd..26230d150bf2 100644 > --- a/fs/xfs/xfs_attr_inactive.c > +++ b/fs/xfs/xfs_attr_inactive.c > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > &child_bp); > if (error) > return error; > - error = bp->b_error; > + error = child_bp->b_error; > if (error) { > xfs_trans_brelse(*trans, child_bp); > return error; >
On Tue, Feb 04, 2020 at 02:49:23PM +0530, Chandan Rajendra wrote: > On Tuesday, February 4, 2020 12:36 PM Zorro Lang wrote: > > > > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > gets 'child_bp' at there: > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > child_blkno, > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > &child_bp); > > if (error) > > return error; > > error = bp->b_error; > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > xfs_trans_brelse(*trans, bp); > > May be add a Fixes tag. The bug was introduced by the commit > 2911edb653b9c64e0aad461f308cae8ce030eb7b (xfs: remove the mappedbno argument > to xfs_da_get_buf). Sure, thanks :) > > Apart from that, I don't see any other issue with your patch. I'm doing xfstests regression test on it. I'll report if I find any regression issue. Thanks, Zorro > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > > > > > [75626.212549] ================================================================== > > [75626.245606] BUG: KASAN: use-after-free in xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > > [75626.280164] Read of size 4 at addr ffff88881ffab004 by task rm/30390 > > > > [75626.315595] CPU: 13 PID: 30390 Comm: rm Tainted: G W 5.5.0+ #1 > > [75626.347856] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 08/02/2014 > > [75626.377864] Call Trace: > > [75626.388868] dump_stack+0x96/0xe0 > > [75626.403778] print_address_description.constprop.4+0x1f/0x300 > > [75626.429656] __kasan_report.cold.8+0x76/0xb0 > > [75626.448950] ? xfs_trans_ordered_buf+0x410/0x440 [xfs] > > [75626.472393] ? xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > > [75626.496705] kasan_report+0xe/0x20 > > [75626.512134] xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] > > [75626.535328] ? xfs_da_read_buf+0x235/0x2c0 [xfs] > > [75626.557270] ? xfs_attr3_leaf_inactive+0x470/0x470 [xfs] > > [75626.583199] ? xfs_da3_root_split+0x1050/0x1050 [xfs] > > [75626.607952] ? lock_contended+0xd20/0xd20 > > [75626.626615] ? xfs_ilock+0x149/0x4c0 [xfs] > > [75626.644661] ? down_write_nested+0x187/0x3c0 > > [75626.663892] ? down_write_trylock+0x2f0/0x2f0 > > [75626.683496] ? __sb_start_write+0x1c4/0x310 > > [75626.702389] ? down_read_trylock+0x360/0x360 > > [75626.721669] ? xfs_trans_buf_set_type+0x90/0x1e0 [xfs] > > [75626.745171] xfs_attr_inactive+0x3e5/0x7b0 [xfs] > > [75626.766097] ? xfs_attr3_node_inactive+0x8a0/0x8a0 [xfs] > > [75626.790101] ? lock_downgrade+0x6d0/0x6d0 > > [75626.808122] ? do_raw_spin_trylock+0xb2/0x180 > > [75626.827859] ? lock_contended+0xd20/0xd20 > > [75626.846154] xfs_inactive+0x4b8/0x5b0 [xfs] > > [75626.865504] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > > [75626.887615] destroy_inode+0xbc/0x1a0 > > [75626.904172] do_unlinkat+0x451/0x5d0 > > [75626.920325] ? __ia32_sys_rmdir+0x40/0x40 > > [75626.938485] ? __check_object_size+0x275/0x324 > > [75626.958819] ? strncpy_from_user+0x7d/0x350 > > [75626.977848] do_syscall_64+0x9f/0x4f0 > > [75626.994333] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [75627.017173] RIP: 0033:0x7f968239567b > > [75627.033260] Code: 73 01 c3 48 8b 0d 0d d8 2c 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 b8 07 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d dd d7 2c 00 f7 d8 64 89 01 48 > > [75627.123796] RSP: 002b:00007ffcdf66ad38 EFLAGS: 00000246 ORIG_RAX: 0000000000000107 > > [75627.158521] RAX: ffffffffffffffda RBX: 0000562cd8b5d5b0 RCX: 00007f968239567b > > [75627.190764] RDX: 0000000000000000 RSI: 0000562cd8b5c380 RDI: 00000000ffffff9c > > [75627.222921] RBP: 0000562cd8b5c2f0 R08: 0000000000000003 R09: 0000000000000000 > > [75627.255236] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffcdf66af20 > > [75627.287435] R13: 0000000000000000 R14: 0000562cd8b5d5b0 R15: 0000000000000000 > > > > [75627.326616] Allocated by task 30390: > > [75627.342780] save_stack+0x19/0x80 > > [75627.357980] __kasan_kmalloc.constprop.7+0xc1/0xd0 > > [75627.379553] kmem_cache_alloc+0xc8/0x300 > > [75627.397288] kmem_zone_alloc+0x10a/0x3f0 [xfs] > > [75627.417376] _xfs_buf_alloc+0x56/0x1140 [xfs] > > [75627.437051] xfs_buf_get_map+0x126/0x7c0 [xfs] > > [75627.457103] xfs_buf_read_map+0xb2/0xaa0 [xfs] > > [75627.477180] xfs_trans_read_buf_map+0x6c8/0x12d0 [xfs] > > [75627.500420] xfs_da_read_buf+0x1d9/0x2c0 [xfs] > > [75627.520579] xfs_da3_node_read+0x23/0x80 [xfs] > > [75627.540620] xfs_attr_inactive+0x5c5/0x7b0 [xfs] > > [75627.561609] xfs_inactive+0x4b8/0x5b0 [xfs] > > [75627.581541] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > > [75627.605628] destroy_inode+0xbc/0x1a0 > > [75627.624025] do_unlinkat+0x451/0x5d0 > > [75627.641629] do_syscall_64+0x9f/0x4f0 > > [75627.658156] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > [75627.687232] Freed by task 30390: > > [75627.701882] save_stack+0x19/0x80 > > [75627.716821] __kasan_slab_free+0x125/0x170 > > [75627.735329] kmem_cache_free+0xcd/0x400 > > [75627.752745] xfs_buf_rele+0x30a/0xcb0 [xfs] > > [75627.772731] xfs_attr3_node_inactive+0x1c7/0x8a0 [xfs] > > [75627.797384] xfs_attr_inactive+0x3e5/0x7b0 [xfs] > > [75627.818450] xfs_inactive+0x4b8/0x5b0 [xfs] > > [75627.837455] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] > > [75627.859765] destroy_inode+0xbc/0x1a0 > > [75627.876296] do_unlinkat+0x451/0x5d0 > > [75627.892466] do_syscall_64+0x9f/0x4f0 > > [75627.909015] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > [75627.938572] The buggy address belongs to the object at ffff88881ffaad80 > > which belongs to the cache xfs_buf of size 680 > > [75627.994075] The buggy address is located 644 bytes inside of > > 680-byte region [ffff88881ffaad80, ffff88881ffab028) > > [75628.047015] The buggy address belongs to the page: > > [75628.069056] page:ffffea00207fea00 refcount:1 mapcount:0 mapping:ffff888098515400 index:0xffff88881ffa9d40 compound_mapcount: 0 > > [75628.124539] raw: 0057ffffc0010200 dead000000000100 dead000000000122 ffff888098515400 > > [75628.162598] raw: ffff88881ffa9d40 0000000080270025 00000001ffffffff 0000000000000000 > > [75628.197491] page dumped because: kasan: bad access detected > > > > [75628.230389] Memory state around the buggy address: > > [75628.252072] ffff88881ffaaf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > [75628.284801] ffff88881ffaaf80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > [75628.317587] >ffff88881ffab000: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > > [75628.350592] ^ > > [75628.364746] ffff88881ffab080: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb > > [75628.397289] ffff88881ffab100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > [75628.429955] ================================================================== > > > > Signed-off-by: Zorro Lang <zlang@redhat.com> > > --- > > fs/xfs/xfs_attr_inactive.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > index bbfa6ba84dcd..26230d150bf2 100644 > > --- a/fs/xfs/xfs_attr_inactive.c > > +++ b/fs/xfs/xfs_attr_inactive.c > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > &child_bp); > > if (error) > > return error; > > - error = bp->b_error; > > + error = child_bp->b_error; > > if (error) { > > xfs_trans_brelse(*trans, child_bp); > > return error; > > > > > -- > chandan > > >
On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > gets 'child_bp' at there: > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > child_blkno, > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > &child_bp); > if (error) > return error; > error = bp->b_error; > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > xfs_trans_brelse(*trans, bp); .... > --- > fs/xfs/xfs_attr_inactive.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > index bbfa6ba84dcd..26230d150bf2 100644 > --- a/fs/xfs/xfs_attr_inactive.c > +++ b/fs/xfs/xfs_attr_inactive.c > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > &child_bp); > if (error) > return error; > - error = bp->b_error; > + error = child_bp->b_error; > if (error) { > xfs_trans_brelse(*trans, child_bp); > return error; Isn't this dead code now? i.e. any error that occurs on the buffer during a xfs_trans_get_buf() call is returned directly and so it's caught by the "if (error)" check. Hence this whole child_bp->b_error check can be removed, right? Cheers, Dave.
On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > gets 'child_bp' at there: > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > child_blkno, > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > &child_bp); > > if (error) > > return error; > > error = bp->b_error; > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > xfs_trans_brelse(*trans, bp); > > .... > > --- > > fs/xfs/xfs_attr_inactive.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > index bbfa6ba84dcd..26230d150bf2 100644 > > --- a/fs/xfs/xfs_attr_inactive.c > > +++ b/fs/xfs/xfs_attr_inactive.c > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > &child_bp); > > if (error) > > return error; > > - error = bp->b_error; > > + error = child_bp->b_error; > > if (error) { > > xfs_trans_brelse(*trans, child_bp); > > return error; > > Isn't this dead code now? i.e. any error that occurs on the buffer > during a xfs_trans_get_buf() call is returned directly and so it's > caught by the "if (error)" check. Hence this whole child_bp->b_error > check can be removed, right? It will be after I send in the second half of the 5.6 merge window. I decided to hang onto the buffer error code rework until all of the kernel fuzz tests finished running and I was satisfied with my own userspace port of the same series. (All that is now done, so I'll send that to linus tomorrow.) --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > gets 'child_bp' at there: > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > child_blkno, > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > &child_bp); > > if (error) > > return error; > > error = bp->b_error; > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > xfs_trans_brelse(*trans, bp); > > .... > > --- > > fs/xfs/xfs_attr_inactive.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > index bbfa6ba84dcd..26230d150bf2 100644 > > --- a/fs/xfs/xfs_attr_inactive.c > > +++ b/fs/xfs/xfs_attr_inactive.c > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > &child_bp); > > if (error) > > return error; > > - error = bp->b_error; > > + error = child_bp->b_error; > > if (error) { > > xfs_trans_brelse(*trans, child_bp); > > return error; > > Isn't this dead code now? i.e. any error that occurs on the buffer > during a xfs_trans_get_buf() call is returned directly and so it's > caught by the "if (error)" check. Hence this whole child_bp->b_error > check can be removed, right? Thanks, by looking into the xfs_trans_get_buf() code, I think you're right. Sorry I didn't recognise that before. But when should we check the bp->b_error? and when's it not necessary? In other words, when XFS set the bp->b_error? Looks like it's set in some *verify* functions and ioend time? Thanks, Zorro > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com >
On Tue, Feb 04, 2020 at 04:09:10PM -0800, Darrick J. Wong wrote: > On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > > gets 'child_bp' at there: > > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > > child_blkno, > > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > > &child_bp); > > > if (error) > > > return error; > > > error = bp->b_error; > > > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > > xfs_trans_brelse(*trans, bp); > > > > .... > > > --- > > > fs/xfs/xfs_attr_inactive.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > > index bbfa6ba84dcd..26230d150bf2 100644 > > > --- a/fs/xfs/xfs_attr_inactive.c > > > +++ b/fs/xfs/xfs_attr_inactive.c > > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > > &child_bp); > > > if (error) > > > return error; > > > - error = bp->b_error; > > > + error = child_bp->b_error; > > > if (error) { > > > xfs_trans_brelse(*trans, child_bp); > > > return error; > > > > Isn't this dead code now? i.e. any error that occurs on the buffer > > during a xfs_trans_get_buf() call is returned directly and so it's > > caught by the "if (error)" check. Hence this whole child_bp->b_error > > check can be removed, right? > > It will be after I send in the second half of the 5.6 merge window. I > decided to hang onto the buffer error code rework until all of the > kernel fuzz tests finished running and I was satisfied with my own > userspace port of the same series. > > (All that is now done, so I'll send that to linus tomorrow.) Oh, that's great! Please ignore this noise(/patch) :) Thanks, Zorro > > --D > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com >
On Wed, Feb 05, 2020 at 11:58:30AM +0800, Zorro Lang wrote: > On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > > gets 'child_bp' at there: > > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > > child_blkno, > > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > > &child_bp); > > > if (error) > > > return error; > > > error = bp->b_error; > > > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > > xfs_trans_brelse(*trans, bp); > > > > .... > > > --- > > > fs/xfs/xfs_attr_inactive.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > > index bbfa6ba84dcd..26230d150bf2 100644 > > > --- a/fs/xfs/xfs_attr_inactive.c > > > +++ b/fs/xfs/xfs_attr_inactive.c > > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > > &child_bp); > > > if (error) > > > return error; > > > - error = bp->b_error; > > > + error = child_bp->b_error; > > > if (error) { > > > xfs_trans_brelse(*trans, child_bp); > > > return error; > > > > Isn't this dead code now? i.e. any error that occurs on the buffer > > during a xfs_trans_get_buf() call is returned directly and so it's > > caught by the "if (error)" check. Hence this whole child_bp->b_error > > check can be removed, right? > > Thanks, by looking into the xfs_trans_get_buf() code, I think you're right. Sorry > I didn't recognise that before. > > But when should we check the bp->b_error? and when's it not necessary? > In other words, when XFS set the bp->b_error? Looks like it's set in some *verify* > functions and ioend time? "Always check b_error after reading." But please do note that the the buffer read functions will return it for you now, so you don't have to check it separately in those cases. (The verifiers and ioend functions are lower level and have to check it explicitly.) --D > Thanks, > Zorro > > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com > > >
On Wed, Feb 05, 2020 at 12:02:11PM +0800, Zorro Lang wrote: > On Tue, Feb 04, 2020 at 04:09:10PM -0800, Darrick J. Wong wrote: > > On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > > > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > > > gets 'child_bp' at there: > > > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > > > child_blkno, > > > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > > > &child_bp); > > > > if (error) > > > > return error; > > > > error = bp->b_error; > > > > > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > > > xfs_trans_brelse(*trans, bp); > > > > > > .... > > > > --- > > > > fs/xfs/xfs_attr_inactive.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > > > index bbfa6ba84dcd..26230d150bf2 100644 > > > > --- a/fs/xfs/xfs_attr_inactive.c > > > > +++ b/fs/xfs/xfs_attr_inactive.c > > > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > > > &child_bp); > > > > if (error) > > > > return error; > > > > - error = bp->b_error; > > > > + error = child_bp->b_error; > > > > if (error) { > > > > xfs_trans_brelse(*trans, child_bp); > > > > return error; > > > > > > Isn't this dead code now? i.e. any error that occurs on the buffer > > > during a xfs_trans_get_buf() call is returned directly and so it's > > > caught by the "if (error)" check. Hence this whole child_bp->b_error > > > check can be removed, right? > > > > It will be after I send in the second half of the 5.6 merge window. I > > decided to hang onto the buffer error code rework until all of the > > kernel fuzz tests finished running and I was satisfied with my own > > userspace port of the same series. > > > > (All that is now done, so I'll send that to linus tomorrow.) Hi, Has this issue been fixed? Due to I still hit xfs/433 fail on latest xfs-linux: [53972.827180] run fstests xfs/433 at 2020-10-24 23:11:10 [53975.984049] XFS (vda3): Mounting V5 Filesystem [53976.134116] XFS (vda3): Ending clean mount [53976.210045] xfs filesystem being mounted at /mnt/xfstests/scratch supports timestamps until 2038 (0x7fffffff) [53982.471101] XFS (vda3): Unmounting Filesystem [53982.823725] XFS (vda3): Mounting V5 Filesystem [53982.876916] XFS (vda3): Ending clean mount [53982.898987] xfs filesystem being mounted at /mnt/xfstests/scratch supports timestamps until 2038 (0x7fffffff) [53982.949671] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" [53982.952998] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" [53982.957436] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" [53982.960791] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" [53982.963502] ================================================================== [53982.966070] BUG: KASAN: use-after-free in xfs_attr3_node_inactive+0x684/0x7c0 [xfs] [53982.968000] Read of size 4 at addr fffffc0106509854 by task rm/2847654 [53982.970018] CPU: 0 PID: 2847654 Comm: rm Tainted: G W 5.9.0-rc4 #1 [53982.971872] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015 [53982.973583] Call trace: [53982.974323] dump_backtrace+0x0/0x3d0 [53982.975267] show_stack+0x1c/0x28 [53982.976159] dump_stack+0x13c/0x1b8 [53982.977092] print_address_description.constprop.12+0x68/0x4e0 [53982.978575] kasan_report+0x134/0x1b8 [53982.979521] __asan_report_load4_noabort+0x2c/0x50 [53982.980876] xfs_attr3_node_inactive+0x684/0x7c0 [xfs] [53982.982320] xfs_attr_inactive+0x414/0x7e8 [xfs] [53982.983633] xfs_inactive+0x398/0x4e8 [xfs] [53982.984835] xfs_fs_destroy_inode+0x440/0xf48 [xfs] [53982.986080] destroy_inode+0xa8/0x178 [53982.987024] evict+0x2bc/0x4a8 [53982.987804] iput+0x504/0x8f8 [53982.988561] do_unlinkat+0x36c/0x480 [53982.989482] __arm64_sys_unlinkat+0x94/0xf0 [53982.990558] do_el0_svc+0x1c4/0x3c0 [53982.991442] el0_sync_handler+0xf8/0x124 [53982.992437] el0_sync+0x140/0x180 [53982.993669] Allocated by task 2847654: [53982.994663] kasan_save_stack+0x24/0x50 [53982.995682] __kasan_kmalloc.isra.6+0xc4/0xe0 [53982.996798] kasan_slab_alloc+0x14/0x20 [53982.997768] slab_post_alloc_hook+0x5c/0x548 [53982.998846] kmem_cache_alloc+0x170/0x400 [53983.000016] _xfs_buf_alloc+0x68/0x1318 [xfs] [53983.001249] xfs_buf_get_map+0x13c/0xcb8 [xfs] [53983.002504] xfs_buf_read_map+0xbc/0xdc0 [xfs] [53983.003765] xfs_trans_read_buf_map+0x534/0x1ff0 [xfs] [53983.005209] xfs_da_read_buf+0x1ac/0x258 [xfs] [53983.006487] xfs_da3_node_read+0x3c/0x98 [xfs] [53983.007735] xfs_attr_inactive+0x5b8/0x7e8 [xfs] [53983.009034] xfs_inactive+0x398/0x4e8 [xfs] [53983.010240] xfs_fs_destroy_inode+0x440/0xf48 [xfs] [53983.011507] destroy_inode+0xa8/0x178 [53983.012442] evict+0x2bc/0x4a8 [53983.013228] iput+0x504/0x8f8 [53983.013972] do_unlinkat+0x36c/0x480 [53983.014884] __arm64_sys_unlinkat+0x94/0xf0 [53983.015964] do_el0_svc+0x1c4/0x3c0 [53983.016864] el0_sync_handler+0xf8/0x124 [53983.017858] el0_sync+0x140/0x180 [53983.019086] Freed by task 2847654: [53983.019946] kasan_save_stack+0x24/0x50 [53983.020953] kasan_set_track+0x24/0x38 [53983.021908] kasan_set_free_info+0x20/0x40 [53983.022940] __kasan_slab_free+0x100/0x170 [53983.023973] kasan_slab_free+0x10/0x18 [53983.024924] slab_free_freelist_hook+0xf8/0x260 [53983.026093] kmem_cache_free+0xd0/0x4b0 [53983.027205] xfs_buf_free+0x354/0x958 [xfs] [53983.028399] xfs_buf_rele+0x1040/0x1a90 [xfs] [53983.029643] xfs_trans_brelse+0x294/0x858 [xfs] [53983.030941] xfs_attr3_node_inactive+0x1a4/0x7c0 [xfs] [53983.032396] xfs_attr_inactive+0x414/0x7e8 [xfs] [53983.033693] xfs_inactive+0x398/0x4e8 [xfs] [53983.034889] xfs_fs_destroy_inode+0x440/0xf48 [xfs] [53983.036143] destroy_inode+0xa8/0x178 [53983.037083] evict+0x2bc/0x4a8 [53983.037868] iput+0x504/0x8f8 [53983.038636] do_unlinkat+0x36c/0x480 [53983.039553] __arm64_sys_unlinkat+0x94/0xf0 [53983.040622] do_el0_svc+0x1c4/0x3c0 [53983.041515] el0_sync_handler+0xf8/0x124 [53983.042523] el0_sync+0x140/0x180 [53983.043750] The buggy address belongs to the object at fffffc0106509600 which belongs to the cache xfs_buf of size 632 [53983.046824] The buggy address is located 596 bytes inside of 632-byte region [fffffc0106509600, fffffc0106509878) [53983.049772] The buggy address belongs to the page: [53983.050993] page:00000000e91654da refcount:1 mapcount:0 mapping:0000000000000000 index:0xfffffc0106500000 pfn:0x14650 [53983.053681] flags: 0x17ffff8000000200(slab) [53983.054748] raw: 17ffff8000000200 ffffffff7ffaaa80 0000000600000006 fffffc012154a380 [53983.056713] raw: fffffc0106500000 000000008055004a 00000001ffffffff 0000000000000000 [53983.058688] page dumped because: kasan: bad access detected [53983.060494] Memory state around the buggy address: [53983.061719] fffffc0106509700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [53983.063569] fffffc0106509780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [53983.065350] >fffffc0106509800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fc [53983.067180] ^ [53983.068670] fffffc0106509880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [53983.070474] fffffc0106509900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [53983.072340] ================================================================== > > Oh, that's great! Please ignore this noise(/patch) :) > > Thanks, > Zorro > > > > > --D > > > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@fromorbit.com > > >
On Mon, Oct 26, 2020 at 01:19:48PM +0800, Zorro Lang wrote: > On Wed, Feb 05, 2020 at 12:02:11PM +0800, Zorro Lang wrote: > > On Tue, Feb 04, 2020 at 04:09:10PM -0800, Darrick J. Wong wrote: > > > On Wed, Feb 05, 2020 at 08:39:32AM +1100, Dave Chinner wrote: > > > > On Tue, Feb 04, 2020 at 03:06:36PM +0800, Zorro Lang wrote: > > > > > This patch fixes below KASAN report. The xfs_attr3_node_inactive() > > > > > gets 'child_bp' at there: > > > > > error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, > > > > > child_blkno, > > > > > XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, > > > > > &child_bp); > > > > > if (error) > > > > > return error; > > > > > error = bp->b_error; > > > > > > > > > > But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: > > > > > xfs_trans_brelse(*trans, bp); > > > > > > > > .... > > > > > --- > > > > > fs/xfs/xfs_attr_inactive.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c > > > > > index bbfa6ba84dcd..26230d150bf2 100644 > > > > > --- a/fs/xfs/xfs_attr_inactive.c > > > > > +++ b/fs/xfs/xfs_attr_inactive.c > > > > > @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( > > > > > &child_bp); > > > > > if (error) > > > > > return error; > > > > > - error = bp->b_error; > > > > > + error = child_bp->b_error; > > > > > if (error) { > > > > > xfs_trans_brelse(*trans, child_bp); > > > > > return error; > > > > > > > > Isn't this dead code now? i.e. any error that occurs on the buffer > > > > during a xfs_trans_get_buf() call is returned directly and so it's > > > > caught by the "if (error)" check. Hence this whole child_bp->b_error > > > > check can be removed, right? > > > > > > It will be after I send in the second half of the 5.6 merge window. I > > > decided to hang onto the buffer error code rework until all of the > > > kernel fuzz tests finished running and I was satisfied with my own > > > userspace port of the same series. > > > > > > (All that is now done, so I'll send that to linus tomorrow.) > > Hi, > > Has this issue been fixed? Due to I still hit xfs/433 fail on latest xfs-linux: Clearly it hasn't. Taking a second look at this, I think the _get_buf call could return 0, having set *child_bp to a buffer that previously encountered an IO error. If that was the case, we just brelse the buffer and bail out. If there wasn't a previous error, we invalidate the buffer and keep going. IOWs, I think this patch is correct. Could you rebase this to 5.10-rc1 and resend it, please? --D > [53972.827180] run fstests xfs/433 at 2020-10-24 23:11:10 > [53975.984049] XFS (vda3): Mounting V5 Filesystem > [53976.134116] XFS (vda3): Ending clean mount > [53976.210045] xfs filesystem being mounted at /mnt/xfstests/scratch supports timestamps until 2038 (0x7fffffff) > [53982.471101] XFS (vda3): Unmounting Filesystem > [53982.823725] XFS (vda3): Mounting V5 Filesystem > [53982.876916] XFS (vda3): Ending clean mount > [53982.898987] xfs filesystem being mounted at /mnt/xfstests/scratch supports timestamps until 2038 (0x7fffffff) > [53982.949671] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" > [53982.952998] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" > [53982.957436] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" > [53982.960791] XFS (vda3): Injecting error (false) at file fs/xfs/xfs_buf.c, line 2345, on filesystem "vda3" > [53982.963502] ================================================================== > [53982.966070] BUG: KASAN: use-after-free in xfs_attr3_node_inactive+0x684/0x7c0 [xfs] > [53982.968000] Read of size 4 at addr fffffc0106509854 by task rm/2847654 > > [53982.970018] CPU: 0 PID: 2847654 Comm: rm Tainted: G W 5.9.0-rc4 #1 > [53982.971872] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015 > [53982.973583] Call trace: > [53982.974323] dump_backtrace+0x0/0x3d0 > [53982.975267] show_stack+0x1c/0x28 > [53982.976159] dump_stack+0x13c/0x1b8 > [53982.977092] print_address_description.constprop.12+0x68/0x4e0 > [53982.978575] kasan_report+0x134/0x1b8 > [53982.979521] __asan_report_load4_noabort+0x2c/0x50 > [53982.980876] xfs_attr3_node_inactive+0x684/0x7c0 [xfs] > [53982.982320] xfs_attr_inactive+0x414/0x7e8 [xfs] > [53982.983633] xfs_inactive+0x398/0x4e8 [xfs] > [53982.984835] xfs_fs_destroy_inode+0x440/0xf48 [xfs] > [53982.986080] destroy_inode+0xa8/0x178 > [53982.987024] evict+0x2bc/0x4a8 > [53982.987804] iput+0x504/0x8f8 > [53982.988561] do_unlinkat+0x36c/0x480 > [53982.989482] __arm64_sys_unlinkat+0x94/0xf0 > [53982.990558] do_el0_svc+0x1c4/0x3c0 > [53982.991442] el0_sync_handler+0xf8/0x124 > [53982.992437] el0_sync+0x140/0x180 > > [53982.993669] Allocated by task 2847654: > [53982.994663] kasan_save_stack+0x24/0x50 > [53982.995682] __kasan_kmalloc.isra.6+0xc4/0xe0 > [53982.996798] kasan_slab_alloc+0x14/0x20 > [53982.997768] slab_post_alloc_hook+0x5c/0x548 > [53982.998846] kmem_cache_alloc+0x170/0x400 > [53983.000016] _xfs_buf_alloc+0x68/0x1318 [xfs] > [53983.001249] xfs_buf_get_map+0x13c/0xcb8 [xfs] > [53983.002504] xfs_buf_read_map+0xbc/0xdc0 [xfs] > [53983.003765] xfs_trans_read_buf_map+0x534/0x1ff0 [xfs] > [53983.005209] xfs_da_read_buf+0x1ac/0x258 [xfs] > [53983.006487] xfs_da3_node_read+0x3c/0x98 [xfs] > [53983.007735] xfs_attr_inactive+0x5b8/0x7e8 [xfs] > [53983.009034] xfs_inactive+0x398/0x4e8 [xfs] > [53983.010240] xfs_fs_destroy_inode+0x440/0xf48 [xfs] > [53983.011507] destroy_inode+0xa8/0x178 > [53983.012442] evict+0x2bc/0x4a8 > [53983.013228] iput+0x504/0x8f8 > [53983.013972] do_unlinkat+0x36c/0x480 > [53983.014884] __arm64_sys_unlinkat+0x94/0xf0 > [53983.015964] do_el0_svc+0x1c4/0x3c0 > [53983.016864] el0_sync_handler+0xf8/0x124 > [53983.017858] el0_sync+0x140/0x180 > > [53983.019086] Freed by task 2847654: > [53983.019946] kasan_save_stack+0x24/0x50 > [53983.020953] kasan_set_track+0x24/0x38 > [53983.021908] kasan_set_free_info+0x20/0x40 > [53983.022940] __kasan_slab_free+0x100/0x170 > [53983.023973] kasan_slab_free+0x10/0x18 > [53983.024924] slab_free_freelist_hook+0xf8/0x260 > [53983.026093] kmem_cache_free+0xd0/0x4b0 > [53983.027205] xfs_buf_free+0x354/0x958 [xfs] > [53983.028399] xfs_buf_rele+0x1040/0x1a90 [xfs] > [53983.029643] xfs_trans_brelse+0x294/0x858 [xfs] > [53983.030941] xfs_attr3_node_inactive+0x1a4/0x7c0 [xfs] > [53983.032396] xfs_attr_inactive+0x414/0x7e8 [xfs] > [53983.033693] xfs_inactive+0x398/0x4e8 [xfs] > [53983.034889] xfs_fs_destroy_inode+0x440/0xf48 [xfs] > [53983.036143] destroy_inode+0xa8/0x178 > [53983.037083] evict+0x2bc/0x4a8 > [53983.037868] iput+0x504/0x8f8 > [53983.038636] do_unlinkat+0x36c/0x480 > [53983.039553] __arm64_sys_unlinkat+0x94/0xf0 > [53983.040622] do_el0_svc+0x1c4/0x3c0 > [53983.041515] el0_sync_handler+0xf8/0x124 > [53983.042523] el0_sync+0x140/0x180 > > [53983.043750] The buggy address belongs to the object at fffffc0106509600 > which belongs to the cache xfs_buf of size 632 > [53983.046824] The buggy address is located 596 bytes inside of > 632-byte region [fffffc0106509600, fffffc0106509878) > [53983.049772] The buggy address belongs to the page: > [53983.050993] page:00000000e91654da refcount:1 mapcount:0 mapping:0000000000000000 index:0xfffffc0106500000 pfn:0x14650 > [53983.053681] flags: 0x17ffff8000000200(slab) > [53983.054748] raw: 17ffff8000000200 ffffffff7ffaaa80 0000000600000006 fffffc012154a380 > [53983.056713] raw: fffffc0106500000 000000008055004a 00000001ffffffff 0000000000000000 > [53983.058688] page dumped because: kasan: bad access detected > > [53983.060494] Memory state around the buggy address: > [53983.061719] fffffc0106509700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [53983.063569] fffffc0106509780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [53983.065350] >fffffc0106509800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fc > [53983.067180] ^ > [53983.068670] fffffc0106509880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > [53983.070474] fffffc0106509900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > [53983.072340] ================================================================== > > > > > Oh, that's great! Please ignore this noise(/patch) :) > > > > Thanks, > > Zorro > > > > > > > > --D > > > > > > > Cheers, > > > > > > > > Dave. > > > > -- > > > > Dave Chinner > > > > david@fromorbit.com > > > > > >
diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c index bbfa6ba84dcd..26230d150bf2 100644 --- a/fs/xfs/xfs_attr_inactive.c +++ b/fs/xfs/xfs_attr_inactive.c @@ -211,7 +211,7 @@ xfs_attr3_node_inactive( &child_bp); if (error) return error; - error = bp->b_error; + error = child_bp->b_error; if (error) { xfs_trans_brelse(*trans, child_bp); return error;
This patch fixes below KASAN report. The xfs_attr3_node_inactive() gets 'child_bp' at there: error = xfs_trans_get_buf(*trans, mp->m_ddev_targp, child_blkno, XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, &child_bp); if (error) return error; error = bp->b_error; But it turns to use 'bp', not 'child_bp'. And the 'bp' has been freed by: xfs_trans_brelse(*trans, bp); [75626.212549] ================================================================== [75626.245606] BUG: KASAN: use-after-free in xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] [75626.280164] Read of size 4 at addr ffff88881ffab004 by task rm/30390 [75626.315595] CPU: 13 PID: 30390 Comm: rm Tainted: G W 5.5.0+ #1 [75626.347856] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 08/02/2014 [75626.377864] Call Trace: [75626.388868] dump_stack+0x96/0xe0 [75626.403778] print_address_description.constprop.4+0x1f/0x300 [75626.429656] __kasan_report.cold.8+0x76/0xb0 [75626.448950] ? xfs_trans_ordered_buf+0x410/0x440 [xfs] [75626.472393] ? xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] [75626.496705] kasan_report+0xe/0x20 [75626.512134] xfs_attr3_node_inactive+0x61e/0x8a0 [xfs] [75626.535328] ? xfs_da_read_buf+0x235/0x2c0 [xfs] [75626.557270] ? xfs_attr3_leaf_inactive+0x470/0x470 [xfs] [75626.583199] ? xfs_da3_root_split+0x1050/0x1050 [xfs] [75626.607952] ? lock_contended+0xd20/0xd20 [75626.626615] ? xfs_ilock+0x149/0x4c0 [xfs] [75626.644661] ? down_write_nested+0x187/0x3c0 [75626.663892] ? down_write_trylock+0x2f0/0x2f0 [75626.683496] ? __sb_start_write+0x1c4/0x310 [75626.702389] ? down_read_trylock+0x360/0x360 [75626.721669] ? xfs_trans_buf_set_type+0x90/0x1e0 [xfs] [75626.745171] xfs_attr_inactive+0x3e5/0x7b0 [xfs] [75626.766097] ? xfs_attr3_node_inactive+0x8a0/0x8a0 [xfs] [75626.790101] ? lock_downgrade+0x6d0/0x6d0 [75626.808122] ? do_raw_spin_trylock+0xb2/0x180 [75626.827859] ? lock_contended+0xd20/0xd20 [75626.846154] xfs_inactive+0x4b8/0x5b0 [xfs] [75626.865504] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] [75626.887615] destroy_inode+0xbc/0x1a0 [75626.904172] do_unlinkat+0x451/0x5d0 [75626.920325] ? __ia32_sys_rmdir+0x40/0x40 [75626.938485] ? __check_object_size+0x275/0x324 [75626.958819] ? strncpy_from_user+0x7d/0x350 [75626.977848] do_syscall_64+0x9f/0x4f0 [75626.994333] entry_SYSCALL_64_after_hwframe+0x49/0xbe [75627.017173] RIP: 0033:0x7f968239567b [75627.033260] Code: 73 01 c3 48 8b 0d 0d d8 2c 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 b8 07 01 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d dd d7 2c 00 f7 d8 64 89 01 48 [75627.123796] RSP: 002b:00007ffcdf66ad38 EFLAGS: 00000246 ORIG_RAX: 0000000000000107 [75627.158521] RAX: ffffffffffffffda RBX: 0000562cd8b5d5b0 RCX: 00007f968239567b [75627.190764] RDX: 0000000000000000 RSI: 0000562cd8b5c380 RDI: 00000000ffffff9c [75627.222921] RBP: 0000562cd8b5c2f0 R08: 0000000000000003 R09: 0000000000000000 [75627.255236] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffcdf66af20 [75627.287435] R13: 0000000000000000 R14: 0000562cd8b5d5b0 R15: 0000000000000000 [75627.326616] Allocated by task 30390: [75627.342780] save_stack+0x19/0x80 [75627.357980] __kasan_kmalloc.constprop.7+0xc1/0xd0 [75627.379553] kmem_cache_alloc+0xc8/0x300 [75627.397288] kmem_zone_alloc+0x10a/0x3f0 [xfs] [75627.417376] _xfs_buf_alloc+0x56/0x1140 [xfs] [75627.437051] xfs_buf_get_map+0x126/0x7c0 [xfs] [75627.457103] xfs_buf_read_map+0xb2/0xaa0 [xfs] [75627.477180] xfs_trans_read_buf_map+0x6c8/0x12d0 [xfs] [75627.500420] xfs_da_read_buf+0x1d9/0x2c0 [xfs] [75627.520579] xfs_da3_node_read+0x23/0x80 [xfs] [75627.540620] xfs_attr_inactive+0x5c5/0x7b0 [xfs] [75627.561609] xfs_inactive+0x4b8/0x5b0 [xfs] [75627.581541] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] [75627.605628] destroy_inode+0xbc/0x1a0 [75627.624025] do_unlinkat+0x451/0x5d0 [75627.641629] do_syscall_64+0x9f/0x4f0 [75627.658156] entry_SYSCALL_64_after_hwframe+0x49/0xbe [75627.687232] Freed by task 30390: [75627.701882] save_stack+0x19/0x80 [75627.716821] __kasan_slab_free+0x125/0x170 [75627.735329] kmem_cache_free+0xcd/0x400 [75627.752745] xfs_buf_rele+0x30a/0xcb0 [xfs] [75627.772731] xfs_attr3_node_inactive+0x1c7/0x8a0 [xfs] [75627.797384] xfs_attr_inactive+0x3e5/0x7b0 [xfs] [75627.818450] xfs_inactive+0x4b8/0x5b0 [xfs] [75627.837455] xfs_fs_destroy_inode+0x3dc/0xb80 [xfs] [75627.859765] destroy_inode+0xbc/0x1a0 [75627.876296] do_unlinkat+0x451/0x5d0 [75627.892466] do_syscall_64+0x9f/0x4f0 [75627.909015] entry_SYSCALL_64_after_hwframe+0x49/0xbe [75627.938572] The buggy address belongs to the object at ffff88881ffaad80 which belongs to the cache xfs_buf of size 680 [75627.994075] The buggy address is located 644 bytes inside of 680-byte region [ffff88881ffaad80, ffff88881ffab028) [75628.047015] The buggy address belongs to the page: [75628.069056] page:ffffea00207fea00 refcount:1 mapcount:0 mapping:ffff888098515400 index:0xffff88881ffa9d40 compound_mapcount: 0 [75628.124539] raw: 0057ffffc0010200 dead000000000100 dead000000000122 ffff888098515400 [75628.162598] raw: ffff88881ffa9d40 0000000080270025 00000001ffffffff 0000000000000000 [75628.197491] page dumped because: kasan: bad access detected [75628.230389] Memory state around the buggy address: [75628.252072] ffff88881ffaaf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [75628.284801] ffff88881ffaaf80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [75628.317587] >ffff88881ffab000: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc [75628.350592] ^ [75628.364746] ffff88881ffab080: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb [75628.397289] ffff88881ffab100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [75628.429955] ================================================================== Signed-off-by: Zorro Lang <zlang@redhat.com> --- fs/xfs/xfs_attr_inactive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)