Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1582645616.7365.118.camel@lca.pw>
Subject: Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift
From:   Qian Cai <cai@lca.pw>
To:     "Darrick J. Wong" <darrick.wong@oracle.com>
Cc:     linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 25 Feb 2020 10:46:56 -0500
In-Reply-To: <20200225152805.GG6740@magnolia>
References: <1582641477-4011-1-git-send-email-cai@lca.pw>
         <20200225152805.GG6740@magnolia>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 2020-02-25 at 07:28 -0800, Darrick J. Wong wrote:
> 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?

in xfs_da3_node_lookup_int(),

	for (blk = &state->path.blk[0], state->path.active = 1;
			 state->path.active <= XFS_DA_NODE_MAXDEPTH;
			 blk++, state->path.active++) {
<snip>
		if (magic == XFS_ATTR_LEAF_MAGIC ||
		    magic == XFS_ATTR3_LEAF_MAGIC) {
			blk->magic = XFS_ATTR_LEAF_MAGIC;
			blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
			break;
		}

		if (magic == XFS_DIR2_LEAFN_MAGIC ||
		    magic == XFS_DIR3_LEAFN_MAGIC) {
			blk->magic = XFS_DIR2_LEAFN_MAGIC;
			blk->hashval = xfs_dir2_leaf_lasthash(args->dp,
							      blk->bp, NULL);
			break;

Isn't that if the first iteration in the loop calls any of those "break", it
will have state->path.active = 1 ?

I suppose this is a long-standing bug that need UBSAN (no obvious harm could be
done later because it will bail out immediately in xfs_da3_path_shift()) and a
set of specific conditions to met to trigger.

> 
> > 
> >  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@lca.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
> >