1. Patchew
  2. linux
  3. View series

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

MottiKumar Babu posted 1 patch 4 weeks ago 
fs/xfs/libxfs/xfs_bmap.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork
Posted by MottiKumar Babu 4 weeks ago 
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)

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)
-- 
2.43.0
Re: [PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork
Posted by kernel test robot 3 weeks, 3 days ago 

Hello,

kernel test robot noticed "xfstests.xfs.221.fail" on:

commit: fc3ce21bdce1661bf1b4af5579a7c72c483b0856 ("[PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork")
url: https://github.com/intel-lab-lkp/linux/commits/MottiKumar-Babu/Fix-out-of-bounds-access-in-xfs_bmapi_allocate-by-validating-whichfork/20241028-033645
base: https://git.kernel.org/cgit/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/all/20241027193541.14212-1-mottikumarbabu@gmail.com/
patch subject: [PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork

in testcase: xfstests
version: xfstests-x86_64-891f4995-1_20241028
with following parameters:

	disk: 4HDD
	fs: xfs
	test: xfs-221



config: x86_64-rhel-8.3-func
compiler: gcc-12
test machine: 4 threads Intel(R) Xeon(R) CPU E3-1225 v5 @ 3.30GHz (Skylake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202411011120.8d4b756-oliver.sang@intel.com

2024-10-31 14:35:22 export TEST_DIR=/fs/sda1
2024-10-31 14:35:22 export TEST_DEV=/dev/sda1
2024-10-31 14:35:22 export FSTYP=xfs
2024-10-31 14:35:22 export SCRATCH_MNT=/fs/scratch
2024-10-31 14:35:22 mkdir /fs/scratch -p
2024-10-31 14:35:22 export SCRATCH_DEV=/dev/sda4
2024-10-31 14:35:22 export SCRATCH_LOGDEV=/dev/sda2
2024-10-31 14:35:22 export SCRATCH_XFS_LIST_METADATA_FIELDS=u3.sfdir3.hdr.parent.i4
2024-10-31 14:35:22 export SCRATCH_XFS_LIST_FUZZ_VERBS=random
2024-10-31 14:35:22 export MKFS_OPTIONS="-mreflink=1 "
2024-10-31 14:35:22 echo xfs/221
2024-10-31 14:35:22 ./check xfs/221
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 lkp-skl-d06 6.12.0-rc2-00034-gfc3ce21bdce1 #1 SMP PREEMPT_DYNAMIC Mon Oct 28 04:16:50 CST 2024
MKFS_OPTIONS  -- -f -mreflink=1 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /fs/scratch

xfs/221       - output mismatch (see /lkp/benchmarks/xfstests/results//xfs/221.out.bad)
    --- tests/xfs/221.out	2024-10-28 16:28:46.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//xfs/221.out.bad	2024-10-31 14:35:30.136173763 +0000
    @@ -8,6 +8,6 @@
     CoW across the transition
     Compare files
     bdbcf02ee0aa977795a79d25fcfdccb1  SCRATCH_MNT/test-221/file1
    -09101629908f9bdd5d178e7ce20bb1bb  SCRATCH_MNT/test-221/file3
    +fa50dba51826899c372464a153cb2117  SCRATCH_MNT/test-221/file3
     09101629908f9bdd5d178e7ce20bb1bb  SCRATCH_MNT/test-221/file3.chk
     Check extent counts
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/xfs/221.out /lkp/benchmarks/xfstests/results//xfs/221.out.bad'  to see the entire diff)
Ran: xfs/221
Failures: xfs/221
Failed 1 of 1 tests




The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241101/202411011120.8d4b756-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork
Posted by Christoph Hellwig 4 weeks ago 
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)
> 
> 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

How is this supposed to happen?
Re: [PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork
Posted by Dave Chinner 4 weeks ago 
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
[PATCH] Follow-up on Submitted Patch: Fix out-of-bounds access in xfs_bmapi_allocate
Posted by MottiKumar Babu 4 weeks ago 
Hi everyone,

I hope this message finds you well. I wanted to follow up on my previously submitted patch titled "[PATCH] Fix out-of-bounds access in xfs_bmapi_allocate by validating whichfork."

As a reminder, this patch addresses an issue reported by Coverity Scan (CID 1633175), where the variable `whichfork` can take invalid values, specifically `2`, leading to an out-of-bounds access in `xfs_bmbt_init_cursor`. The added validation check ensures that `whichfork` remains within the valid range of 0 or 1. If it falls outside this range, the function will return `-EINVAL`, enhancing the code's robustness and preventing potential crashes or undefined behavior.

I appreciate any feedback or suggestions you might have.

Best,  
MottiKumar Babu  
mottikumarbabu@gmail.com

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.