Skip to content

Commit edd8276

Browse files
Dave ChinnerDarrick J. Wong
authored andcommitted
xfs: AGF length has never been bounds checked
The AGF verifier does not check that the AGF length field is within known good bounds. This has never been checked by runtime kernel code (i.e. the lack of verification goes back to 1993) yet we assume in many places that it is correct and verify other metdata against it. Add length verification to the AGF verifier. The length of the AGF must be equal to the size of the AG specified in the superblock, unless it is the last AG in the filesystem. In that case, it must be less than or equal to sb->sb_agblocks and greater than XFS_MIN_AG_BLOCKS, which is the smallest AG a growfs operation will allow to exist. This requires a bit of rework of the verifier function. We want to verify metadata before we use it to verify other metadata. Hence we need to verify the AGF sequence numbers before using them to verify the length of the AGF. Then we can verify the AGF length before we verify AGFL fields. Then we can verifier other fields that are bounds limited by the AGF length. And, finally, by calculating agf_length only once into a local variable, we can collapse repeated "if (xfs_has_foo() &&" conditionaly checks into single checks. This makes the code much easier to follow as all the checks for a given feature are obviously in the same place. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
1 parent f1e1765 commit edd8276

File tree

1 file changed

+56
-34
lines changed

1 file changed

+56
-34
lines changed

fs/xfs/libxfs/xfs_alloc.c

Lines changed: 56 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2974,6 +2974,7 @@ xfs_agf_verify(
29742974
{
29752975
struct xfs_mount *mp = bp->b_mount;
29762976
struct xfs_agf *agf = bp->b_addr;
2977+
uint32_t agf_length = be32_to_cpu(agf->agf_length);
29772978

29782979
if (xfs_has_crc(mp)) {
29792980
if (!uuid_equal(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid))
@@ -2985,18 +2986,49 @@ xfs_agf_verify(
29852986
if (!xfs_verify_magic(bp, agf->agf_magicnum))
29862987
return __this_address;
29872988

2988-
if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
2989-
be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
2990-
be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
2991-
be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
2992-
be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp)))
2989+
if (!XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)))
29932990
return __this_address;
29942991

2995-
if (be32_to_cpu(agf->agf_length) > mp->m_sb.sb_dblocks)
2992+
/*
2993+
* Both agf_seqno and agf_length need to validated before anything else
2994+
* block number related in the AGF or AGFL can be checked.
2995+
*
2996+
* During growfs operations, the perag is not fully initialised,
2997+
* so we can't use it for any useful checking. growfs ensures we can't
2998+
* use it by using uncached buffers that don't have the perag attached
2999+
* so we can detect and avoid this problem.
3000+
*/
3001+
if (bp->b_pag && be32_to_cpu(agf->agf_seqno) != bp->b_pag->pag_agno)
3002+
return __this_address;
3003+
3004+
/*
3005+
* Only the last AGF in the filesytsem is allowed to be shorter
3006+
* than the AG size recorded in the superblock.
3007+
*/
3008+
if (agf_length != mp->m_sb.sb_agblocks) {
3009+
/*
3010+
* During growfs, the new last AGF can get here before we
3011+
* have updated the superblock. Give it a pass on the seqno
3012+
* check.
3013+
*/
3014+
if (bp->b_pag &&
3015+
be32_to_cpu(agf->agf_seqno) != mp->m_sb.sb_agcount - 1)
3016+
return __this_address;
3017+
if (agf_length < XFS_MIN_AG_BLOCKS)
3018+
return __this_address;
3019+
if (agf_length > mp->m_sb.sb_agblocks)
3020+
return __this_address;
3021+
}
3022+
3023+
if (be32_to_cpu(agf->agf_flfirst) >= xfs_agfl_size(mp))
3024+
return __this_address;
3025+
if (be32_to_cpu(agf->agf_fllast) >= xfs_agfl_size(mp))
3026+
return __this_address;
3027+
if (be32_to_cpu(agf->agf_flcount) > xfs_agfl_size(mp))
29963028
return __this_address;
29973029

29983030
if (be32_to_cpu(agf->agf_freeblks) < be32_to_cpu(agf->agf_longest) ||
2999-
be32_to_cpu(agf->agf_freeblks) > be32_to_cpu(agf->agf_length))
3031+
be32_to_cpu(agf->agf_freeblks) > agf_length)
30003032
return __this_address;
30013033

30023034
if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) < 1 ||
@@ -3007,38 +3039,28 @@ xfs_agf_verify(
30073039
mp->m_alloc_maxlevels)
30083040
return __this_address;
30093041

3010-
if (xfs_has_rmapbt(mp) &&
3011-
(be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]) < 1 ||
3012-
be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]) >
3013-
mp->m_rmap_maxlevels))
3014-
return __this_address;
3015-
3016-
if (xfs_has_rmapbt(mp) &&
3017-
be32_to_cpu(agf->agf_rmap_blocks) > be32_to_cpu(agf->agf_length))
3042+
if (xfs_has_lazysbcount(mp) &&
3043+
be32_to_cpu(agf->agf_btreeblks) > agf_length)
30183044
return __this_address;
30193045

3020-
/*
3021-
* during growfs operations, the perag is not fully initialised,
3022-
* so we can't use it for any useful checking. growfs ensures we can't
3023-
* use it by using uncached buffers that don't have the perag attached
3024-
* so we can detect and avoid this problem.
3025-
*/
3026-
if (bp->b_pag && be32_to_cpu(agf->agf_seqno) != bp->b_pag->pag_agno)
3027-
return __this_address;
3046+
if (xfs_has_rmapbt(mp)) {
3047+
if (be32_to_cpu(agf->agf_rmap_blocks) > agf_length)
3048+
return __this_address;
30283049

3029-
if (xfs_has_lazysbcount(mp) &&
3030-
be32_to_cpu(agf->agf_btreeblks) > be32_to_cpu(agf->agf_length))
3031-
return __this_address;
3050+
if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]) < 1 ||
3051+
be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]) >
3052+
mp->m_rmap_maxlevels)
3053+
return __this_address;
3054+
}
30323055

3033-
if (xfs_has_reflink(mp) &&
3034-
be32_to_cpu(agf->agf_refcount_blocks) >
3035-
be32_to_cpu(agf->agf_length))
3036-
return __this_address;
3056+
if (xfs_has_reflink(mp)) {
3057+
if (be32_to_cpu(agf->agf_refcount_blocks) > agf_length)
3058+
return __this_address;
30373059

3038-
if (xfs_has_reflink(mp) &&
3039-
(be32_to_cpu(agf->agf_refcount_level) < 1 ||
3040-
be32_to_cpu(agf->agf_refcount_level) > mp->m_refc_maxlevels))
3041-
return __this_address;
3060+
if (be32_to_cpu(agf->agf_refcount_level) < 1 ||
3061+
be32_to_cpu(agf->agf_refcount_level) > mp->m_refc_maxlevels)
3062+
return __this_address;
3063+
}
30423064

30433065
return NULL;
30443066
}

0 commit comments

Comments
 (0)