* [PATCH] xfs: fix an undefined behaviour in _da3_path_shift @ 2020-02-25 14:37 Qian Cai 2020-02-25 15:28 ` Darrick J. Wong 2020-02-25 18:07 ` Christoph Hellwig 0 siblings, 2 replies; 6+ messages in thread From: Qian Cai @ 2020-02-25 14:37 UTC (permalink / raw) To: darrick.wong; +Cc: linux-xfs, linux-kernel, Qian Cai 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]. 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]; + + for (; level >= 0; blk--, level--) { xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, blk->bp->b_addr); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift 2020-02-25 14:37 [PATCH] xfs: fix an undefined behaviour in _da3_path_shift Qian Cai @ 2020-02-25 15:28 ` Darrick J. Wong 2020-02-25 15:46 ` Qian Cai 2020-02-25 18:07 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2020-02-25 15:28 UTC (permalink / raw) To: Qian Cai; +Cc: linux-xfs, linux-kernel 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@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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift 2020-02-25 15:28 ` Darrick J. Wong @ 2020-02-25 15:46 ` Qian Cai 2020-02-25 19:08 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Qian Cai @ 2020-02-25 15:46 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift 2020-02-25 15:46 ` Qian Cai @ 2020-02-25 19:08 ` Darrick J. Wong 0 siblings, 0 replies; 6+ messages in thread From: Darrick J. Wong @ 2020-02-25 19:08 UTC (permalink / raw) To: Qian Cai; +Cc: linux-xfs, linux-kernel 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 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift 2020-02-25 14:37 [PATCH] xfs: fix an undefined behaviour in _da3_path_shift Qian Cai 2020-02-25 15:28 ` Darrick J. Wong @ 2020-02-25 18:07 ` Christoph Hellwig 2020-02-25 18:23 ` Qian Cai 1 sibling, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2020-02-25 18:07 UTC (permalink / raw) To: Qian Cai; +Cc: darrick.wong, linux-xfs, linux-kernel I think we code do this a tad more cleaner, something like: diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c index 875e04f82541..542a4edfcf54 100644 --- a/fs/xfs/libxfs/xfs_da_btree.c +++ b/fs/xfs/libxfs/xfs_da_btree.c @@ -1986,7 +1986,8 @@ xfs_da3_path_shift( 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--) { + for ( ; level >= 0; level--) { + blk = &path->blk[level]; xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, blk->bp->b_addr); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: fix an undefined behaviour in _da3_path_shift 2020-02-25 18:07 ` Christoph Hellwig @ 2020-02-25 18:23 ` Qian Cai 0 siblings, 0 replies; 6+ messages in thread From: Qian Cai @ 2020-02-25 18:23 UTC (permalink / raw) To: Christoph Hellwig; +Cc: darrick.wong, linux-xfs, linux-kernel On Tue, 2020-02-25 at 10:07 -0800, Christoph Hellwig wrote: > I think we code do this a tad more cleaner, something like: > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index 875e04f82541..542a4edfcf54 100644 > --- a/fs/xfs/libxfs/xfs_da_btree.c > +++ b/fs/xfs/libxfs/xfs_da_btree.c > @@ -1986,7 +1986,8 @@ xfs_da3_path_shift( > 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--) { > + for ( ; level >= 0; level--) { > + blk = &path->blk[level]; > xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, > blk->bp->b_addr); > Yes, indeed. I'll send a v2 until Darrick is still not convinced that "path->active == 1" could reach here? ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-25 19:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-25 14:37 [PATCH] xfs: fix an undefined behaviour in _da3_path_shift Qian Cai 2020-02-25 15:28 ` Darrick J. Wong 2020-02-25 15:46 ` Qian Cai 2020-02-25 19:08 ` Darrick J. Wong 2020-02-25 18:07 ` Christoph Hellwig 2020-02-25 18:23 ` Qian Cai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox