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;
Date:   Tue, 25 Feb 2020 11:08:04 -0800
From:   "Darrick J. Wong" <darrick.wong@oracle.com>
To:     Qian Cai <cai@lca.pw>
Cc:     linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift
Message-ID: <20200225190804.GP6740@magnolia>
References: <1582641477-4011-1-git-send-email-cai@lca.pw>
 <20200225152805.GG6740@magnolia>
 <1582645616.7365.118.camel@lca.pw>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1582645616.7365.118.camel@lca.pw>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 25, 2020 at 10:46:56AM -0500, Qian Cai wrote:
> 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 ?

Yes.  The commit message ought to state that active == 1 is a valid
state when we're trying to add an entry to a single dir leaf block and
are trying to shift forward to see if there's a sibling block that would
be a better place to put the new entry.

This is to build confidence in future readers that we actually
understood the circumstances of the UBSAN error and aren't just
monkeypatching the code to shut up the automated checks.

--D

> 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
> > >