[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200225152805.GG6740@magnolia>
Date: Tue, 25 Feb 2020 07:28:05 -0800
From: "Darrick J. Wong" <darrick.wong@...cle.com>
To: Qian Cai <cai@....pw>
Cc: linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift
On Tue, Feb 25, 2020 at 09:37:57AM -0500, Qian Cai wrote:
> state->path.active could be 1 in xfs_da3_node_lookup_int() and then in
> xfs_da3_path_shift() could see state->path.blk[-1].
Under what circumstancs can it be 1? Is this a longstanding bug in XFS?
A corrupted filesystem? A deliberately corrupted filesystem?
>
> UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_da_btree.c:1989:14
> index -1 is out of range for type 'xfs_da_state_blk_t [5]'
> Call trace:
> dump_backtrace+0x0/0x2c8
> show_stack+0x20/0x2c
> dump_stack+0xe8/0x150
> __ubsan_handle_out_of_bounds+0xe4/0xfc
> xfs_da3_path_shift+0x860/0x86c [xfs]
> xfs_da3_node_lookup_int+0x7c8/0x934 [xfs]
> xfs_dir2_node_addname+0x2c8/0xcd0 [xfs]
> xfs_dir_createname+0x348/0x38c [xfs]
> xfs_create+0x6b0/0x8b4 [xfs]
> xfs_generic_create+0x12c/0x1f8 [xfs]
> xfs_vn_mknod+0x3c/0x4c [xfs]
> xfs_vn_create+0x34/0x44 [xfs]
> do_last+0xd4c/0x10c8
> path_openat+0xbc/0x2f4
> do_filp_open+0x74/0xf4
> do_sys_openat2+0x98/0x180
> __arm64_sys_openat+0xf8/0x170
> do_el0_svc+0x170/0x240
> el0_sync_handler+0x150/0x250
> el0_sync+0x164/0x180
>
> Signed-off-by: Qian Cai <cai@....pw>
> ---
> fs/xfs/libxfs/xfs_da_btree.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 875e04f82541..0906b7748a3f 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -1986,7 +1986,11 @@ static inline int xfs_dabuf_nfsb(struct xfs_mount *mp, int whichfork)
> ASSERT(path != NULL);
> ASSERT((path->active > 0) && (path->active < XFS_DA_NODE_MAXDEPTH));
> level = (path->active-1) - 1; /* skip bottom layer in path */
> - for (blk = &path->blk[level]; level >= 0; blk--, level--) {
> +
> + if (level >= 0)
> + blk = &path->blk[level];
...because if the reason is "corrupt metadata" then perhaps this should
return -EFSCORRUPTED? But I don't know enough about the context to know
the answer to that question.
--D
> +
> + for (; level >= 0; blk--, level--) {
> xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
> blk->bp->b_addr);
>
> --
> 1.8.3.1
>
Powered by blists - more mailing lists