Re: git: 076002f24d35 - main - Do comprehensive UFS/FFS superblock integrity checks when reading a superblock.

On Fri, May 27, 2022 at 07:22:24PM +0000, Kirk McKusick wrote:
> The branch main has been updated by mckusick:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=076002f24d35962f0d21f44bfddd34ee4d7f015d
> 
> commit 076002f24d35962f0d21f44bfddd34ee4d7f015d
> Author:     Kirk McKusick <mckusick@FreeBSD.org>
> AuthorDate: 2022-05-27 19:21:11 +0000
> Commit:     Kirk McKusick <mckusick@FreeBSD.org>
> CommitDate: 2022-05-27 19:22:07 +0000
> 
>     Do comprehensive UFS/FFS superblock integrity checks when reading a superblock.
>     
>     Historically only minimal checks were made of a superblock when it
>     was read in as it was assumed that fsck would have been run to
>     correct any errors before attempting to use the filesystem. Recently
>     several bug reports have been submitted reporting kernel panics
>     that can be triggered by deliberately corrupting filesystem superblocks,
>     see Bug 263979 - [meta] UFS / FFS / GEOM crash (panic) tracking
>     which is tracking the reported corruption bugs.
>     
>     This change upgrades the checks that are performed. These additional
>     checks should prevent panics from a corrupted superblock. Although
>     it appears in only one place, the new code will apply to the kernel
>     modules and (through libufs) user applications that read in superblocks.
>     
>     Reported by:  Robert Morris and Neeraj
>     Reviewed by:  kib
>     Tested by:    Peter Holm
>     PR:           263979
>     MFC after:    1 month
>     Differential Revision: https://reviews.freebsd.org/D35219
> ---
>  sys/ufs/ffs/ffs_subr.c | 163 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 146 insertions(+), 17 deletions(-)
> 
> diff --git a/sys/ufs/ffs/ffs_subr.c b/sys/ufs/ffs/ffs_subr.c
> index 01e9f45e1205..28c2fee1cb37 100644
> --- a/sys/ufs/ffs/ffs_subr.c
> +++ b/sys/ufs/ffs/ffs_subr.c
> [...]
> +static int
> +validate_sblock(struct fs *fs, int isaltsblk)
> +{
> +	int i, sectorsize;
> +	u_int64_t maxfilesize, minfpg, sizepb;
> +
> +	sectorsize = dbtob(1);
> +	if (fs->fs_magic == FS_UFS2_MAGIC) {
> +		if ((!isaltsblk && (fs->fs_sblockloc != SBLOCK_UFS2 ||
> +		    fs->fs_sblockactualloc != SBLOCK_UFS2)) ||
> +		    fs->fs_maxsymlinklen != ((UFS_NDADDR + UFS_NIADDR) *
> +			sizeof(ufs2_daddr_t)) ||
> +		    fs->fs_nindir != fs->fs_bsize / sizeof(ufs2_daddr_t) ||
> +		    fs->fs_inopb != fs->fs_bsize / sizeof(struct ufs2_dinode))
> +			return (ENOENT);
> +	} else if (fs->fs_magic == FS_UFS1_MAGIC) {
> +		if ((!isaltsblk && (fs->fs_sblockloc > SBLOCK_UFS1 ||
> +		    fs->fs_sblockactualloc != SBLOCK_UFS1)) ||

Hi Kirk,

The tests of fs_sblockactualloc fail on older filesystems or those
created by makefs(8), causing mount to fail.  That field was added
relatively recently, so I suspect a value of zero should also be
permitted?