On Thu, Jun 29, 2023 at 12:42:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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.
--
Dave Chinner
david@xxxxxxxxxxxxx
