`struct fs` contains some pointer fields. The binary representation of a `struct fs` is written out verbatim to the disk: see ffs_sbput() in sys/ufs/ffs/ffs_subr.c. This inadvertently causes garbage to be stored, in particular by newfs(1). (It is garbage because those pointers pointed to the then-current stack or heap.) Although a mere pointer doesn't reveal anything useful (like a part of a private key) to an attacker, garbage output deteriorates reproducibility. In an attempt to work around this design, users of sbput(3) ought to zero out those pointer fields, but this isn't totally possible, because the function makes use of the `fs_csp` field. So the solution should be to save the pointers before writing the whole struct out, and to restore them afterwards. It would have been ideal if `struct fs` didn't contain any pointers at all.
In CheriBSD's cheri-purecap-kernel branch (https://github.com/CTSRD-CHERI/cheribsd/tree/cheri-purecap-kernel) we have a modification that reduces this to a single pointer to a structure holding the other pointers. In our case this is because the in-kernel use includes too many pointers to fit when you blow them up to 128-bit. This doesn't address the leakage issues, but might be a place to start from.
Created attachment 215239 [details] in ffs_sbput(), clear the pointers for the duration of writing
(In reply to goatshit54108 from comment #2) This won't apply to HEAD, where we additionally compute a checksum to store with the superblock. I suspect a better solution might be to replace these pointer fields with spares, always, zero, and move the pointers into struct ufsmount.
(In reply to Mark Johnston from comment #3) Although the ufsmount structure has a pointer to the fs structure, the inverse is not available. So, if we moved some of the fs_ pointers to the ufsmount structure, simply having the fs pointer would not be sufficient to get to the ufsmount fields (i.e., we could not have a macro to hide the move, we would have to go to every place they were used and figure out a way to find the ufsmount structure).
(In reply to Brooks Davis from comment #1) I like the idea of reducing the number of pointers in the superblock to just one. I will look at adopting your CHERI changes.
(In reply to goatshit54108 from comment #2) I concur with your proposed changes. I am first going to bring in the CHERI changes to reduce the pointers in the superblock to one. I will then NULL out that pointer when writing the superblock along the lines that you propose.
A commit references this bug: Author: mckusick Date: Fri Jun 19 01:02:55 UTC 2020 New revision: 362358 URL: https://svnweb.freebsd.org/changeset/base/362358 Log: Move the pointers stored in the superblock into a separate fs_summary_info structure. This change was originally done by the CheriBSD project as they need larger pointers that do not fit in the existing superblock. This cleanup of the superblock eases the task of the commit that immediately follows this one. Suggested by: brooks Reviewed by: kib PR: 246983 Sponsored by: Netflix Changes: head/sbin/newfs/mkfs.c head/stand/libsa/ufs.c head/sys/geom/journal/g_journal_ufs.c head/sys/geom/label/g_label_ufs.c head/sys/ufs/ffs/ffs_snapshot.c head/sys/ufs/ffs/ffs_subr.c head/sys/ufs/ffs/ffs_vfsops.c head/sys/ufs/ffs/fs.h head/usr.sbin/fstyp/ufs.c head/usr.sbin/quot/quot.c
A commit references this bug: Author: mckusick Date: Fri Jun 19 01:04:26 UTC 2020 New revision: 362359 URL: https://svnweb.freebsd.org/changeset/base/362359 Log: The binary representation of the superblock (the fs structure) is written out verbatim to the disk: see ffs_sbput() in sys/ufs/ffs/ffs_subr.c. It contains a pointer to the fs_summary_info structure. This pointer value inadvertently causes garbage to be stored. It is garbage because the pointer to the fs_summary_info structure is the address the then current stack or heap. Although a mere pointer does not reveal anything useful (like a part of a private key) to an attacker, garbage output deteriorates reproducibility. This commit zeros out the pointer to the fs_summary_info structure before writing the out the superblock. Reviewed by: kib Tested by: Peter Holm PR: 246983 Sponsored by: Netflix Changes: head/sys/ufs/ffs/ffs_subr.c head/sys/ufs/ffs/ffs_vfsops.c
Fixed by two commits noted below.
Several other issues have been raised by the submitter.
The following four commits were made (and accidentally cited bug 247425): -r362558 Clean up comments in libufs struct uufsd. -r362559 Make libufs track and clean up fs_si structure. -r362560 Correctly describe libufs library sbget() and sbput() return values. -r362561 Avoid writing superblock summary info in g_journal. With these changes, this bug can now be closed.
A commit references this bug: Author: mckusick Date: Sat Aug 15 21:40:37 UTC 2020 New revision: 364262 URL: https://svnweb.freebsd.org/changeset/base/364262 Log: Use the sbput() function to write alternate superblocks so that they get a checkhash. PR: 246983 Sponsored by: Netflix Changes: head/sbin/fsck_ffs/main.c