Comprehensive UFS/FFS superblock integrity checks made when a superblock is read

[mckusick]

Summary

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 made by deliberately corrupting filesystem superblocks, see Bug 263979 - [meta] UFS / FFS / GEOM crash (panic) tracking (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263979).

This change upgrades the checks that are performed. These additional checks should prevent panics from a corrupted superblock. Although it appears only in one place, the new code will apply to the kernel modules and user applications that read in superblocks.

Test Plan

The reported corruption scenarios will be tested. Peter Holm will be requested to run his tests against the new code to at least ensure that it does not break anything by being too strict about what it will accept.

[pho]

I have uploaded a minimal test scenario, where the second mount fails with your patch:

mount-error.sh995 BDownload

[kib]

sys/ufs/ffs/ffs_subr.c

312

Should the argument be unsigned?
Then perhaps you could use fls(val) - 1 (please double-check me).
Also might be a good idea to put it into e.g. sys/libkern.h for other users, I did not found existing analog in our sources.

322

Why not use powerof2(x) from sys/param.h ?

[mckusick]

Changes suggested by kib and refinements suggested by Robert Morris

[mckusick]

Still need to track down problem cited by Peter Holm.

sys/ufs/ffs/ffs_subr.c

312

I could use fls() as you suggest which would lead to

#define ILOG2(num) ((1 << (fls(num) - 1) == num) ? (fls(num) - 1) : -1)

or I could create a new function which would not end up calling fts twice. It needs to be used in both kernel and user compiles, so it would have to be in both libkern.h and also added to ffs.3. Not sure that it is useful enough to do that. What do you think?

322

I did not know about powerof2() in sys/param.h. I only looked for the all-cap version since that is what the style guide says such non-function things should be :-) Now using powerof2()

[kib]

sys/ufs/ffs/ffs_subr.c

312

This is a strange function, I must admit. So do you want to get a logarithm if the number is just power of 2, and -1 otherwise? Or do you want the log2 always, except for zero?

I thought that you wanted

#define ILOG2(num) (fls(num) - 1)

and that was my proposal. Your version can be written to use fls only once using GNU extension:

#define ILOG2(num) __extention__ ({
   unsigned _f = fls(num) - 1;
   (1U << _f) == num ? _f : -1;
})

317

In this variant, it should be 1U << (...) I believe, otherwise you get undefined behavior.

[mckusick]

From Peter Holm: Relax bounds checking on fs_ipg and fs_fpg to account for tiny filesystems.

Avoid hanging in kernel malloc when allocating space for superblock summary information by using NOWAIT. Per kib, there is no way to do a bounded wait for kernel memory. So let the application layer delay and retry to check for a transient kernel memory shortage.

[mckusick]

Update to account for reported problems. Hopefully relaxed checks will not open a new door for attack.

sys/ufs/ffs/ffs_subr.c

312

I realize that just using

#define ILOG2(num) (fls(num) - 1)

is actually fine since I have already checked that the num argument is a power of 2 all I need to verify is that ILOG2 is correct. I have made that update in my latest diff.

[kib]

sys/ufs/ffs/ffs_subr.c

386

Are there enough checks to ensure that INOPF() != 0?

[mckusick]

sys/ufs/ffs/ffs_subr.c

386

We check that fs_bsize >= MINBSIZE (4096).
UFS2 has the largest inodes sizeof(struct ufs2_dinode) (256).
We check that fs_inopb == (fs_bsize / sizeof(struct ufs2_dinode)) which must be at least 16 (4096 / 256).
We check that fs_frag <= MAXFRAG (8) and fs_fragshift is ILOG2(fs_frag).
Since INOPF is fs_inopb >> fs_fragshift it must be at least 2.
Another way to look at it is that the minimum fs_fsize is 512 which would hold 2 UFS2 inodes.

[kib]

BTW, would it make sense to expand fsck or dumpfs to provide detailed human-readable reason to point out exactly which superblock sanity condition failed? I do not suggest to bloat kernel with this code, but some way to see why kernel refuses to accept a volume besides scrupulous checking of dumpfs output against each line in this function, would be useful IMO.

[mckusick]

Good idea to add reason for failure. I wonder if fsdb might be the right place to add it? We do need a simple check to determine if the block should be considered a superblock which is currently whether the magic number matches UFS1 or UFS2 so that we can look in several possible places for the superblock or so that the tasting code can check to see if it is a UFS filesystem. But all the other checks should be called out.

[pho]

The disk image fuzzer test no longer triggers a panic with this patch.
I have not detected any side effects with D35219.id106252.diff.
LGTM.

[mckusick]

Thanks for the testing Peter.

Mark Morris has confirmed that it fixes his submitted bugs:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263811
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263822
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263836
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263934
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263971