diff mbox series

[RFC,v2,12/14] xfs: make the calculation generic in xfs_sb_validate_fsb_count()

Message ID 20240213093713.1753368-13-kernel@pankajraghav.com (mailing list archive)
State New
Headers show
Series enable bs > ps in XFS | expand

Commit Message

Pankaj Raghav (Samsung) Feb. 13, 2024, 9:37 a.m. UTC 
From: Pankaj Raghav <p.raghav@samsung.com>

Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
make the calculation generic so that page cache count can be calculated
correctly for LBS.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 fs/xfs/xfs_mount.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong Feb. 13, 2024, 4:26 p.m. UTC | #1 
On Tue, Feb 13, 2024 at 10:37:11AM +0100, Pankaj Raghav (Samsung) wrote:
> From: Pankaj Raghav <p.raghav@samsung.com>
> 
> Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
> make the calculation generic so that page cache count can be calculated
> correctly for LBS.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>  fs/xfs/xfs_mount.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index aabb25dc3efa..bfbaaecaf668 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -133,9 +133,13 @@ xfs_sb_validate_fsb_count(
>  {
>  	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
>  	ASSERT(sbp->sb_blocklog >= BBSHIFT);
> +	unsigned long mapping_count;

Nit: indenting

	unsigned long		mapping_count;

> +	uint64_t bytes = nblocks << sbp->sb_blocklog;

What happens if someone feeds us a garbage fs with sb_blocklog > 64?
Or did we check that previously, so an overflow isn't possible?

> +
> +	mapping_count = bytes >> PAGE_SHIFT;

Does this result in truncation when unsigned long is 32 bits?

--D

>  
>  	/* Limited by ULONG_MAX of page cache index */
> -	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
> +	if (mapping_count > ULONG_MAX)
>  		return -EFBIG;
>  	return 0;
>  }
> -- 
> 2.43.0
> 
>
Pankaj Raghav (Samsung) Feb. 13, 2024, 9:48 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 08:26:11AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 13, 2024 at 10:37:11AM +0100, Pankaj Raghav (Samsung) wrote:
> > From: Pankaj Raghav <p.raghav@samsung.com>
> > 
> > Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
> > make the calculation generic so that page cache count can be calculated
> > correctly for LBS.
> > 
> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> > ---
> >  fs/xfs/xfs_mount.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index aabb25dc3efa..bfbaaecaf668 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -133,9 +133,13 @@ xfs_sb_validate_fsb_count(
> >  {
> >  	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
> >  	ASSERT(sbp->sb_blocklog >= BBSHIFT);
> > +	unsigned long mapping_count;
> 
> Nit: indenting
> 
> 	unsigned long		mapping_count;

I will add this in the next revision.
> 
> > +	uint64_t bytes = nblocks << sbp->sb_blocklog;
> 
> What happens if someone feeds us a garbage fs with sb_blocklog > 64?
> Or did we check that previously, so an overflow isn't possible?
> 
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?

> > +
> > +	mapping_count = bytes >> PAGE_SHIFT;
> 
> Does this result in truncation when unsigned long is 32 bits?

Ah, good point. So it is better to use uint64_t for mapping_count as
well to be on the safe side?

> 
> --D
> 
> >  
> >  	/* Limited by ULONG_MAX of page cache index */
> > -	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
> > +	if (mapping_count > ULONG_MAX)
> >  		return -EFBIG;
> >  	return 0;
> >  }
> > -- 
> > 2.43.0
> > 
> >
Dave Chinner Feb. 13, 2024, 10:44 p.m. UTC | #3 
On Tue, Feb 13, 2024 at 10:48:17PM +0100, Pankaj Raghav (Samsung) wrote:
> On Tue, Feb 13, 2024 at 08:26:11AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 13, 2024 at 10:37:11AM +0100, Pankaj Raghav (Samsung) wrote:
> > > From: Pankaj Raghav <p.raghav@samsung.com>
> > > 
> > > Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
> > > make the calculation generic so that page cache count can be calculated
> > > correctly for LBS.
> > > 
> > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> > > ---
> > >  fs/xfs/xfs_mount.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index aabb25dc3efa..bfbaaecaf668 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -133,9 +133,13 @@ xfs_sb_validate_fsb_count(
> > >  {
> > >  	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
> > >  	ASSERT(sbp->sb_blocklog >= BBSHIFT);
> > > +	unsigned long mapping_count;
> > 
> > Nit: indenting
> > 
> > 	unsigned long		mapping_count;
> 
> I will add this in the next revision.
> > 
> > > +	uint64_t bytes = nblocks << sbp->sb_blocklog;
> > 
> > What happens if someone feeds us a garbage fs with sb_blocklog > 64?
> > Or did we check that previously, so an overflow isn't possible?
> > 
> 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....

-Dave.
Pankaj Raghav (Samsung) Feb. 14, 2024, 3:51 p.m. UTC | #4 
> > 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@fromorbit.com
diff mbox series

Patch

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index aabb25dc3efa..bfbaaecaf668 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -133,9 +133,13 @@  xfs_sb_validate_fsb_count(
 {
 	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
 	ASSERT(sbp->sb_blocklog >= BBSHIFT);
+	unsigned long mapping_count;
+	uint64_t bytes = nblocks << sbp->sb_blocklog;
+
+	mapping_count = bytes >> PAGE_SHIFT;
 
 	/* Limited by ULONG_MAX of page cache index */
-	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
+	if (mapping_count > ULONG_MAX)
 		return -EFBIG;
 	return 0;
 }