tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Proposal: validate FFS root inode during the mount.
Hi all,
During the fuzzing of FFS filesystem, we had a couple of issues caused by corrupted inode fields.
Especially there is a lot of things that can go wrong when `di_mode`, `di_size` and `di_nlink` are empty.
Example of corrupted root inode, ufs1 but ufs2 will be similar.
```
fsdb (inum: 2)> pf inode number=2 format=ufs1
command `pf inode number=2 format=ufs1
'
Disk format ufs1inode 2 block: 512
----------------------------
di_mode: 0x0 << di_nlink: 0x0 <<
di_size: 0x0 << di_atime: 0x0
di_atimensec: 0x0 di_mtime: 0x0
di_mtimensec: 0x0 di_ctime: 0x0
di_ctimensec: 0x0 di_flags: 0x0
di_blocks: 0x0 di_gen: 0x6c3122e2
di_uid: 0x0 di_gid: 0x0
di_modrev: 0x0
--- inode.di_oldids ---
...
```
Below dump of inode from freshly created FS:
```
fsdb (inum: 2)> pf inode number=2
command `pf inode number=2
'
Disk format ufs1inode 2 block: 512
----------------------------
di_mode: 0x41ed di_nlink: 0x2
di_size: 0x200 di_atime: 0x0
di_atimensec: 0x0 di_mtime: 0x0
di_mtimensec: 0x0 di_ctime: 0x0
di_ctimensec: 0x0 di_flags: 0x0
di_blocks: 0x1 di_gen: 0x68881d2c
di_uid: 0x0 di_gid: 0x0
di_modrev: 0x0
--- inode.di_oldids ---
...
```
Main issues with such corrupted root inode are:
- failing all operation where `namei` is involved, like `lstat` or `umount` (so after mounting the corrupted image user cannot umount it).
- system crashes when the block size is equal to zero on lstat or other lookup ops (i.e. `ls -alh /mnt` will cause panic).
There is couple more but I didn't study them in details.
To make sure that corrupted mount won't cause harm to the user,
I want to add function to validate root inode on mount step (after superblock validation)
Initial version that I prepared read inode from the disk and check these fields,
however, it does not add root vnode to the vcache.
Downsize is obviously additional read from the disk however I think is worth it.
I did some walk through vnode life-cycle and adding it to the vcache on mount step does not seem to impact lifecycle.
Although for now, I decided to not make things complicated,
but I will be more than happy to add this functionality if people consider that is worth it.
Any thoughts about such validation?
I attached the patch below.
Is not the final version as I need to check a couple of remount scenarios.
Thanks
Maciej
ps. More details about Fuzzing FFS:
diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
index 138b8781b60f..71e71f1a830e 100644
--- a/sys/ufs/ffs/ffs_vfsops.c
+++ b/sys/ufs/ffs/ffs_vfsops.c
@@ -92,6 +92,7 @@ __KERNEL_RCSID(0, "$NetBSD: ffs_vfsops.c,v 1.361 2019/01/01 10:06:55 hannken Exp
#include <sys/kauth.h>
#include <sys/wapbl.h>
#include <sys/module.h>
+#include <sys/stat.h>
#include <miscfs/genfs/genfs.h>
#include <miscfs/specfs/specdev.h>
@@ -115,6 +116,7 @@ MODULE(MODULE_CLASS_VFS, ffs, NULL);
static int ffs_vfs_fsync(vnode_t *, int);
static int ffs_superblock_validate(struct fs *);
+static int ffs_rootinode_validate(struct vnode *, struct fs *, struct ufsmount *);
static int ffs_is_appleufs(struct vnode *, struct fs *);
static int ffs_init_vnode(struct ufsmount *, struct vnode *, ino_t);
@@ -798,6 +800,13 @@ ffs_reload(struct mount *mp, kauth_cred_t cred, struct lwp *l)
return (EINVAL);
}
+ if (!ffs_rootinode_validate(devvp, newfs, ump)) {
+ DPRINTF("Corrupted root inode cannot mount Filesystem.");
+ DPRINTF("Please run fsck first!");
+ kmem_free(newfs, fs_sbsize);
+ return (EINVAL);
+ }
+
/*
* The current implementation doesn't handle the possibility that
* these values may have changed.
@@ -943,6 +952,58 @@ ffs_reload(struct mount *mp, kauth_cred_t cred, struct lwp *l)
*/
static const int sblock_try[] = SBLOCKSEARCH;
+int ffs_rootinode_validate(struct vnode *devvp, struct fs *fs, struct ufsmount *ump)
+{
+ struct ufs1_dinode *dp1;
+ struct ufs2_dinode *dp2;
+ struct inode ip;
+ struct buf *bp;
+ int error;
+ int result;
+ ino_t root_ino;
+
+ result = 1;
+ root_ino = UFS_ROOTINO;
+
+ /* Read the disk contents for the inode. */
+ error = bread(devvp, FFS_FSBTODB(fs, ino_to_fsba(fs, root_ino)),
+ (int)fs->fs_bsize, 0, &bp);
+ if (error) {
+ printf("CANNOT bread!!");
+ return error;
+ }
+
+ /* Initialize temporary root inode. */
+ memset(&ip, 0, sizeof(struct inode));
+ ip.i_ump = ump;
+ ip.i_fs = fs;
+ ip.i_dev = ump->um_dev;
+ ip.i_number = root_ino;
+
+ ffs_load_inode(bp, &ip, fs, root_ino);
+
+ if (fs->fs_magic == FS_UFS1_MAGIC) {
+ dp1 = ip.i_din.ffs1_din;
+
+ if (!S_ISDIR(dp1->di_mode) || !dp1->di_size || !dp1->di_blocks
+ || !dp1->di_nlink) {
+ result = 0;
+ }
+ } else { /* fs->fs_magic == FS_UFS2_MAGIC */
+ dp2 = ip.i_din.ffs2_din;
+
+ if (!S_ISDIR(dp2->di_mode) || !dp2->di_size || !dp2->di_blocks
+ || !dp2->di_nlink) {
+ result = 0;
+ }
+ }
+
+ brelse(bp, 0);
+
+ return result;
+}
+
+
static int
ffs_superblock_validate(struct fs *fs)
@@ -1234,6 +1296,12 @@ ffs_mountfs(struct vnode *devvp, struct mount *mp, struct lwp *l)
ump->um_fs = fs;
+ if (!ffs_rootinode_validate(devvp, fs, ump)) {
+ DPRINTF("Corrupted root inode cannot mount Filesystem.");
+ DPRINTF("Please run fsck first!");
+ kmem_free(fs, fs_sbsize);
+ return (EINVAL);
+ }
#ifdef WAPBL
if ((mp->mnt_wapbl_replay == 0) && (fs->fs_flags & FS_DOWAPBL)) {
error = ffs_wapbl_replay_start(mp, fs, devvp);
Home |
Main Index |
Thread Index |
Old Index