Re: [PATCH] xfs: add bounds checking to xlog_recover_process_data

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 03, 2024 at 05:46:08PM +0800, lei lu wrote:
> There is a lack of verification of the space occupied by fixed members
> of xlog_op_header in the xlog_recover_process_data.
> 
> We can create a crafted image to trigger an out of bounds read by
> following these steps:
>     1) Mount an image of xfs, and do some file operations to leave records
>     2) Before umounting, copy the image for subsequent steps to simulate
>        abnormal exit. Because umount will ensure that tail_blk and
>        head_blk are the same, which will result in the inability to enter
>        xlog_recover_process_data
>     3) Write a tool to parse and modify the copied image in step 2
>     4) Make the end of the xlog_op_header entries only 1 byte away from
>        xlog_rec_header->h_size
>     5) xlog_rec_header->h_num_logops++
>     6) Modify xlog_rec_header->h_crc
> 
> Fix:
> Add a check to make sure there is sufficient space to access fixed members
> of xlog_op_header.
> 
> Signed-off-by: lei lu <llfamsec@gmail.com>
> ---
>  fs/xfs/xfs_log_recover.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 1251c81e55f9..14609ce212db 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2456,7 +2456,10 @@ xlog_recover_process_data(
>  
>  		ohead = (struct xlog_op_header *)dp;
>  		dp += sizeof(*ohead);
> -		ASSERT(dp <= end);
> +		if (dp > end) {
> +			xfs_warn(log->l_mp, "%s: op header overrun", __func__);
> +			return -EFSCORRUPTED;
> +		}

looks fine.

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

-- 
Dave Chinner
david@fromorbit.com