Re: [PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 28, 2024 at 01:05:27AM +0530, MottiKumar Babu wrote:
> This issue was reported by Coverity Scan.
> 
> Report:
> CID 1633175 Out-of-bounds access - Access of memory not owned by this buffer may cause crashes or incorrect computations.
> In xfs_bmapi_allocate: Out-of-bounds access to a buffer (CWE-119)

We really need more details than thisi about the issue. I have no
idea what issue this describes, nor the code which it refers to.
Where is the out of bounds memory access occurring, how does it
trigger and where does the code end up crashing as a result?

A link to the coverity report woudl certainly help....

> Signed-off-by: MottiKumar Babu <mottikumarbabu@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 36dd08d13293..6ff378d2d3d9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4169,6 +4169,10 @@ xfs_bmapi_allocate(
>  		 * is not on the busy list.
>  		 */
>  		bma->datatype = XFS_ALLOC_NOBUSY;
> +		// Ensure whichfork is valid (0 or 1) before further checks
> +		if (whichfork < 0 || whichfork > 1) {
> +			return -EINVAL; // Invalid fork
> +		}
>  		if (whichfork == XFS_DATA_FORK || whichfork == XFS_COW_FORK) {
>  			bma->datatype |= XFS_ALLOC_USERDATA;
>  			if (bma->offset == 0)

That's not going to work. If you look at how whichfork is
initialised early on in the xfs_bmapi_allocate() function, you'll
see it calls this function:

static inline int xfs_bmapi_whichfork(uint32_t bmapi_flags)
{
        if (bmapi_flags & XFS_BMAPI_COWFORK)
                return XFS_COW_FORK;
        else if (bmapi_flags & XFS_BMAPI_ATTRFORK)
                return XFS_ATTR_FORK;
        return XFS_DATA_FORK;
}

A value of 2 (XFS_COW_FORK) is definitely a valid value for
whichfork to have. Indeed, the line of code after the fix checks if
whichfork == XFS_COW_FORK, indicating that such a value is expected
and should be handled correctly.

However, this patch will result in rejecting any request to allocate
blocks in the XFS_COW_FORK. This will fail any COW operation we try
to perform with -EINVAL. i.e. overwrites after a reflink copy will
fail.

This sort of regression would be picked up very quickly by fstests.
Hence it is important that any change - even simple changes - are
regression tested before they are proposed for review and merge....

-Dave.

-- 
Dave Chinner
david@fromorbit.com