linux-kernel - Re: [RFC v2 12/14] xfs: make the calculation generic in xfs_sb_validate_fsb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 14 Feb 2024 16:51:22 +0100
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Dave Chinner <david@...morbit.com>
Cc: "Darrick J. Wong" <djwong@...nel.org>, linux-xfs@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, mcgrof@...nel.org, gost.dev@...sung.com, 
	akpm@...ux-foundation.org, kbusch@...nel.org, chandan.babu@...cle.com, p.raghav@...sung.com, 
	linux-kernel@...r.kernel.org, hare@...e.de, willy@...radead.org, linux-mm@...ck.org
Subject: Re: [RFC v2 12/14] xfs: make the calculation generic in
 xfs_sb_validate_fsb_count()

> > I was thinking of possibility of an overflow but at the moment the 
> > blocklog is capped at 16 (65536 bytes) right? mkfs refuses any block
> > sizes more than 64k. And we have check for this in xfs_validate_sb_common()
> > in the kernel, which will catch it before this happens?
> 
> The sb_blocklog is checked in the superblock verifier when we first read in the
> superblock:
> 
> 	    sbp->sb_blocksize < XFS_MIN_BLOCKSIZE                       ||
>             sbp->sb_blocksize > XFS_MAX_BLOCKSIZE                       ||
>             sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG                    ||
>             sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG                    ||
>             sbp->sb_blocksize != (1 << sbp->sb_blocklog)                ||
> 
> #define XFS_MAX_BLOCKSIZE_LOG 16
> 
> However, we pass mp->m_sb.sb_dblocks or m_sb.sb_rblocks to this
> function, and they are validated by the same verifier as invalid
> if:
> 
> 	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)
> 
> #define XFS_MAX_DBLOCKS(s) ((xfs_rfsblock_t)(s)->sb_agcount *
>                                              (s)->sb_agblocks)
> 
> Which means as long as someone can corrupt some combination of
> sb_dblocks, sb_agcount and sb_agblocks that allows sb_dblocks to be
> greater than 2^48 on a 64kB fsb fs, then that the above code:
> 
> 	uint64_t bytes = nblocks << sbp->sb_blocklog;
> 
> will overflow.
> 
> I also suspect that we can feed a huge rtdev to this new code
> and have it overflow without needing to corrupt the superblock in
> any way....

So we could use the check_mul_overflow to detect these cases:

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 596aa2cdefbc..23faa993fb80 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -132,8 +132,12 @@ xfs_sb_validate_fsb_count(
        uint64_t        nblocks)
 {
        ASSERT(sbp->sb_blocklog >= BBSHIFT);
-       unsigned long mapping_count;
-       uint64_t bytes = nblocks << sbp->sb_blocklog;
+       uint64_t mapping_count;
+       uint64_t bytes;
+
+       if (check_mul_overflow(nblocks, (1 << sbp->sb_blocklog), &bytes))
+               return -EFBIG;
 
        if (!IS_ENABLED(CONFIG_XFS_LBS))
                ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);

> 
> -Dave.
> -- 
> Dave Chinner
> david@...morbit.com