[PATCH v3] xfs: fix an undefined behaviour in _da3_path_shift

[PATCH v3] xfs: fix an undefined behaviour in _da3_path_shift

[Qian Cai]

In xfs_da3_path_shift() "blk" can be assigned to state->path.blk[-1] if
state->path.active is 1 (which is a valid state) when it tries to add an
entry to a single dir leaf block and then to shift forward to see if
there's a sibling block that would be a better place to put the new
entry. This causes a UBSAN warning given negative array indices are
undefined behavior in C. In practice the warning is entirely harmless
given that "blk" is never dereferenced in this case, but it is still
better to fix up the warning and slightly improve the code.

 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

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Qian Cai <cai@lca.pw>
---

 v3: Borrow the commit log from Christoph.
 v2: Update the commit log thanks to Darrick.
     Simplify the code.

 fs/xfs/libxfs/xfs_da_btree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 875e04f82541..e864c3d47f60 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);
 
-- 
2.21.0 (Apple Git-122.2)

Re: [PATCH v3] xfs: fix an undefined behaviour in _da3_path_shift

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3] xfs: fix an undefined behaviour in _da3_path_shift

[Darrick J. Wong]

On Tue, Feb 25, 2020 at 09:06:37PM -0500, Qian Cai wrote:
> In xfs_da3_path_shift() "blk" can be assigned to state->path.blk[-1] if
> state->path.active is 1 (which is a valid state) when it tries to add an
> entry to a single dir leaf block and then to shift forward to see if
> there's a sibling block that would be a better place to put the new
> entry. This causes a UBSAN warning given negative array indices are
> undefined behavior in C. In practice the warning is entirely harmless
> given that "blk" is never dereferenced in this case, but it is still
> better to fix up the warning and slightly improve the code.
> 
>  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
> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Qian Cai <cai@lca.pw>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
>  v3: Borrow the commit log from Christoph.
>  v2: Update the commit log thanks to Darrick.
>      Simplify the code.
> 
>  fs/xfs/libxfs/xfs_da_btree.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 875e04f82541..e864c3d47f60 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);
>  
> -- 
> 2.21.0 (Apple Git-122.2)
>