Re: [PATCH 1/4] xfs: fully initialize xfs_da_args in xchk_directory_blocks

[Dave Chinner <david@fromorbit.com>]

On Sun, Oct 02, 2022 at 11:19:48AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While running the online fsck test suite, I noticed the following
> assertion in the kernel log (edited for brevity):
> 
> XFS: Assertion failed: 0, file: fs/xfs/xfs_health.c, line: 571
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 11667 at fs/xfs/xfs_message.c:104 assfail+0x46/0x4a [xfs]
> CPU: 3 PID: 11667 Comm: xfs_scrub Tainted: G        W         5.19.0-rc7-xfsx #rc7 6e6475eb29fd9dda3181f81b7ca7ff961d277a40
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:assfail+0x46/0x4a [xfs]
> Call Trace:
>  <TASK>
>  xfs_dir2_isblock+0xcc/0xe0
>  xchk_directory_blocks+0xc7/0x420
>  xchk_directory+0x53/0xb0
>  xfs_scrub_metadata+0x2b6/0x6b0
>  xfs_scrubv_metadata+0x35e/0x4d0
>  xfs_ioc_scrubv_metadata+0x111/0x160
>  xfs_file_ioctl+0x4ec/0xef0
>  __x64_sys_ioctl+0x82/0xa0
>  do_syscall_64+0x2b/0x80
>  entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> This assertion triggers in xfs_dirattr_mark_sick when the caller passes
> in a whichfork value that is neither of XFS_{DATA,ATTR}_FORK.  The cause
> of this is that xchk_directory_blocks only partially initializes the
> xfs_da_args structure that is passed to xfs_dir2_isblock.  If the data
> fork is not correct, the XFS_IS_CORRUPT clause will trigger.  My
> development branch reports this failure to the health monitoring
> subsystem, which accesses the uninitialized args->whichfork field,
> leading the the assertion tripping.  We really shouldn't be passing
> random stack contents around, so the solution here is to force the
> compiler to zero-initialize the struct.
> 
> Found by fuzzing u3.bmx[0].blockcount = middlebit on xfs/1554.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/dir.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Looks good, surprised it took this long to trip over this...

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com