[PATCH] xfs: add bounds checking to xlog_recover_process_data

[PATCH] xfs: add bounds checking to xlog_recover_process_data

[lei lu]

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;
+		}
 
 		/* errors will abort recovery */
 		error = xlog_recover_process_ophdr(log, rhash, rhead, ohead,
-- 
2.34.1

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  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;
> +		}
>  
>  		/* errors will abort recovery */
>  		error = xlog_recover_process_ophdr(log, rhash, rhead, ohead,
> -- 
> 2.34.1
> 
> 

[PATCH] xfs: add bounds checking to xlog_recover_process_data

[Kuntal Nayak]

From: lei lu <llfamsec@gmail.com>

[ Upstream commit fb63435b7c7dc112b1ae1baea5486e0a6e27b196 ]

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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
Signed-off-by: Kuntal Nayak <kuntal.nayak@broadcom.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 e61f28ce3..eafe76f30 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2419,7 +2419,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;
+		}
 
 		/* errors will abort recovery */
 		error = xlog_recover_process_ophdr(log, rhash, rhead, ohead,
-- 
2.39.3