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:   Wed, 26 Feb 2020 08:28:36 -0800
From:   "Darrick J. Wong" <darrick.wong@oracle.com>
To:     Qian Cai <cai@lca.pw>
Cc:     hch@infradead.org, linux-xfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] xfs: fix an undefined behaviour in _da3_path_shift
Message-ID: <20200226162836.GC8045@magnolia>
References: <20200226020637.1065-1-cai@lca.pw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200226020637.1065-1-cai@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 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)
>