On Sat, Jul 09, 2022 at 03:52:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> The kernel build robot reported a UAF error while running xfs/433
> (edited somewhat for brevity):
>
> BUG: KASAN: use-after-free in xfs_attr3_node_inactive (fs/xfs/xfs_attr_inactive.c:214) xfs
> Read of size 4 at addr ffff88820ac2bd44 by task kworker/0:2/139
>
> CPU: 0 PID: 139 Comm: kworker/0:2 Tainted: G S 5.19.0-rc2-00004-g7cf2b0f9611b #1
> Hardware name: Hewlett-Packard p6-1451cx/2ADA, BIOS 8.15 02/05/2013
> Workqueue: xfs-inodegc/sdb4 xfs_inodegc_worker [xfs]
> Call Trace:
> <TASK>
> dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
> print_address_description+0x1f/0x200
> print_report.cold (mm/kasan/report.c:430)
> kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493)
> xfs_attr3_node_inactive (fs/xfs/xfs_attr_inactive.c:214) xfs
> xfs_attr3_root_inactive (fs/xfs/xfs_attr_inactive.c:296) xfs
> xfs_attr_inactive (fs/xfs/xfs_attr_inactive.c:371) xfs
> xfs_inactive (fs/xfs/xfs_inode.c:1781) xfs
> xfs_inodegc_worker (fs/xfs/xfs_icache.c:1837 fs/xfs/xfs_icache.c:1860) xfs
> process_one_work
> worker_thread
> kthread
> ret_from_fork
> </TASK>
>
> Allocated by task 139:
> kasan_save_stack (mm/kasan/common.c:39)
> __kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:469)
> kmem_cache_alloc (mm/slab.h:750 mm/slub.c:3214 mm/slub.c:3222 mm/slub.c:3229 mm/slub.c:3239)
> _xfs_buf_alloc (include/linux/instrumented.h:86 include/linux/atomic/atomic-instrumented.h:41 fs/xfs/xfs_buf.c:232) xfs
> xfs_buf_get_map (fs/xfs/xfs_buf.c:660) xfs
> xfs_buf_read_map (fs/xfs/xfs_buf.c:777) xfs
> xfs_trans_read_buf_map (fs/xfs/xfs_trans_buf.c:289) xfs
> xfs_da_read_buf (fs/xfs/libxfs/xfs_da_btree.c:2652) xfs
> xfs_da3_node_read (fs/xfs/libxfs/xfs_da_btree.c:392) xfs
> xfs_attr3_root_inactive (fs/xfs/xfs_attr_inactive.c:272) xfs
> xfs_attr_inactive (fs/xfs/xfs_attr_inactive.c:371) xfs
> xfs_inactive (fs/xfs/xfs_inode.c:1781) xfs
> xfs_inodegc_worker (fs/xfs/xfs_icache.c:1837 fs/xfs/xfs_icache.c:1860) xfs
> process_one_work
> worker_thread
> kthread
> ret_from_fork
>
> Freed by task 139:
> kasan_save_stack (mm/kasan/common.c:39)
> kasan_set_track (mm/kasan/common.c:45)
> kasan_set_free_info (mm/kasan/generic.c:372)
> __kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328 mm/kasan/common.c:374)
> kmem_cache_free (mm/slub.c:1753 mm/slub.c:3507 mm/slub.c:3524)
> xfs_buf_rele (fs/xfs/xfs_buf.c:1040) xfs
> xfs_attr3_node_inactive (fs/xfs/xfs_attr_inactive.c:210) xfs
> xfs_attr3_root_inactive (fs/xfs/xfs_attr_inactive.c:296) xfs
> xfs_attr_inactive (fs/xfs/xfs_attr_inactive.c:371) xfs
> xfs_inactive (fs/xfs/xfs_inode.c:1781) xfs
> xfs_inodegc_worker (fs/xfs/xfs_icache.c:1837 fs/xfs/xfs_icache.c:1860) xfs
> process_one_work
> worker_thread
> kthread
> ret_from_fork
>
> I reproduced this for my own satisfaction, and got the same report,
> along with an extra morsel:
>
> The buggy address belongs to the object at ffff88802103a800
> which belongs to the cache xfs_buf of size 432
> The buggy address is located 396 bytes inside of
> 432-byte region [ffff88802103a800, ffff88802103a9b0)
>
> I tracked this code down to:
>
> 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;
>
> That doesn't look right -- I think this should be dereferencing
> child_bp, not bp. Looking through the codebase history, I think this
> was added by commit 2911edb653b9 ("xfs: remove the mappedbno argument to
> xfs_da_get_buf"), which replaced a call to xfs_da_get_buf with the
> current call to xfs_trans_get_buf. Not sure why we trans_brelse'd @bp
> earlier in the function, but I'm guessing it's to avoid pinning too many
> buffers in memory while we inactivate the bottom of the attr tree.
> Hence we now have to get the buffer back.
>
> I /think/ this was supposed to check child_bp->b_error and fail the rest
> of the invalidation if child_bp had experienced any kind of IO or
> corruption error. I bet the xfs_da3_node_read earlier in the loop will
> catch most cases of incoming on-disk corruption which makes this check
> mostly moot unless someone corrupts the buffer and the AIL pushes it out
> to disk while the buffer's unlocked.
>
> In the first case we'll never get to the bad check, and in the second
> case the AIL will shut down the log, at which point there's no reason to
> check b_error. Remove the check, and null out @bp to avoid this problem
> in the future.
>
> Cc: hch@xxxxxx
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Fixes: 2911edb653b9 ("xfs: remove the mappedbno argument to xfs_da_get_buf")
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> v2: remove the entire b_error test and bailout block, null out some more
> freed variables
> ---
> fs/xfs/xfs_attr_inactive.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> index 0e83cab9cdde..895ba8b7a26c 100644
> --- a/fs/xfs/xfs_attr_inactive.c
> +++ b/fs/xfs/xfs_attr_inactive.c
> @@ -158,6 +158,7 @@ xfs_attr3_node_inactive(
> }
> child_fsb = be32_to_cpu(ichdr.btree[0].before);
> xfs_trans_brelse(*trans, bp); /* no locks for later trans */
> + bp = NULL;
>
> /*
> * If this is the node level just above the leaves, simply loop
> @@ -211,12 +212,8 @@ xfs_attr3_node_inactive(
> &child_bp);
> if (error)
> return error;
> - error = bp->b_error;
> - if (error) {
> - xfs_trans_brelse(*trans, child_bp);
> - return error;
> - }
> xfs_trans_binval(*trans, child_bp);
> + child_bp = NULL;
>
> /*
> * If we're not done, re-read the parent to get the next
> @@ -233,6 +230,7 @@ xfs_attr3_node_inactive(
> bp->b_addr);
> child_fsb = be32_to_cpu(phdr.btree[i + 1].before);
> xfs_trans_brelse(*trans, bp);
> + bp = NULL;
> }
> /*
> * Atomically commit the whole invalidate stuff.
Looks good now.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
