Page MenuHomeFreeBSD

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

Authored by mckusick on May 16 2022, 12:06 AM.
Referenced Files
F62387567: D35219.id106030.diff
Tue, Jun 6, 6:08 AM
Unknown Object (File)
Tue, May 30, 6:16 PM
Unknown Object (File)
Mon, May 29, 1:49 PM
Unknown Object (File)
Thu, May 18, 3:29 AM
Unknown Object (File)
Wed, May 17, 6:26 AM
Unknown Object (File)
Wed, May 17, 6:25 AM
Unknown Object (File)
Mar 12 2023, 10:31 PM
Unknown Object (File)
Feb 27 2023, 12:01 PM



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 (

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.

Diff Detail

rG FreeBSD src repository
Lint Not Applicable
Tests Not Applicable

Event Timeline

mckusick created this revision.

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


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.


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

mckusick marked an inline comment as done.

Changes suggested by kib and refinements suggested by Robert Morris

Still need to track down problem cited by Peter Holm.


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?


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()


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;

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

mckusick marked an inline comment as done.

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.

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


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.


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

mckusick added inline comments.

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.

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.

This revision is now accepted and ready to land.May 22 2022, 1:32 PM

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.

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