| Message ID | 20230629194230.GH11441@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: various fixes for 6.5 | expand |
On Thu, Jun 29, 2023 at 12:42:30PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Similar to the recent patch strengthening the AGF agf_length > verification, the AGI verifier does not check that the AGI length field > is within known good bounds. This isn't currently checked by runtime > kernel code, yet we assume in many places that it is correct and verify > other metadata against it. > > Add length verification to the AGI verifier. Just like the AGF length > checking, the length of the AGI 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. > > There's only one place in the filesystem that actually uses agi_length, > but let's not leave it vulnerable to the same weird nonsense that > generates syzbot bugs, eh? > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > NOTE: Untested, but this patch builds... > --- > fs/xfs/libxfs/xfs_ialloc.c | 49 ++++++++++++++++++++++++++++++++------------ > 1 file changed, 36 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c > index 1e5fafbc0cdb..fec6713e1fa9 100644 > --- a/fs/xfs/libxfs/xfs_ialloc.c > +++ b/fs/xfs/libxfs/xfs_ialloc.c > @@ -2486,11 +2486,12 @@ xfs_ialloc_log_agi( > > static xfs_failaddr_t > xfs_agi_verify( > - struct xfs_buf *bp) > + struct xfs_buf *bp) > { > - struct xfs_mount *mp = bp->b_mount; > - struct xfs_agi *agi = bp->b_addr; > - int i; > + struct xfs_mount *mp = bp->b_mount; > + struct xfs_agi *agi = bp->b_addr; > + uint32_t agi_length = be32_to_cpu(agi->agi_length); > + int i; > > if (xfs_has_crc(mp)) { > if (!uuid_equal(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid)) > @@ -2507,6 +2508,37 @@ xfs_agi_verify( > if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum))) > return __this_address; > > + /* > + * Both agi_seqno and agi_length need to validated before anything else > + * block number related in the AGI can be checked. > + * > + * During growfs operations, the perag is not fully initialised, > + * so we can't use it for any useful checking. growfs ensures we can't > + * use it by using uncached buffers that don't have the perag attached > + * so we can detect and avoid this problem. > + */ > + if (bp->b_pag && be32_to_cpu(agi->agi_seqno) != bp->b_pag->pag_agno) > + return __this_address; > + > + /* > + * Only the last AGI in the filesytsem is allowed to be shorter > + * than the AG size recorded in the superblock. > + */ > + if (agi_length != mp->m_sb.sb_agblocks) { > + /* > + * During growfs, the new last AGI can get here before we > + * have updated the superblock. Give it a pass on the seqno > + * check. > + */ > + if (bp->b_pag && > + be32_to_cpu(agi->agi_seqno) != mp->m_sb.sb_agcount - 1) > + return __this_address; > + if (agi_length < XFS_MIN_AG_BLOCKS) > + return __this_address; > + if (agi_length > mp->m_sb.sb_agblocks) > + return __this_address; > + } I'd pull this into a helper function that both the AGF and AGI verifiers call. It's the same checks, with the same caveats and growfs landmines, so I think would be better as a helper... Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c index 1e5fafbc0cdb..fec6713e1fa9 100644 --- a/fs/xfs/libxfs/xfs_ialloc.c +++ b/fs/xfs/libxfs/xfs_ialloc.c @@ -2486,11 +2486,12 @@ xfs_ialloc_log_agi( static xfs_failaddr_t xfs_agi_verify( - struct xfs_buf *bp) + struct xfs_buf *bp) { - struct xfs_mount *mp = bp->b_mount; - struct xfs_agi *agi = bp->b_addr; - int i; + struct xfs_mount *mp = bp->b_mount; + struct xfs_agi *agi = bp->b_addr; + uint32_t agi_length = be32_to_cpu(agi->agi_length); + int i; if (xfs_has_crc(mp)) { if (!uuid_equal(&agi->agi_uuid, &mp->m_sb.sb_meta_uuid)) @@ -2507,6 +2508,37 @@ xfs_agi_verify( if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum))) return __this_address; + /* + * Both agi_seqno and agi_length need to validated before anything else + * block number related in the AGI can be checked. + * + * During growfs operations, the perag is not fully initialised, + * so we can't use it for any useful checking. growfs ensures we can't + * use it by using uncached buffers that don't have the perag attached + * so we can detect and avoid this problem. + */ + if (bp->b_pag && be32_to_cpu(agi->agi_seqno) != bp->b_pag->pag_agno) + return __this_address; + + /* + * Only the last AGI in the filesytsem is allowed to be shorter + * than the AG size recorded in the superblock. + */ + if (agi_length != mp->m_sb.sb_agblocks) { + /* + * During growfs, the new last AGI can get here before we + * have updated the superblock. Give it a pass on the seqno + * check. + */ + if (bp->b_pag && + be32_to_cpu(agi->agi_seqno) != mp->m_sb.sb_agcount - 1) + return __this_address; + if (agi_length < XFS_MIN_AG_BLOCKS) + return __this_address; + if (agi_length > mp->m_sb.sb_agblocks) + return __this_address; + } + if (be32_to_cpu(agi->agi_level) < 1 || be32_to_cpu(agi->agi_level) > M_IGEO(mp)->inobt_maxlevels) return __this_address; @@ -2516,15 +2548,6 @@ xfs_agi_verify( be32_to_cpu(agi->agi_free_level) > M_IGEO(mp)->inobt_maxlevels)) return __this_address; - /* - * during growfs operations, the perag is not fully initialised, - * so we can't use it for any useful checking. growfs ensures we can't - * use it by using uncached buffers that don't have the perag attached - * so we can detect and avoid this problem. - */ - if (bp->b_pag && be32_to_cpu(agi->agi_seqno) != bp->b_pag->pag_agno) - return __this_address; - for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) { if (agi->agi_unlinked[i] == cpu_to_be32(NULLAGINO)) continue;