linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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
* 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
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;
as well as URLs for NNTP newsgroup(s).