Re: Proposal: validate FFS root inode during the mount.

[Mouse]

>> There are 3 popular behaviours and all can be useful.

>> -   return errors
>> -   return errors + reject further writes
>> -   panic

I can think of at least one more: forcibly unmount the filesystem
(which, like any forced unmount, means returning errors for I/O
attempts).

>> The second is Linux default behaviour and some people hate it,
>> maybe because the read-only mode is rather silent.

I don't think I would have much use for it, but that's very different
from thinking it shouldn't be available.  Ideally, I would like to see
all four of the above available - though of course whoever does the
work gets to decide what work gets done.

> 1. My biggest concern, and reason for putting this validation in
> place, is the case where we allow mount the Filesystem that later
> cannot be umounted from Userland.

> [...] umount(2) [...]

As far as I can tell there is no umount(2).  There isn't in the
versions I have easy access to (it's unmount(2)), and someone has
improved man.netbsd.org to the point where it doesn't work at all for
me so I can't check that.

Assuming that should be unmount(2), I agree that it is a bug if a
mounted filesystem cannot be unmounted at all.

> 2. other validation for root inode,

I don't think this is a bad thing; I just see it as fixing a tiny
sliver of a much larger problem.  Abstractly, I believe that the
on-disk data structures must be considered possibly-hostile external
input; I think there should be nothing the disk can hold which crashes
the system.  Obviously we're a long way from that, and I can't see
getting closer to it as a bad thing in general.  I just see this as
checking three or four values out of what, a dozen?, in one inode,
out of typically millions - I question whether the value in fixing this
tiny sliver of the problem outweighs the maintenance cost of the
special-case code it involves.  (Of course, questioning whether it does
is not the same as thinking it doesn't.)

> I [...] do not want to check entire FS on the mount because that will
> make mount process very unuseful.

No, of course not.  I doubt anyone thinks that would be a good idea.  I
would not expect that checking to happen until the inode in question
actually gets used.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: Proposal: validate FFS root inode during the mount.

[Maciej]

Apologizes for bit vague description, I will try to describe it better
We touch a couple of different topics so I will address then in order.

1. My biggest concern, and reason for putting this validation in place,
is the case where we allow mount the Filesystem that later cannot be umounted 
from Userland.

Both umount(2) and umount(8) (with or without -R) will fail as they relay on 
kernel vfs_lookup in the path resolution step.
When root inode does not have i_mode set we will end up with ENOENT due to the 
failure inside ffs_loadvnode
```
ffs_loadvnode()

if (ip->i_mode == 0) {
ffs_deinit_vnode(ump, vp);

return ENOENT;
}
```
All lookup operations end up in vcache -> VFS_LOADVNODE, so is clear that we 
won't be able to resolve the given path without loading inode from disk.

>From a user perspective, this can lead to the situations where after 
>corruption user cannot umount FS and the only way is to power off system in 
>order to umount it and repair,
which IMO is bad/unexpected behaviour.
Although i_mode should not be zero, the strange value inside i_mode can lead to 
different execution paths, because of that I want to check if the i_mode is a 
directory.

Hope people will share a similar opinion about this part.


2. other validation for root inode,

` !S_ISDIR(dp1->di_mode) || !dp1->di_size || !dp1->di_blocks || !dp1->di_nlink) 
`

Couple other cases that I found probably worth to consider for FFS root inode 
and were placed inside the patch,
Note: I DID NOT prove that incorrect values inside them case the panic during 
the mount(2) system call step, but only when another system call is issued.

di_blocks - if is equal to zero lstat operations will cause the panic, that may 
happen due to the tools on the startup. But is only MIGHT so someone may argue 
that this can happen to any other file and we clearly do not want to do a scan 
on every mount.

di_nlink - if equal to the zero and we have di_blocks or di_size non-equal to 
the zero, vput operation will cause panic inside ufs_inactive. That can happen 
due to i.e. stat systemcall, so most likely during the boot proces.

That is my justification about rest of validation which can be considered as 
pragmatic/unnecessary but as I already checked DIR mode on the root inode, 
checking these fields does not cost anything.
Also I did not check all cases and is possible that some strange combination 
can cause panic on the mount.
Another argument about this validation is the fact that is done inside 
ffs_mount which is also called by the reload which can cause panic due to the 
usage of vput.
As I said unfortunately at this moment I don't have particular crash example, 
so I understand people concerns.

3. "Kirk has added a bunch of checksum and other integrity checks to FreeBSD$"

 Thank you for the info I will spend some time and study what is validated 
there.

4. Validation on the mount time: I don't have a strong opinion about the 
preferred behaviour and also do not want to check entire FS on the mount 
because that will make mount process very unuseful.

To summarize:
I consider point 1 (umount issue) as a logical bug fix, and 2 as nice to have 
(or ew possible future fix).

‐‐‐ Original Message ‐‐‐
On Wednesday, November 20, 2019 8:27 PM,  wrote:

> mo...@rodents-montreal.org (Mouse) writes:
>
> > But for the machine's own filesystems? Corruption should panic:
> > mount -o onerror=panic /dev/wd2a /builds
>
> There are 3 popular behaviours and all can be useful.
>
> -   return errors
> -   return errors + reject further writes
> -   panic
>
> The second is Linux default behaviour and some people hate it,
> maybe because the read-only mode is rather silent. If you do
> react to it, it can be more graceful than a panic.
>
> --
> --
> Michael van Elst
> Internet: mlel...@serpens.de
> "A potential Snark may lurk in every tree."
>

Re: Proposal: validate FFS root inode during the mount.

[Michael van Elst]

mo...@rodents-montreal.org (Mouse) writes:

>But for the machine's own filesystems?  Corruption should panic:
>   mount -o onerror=panic /dev/wd2a /builds

There are 3 popular behaviours and all can be useful.
- return errors
- return errors + reject further writes
- panic

The second is Linux default behaviour and some people hate it,
maybe because the read-only mode is rather silent. If you do
react to it, it can be more graceful than a panic.

-- 
-- 
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."

Re: Proposal: validate FFS root inode during the mount.

[Mouse]

>> Rejecting won't help much, there are so many other parts that may be
>> corrupt that you cannot validate on mount.
> For start we want to stop the kernel from crashing on mount.

So you'd rather have it crash at some unpredictable time after mount?
Okay, that's mostly snark, but there's a serious point lurking.

>> The goal should be to gracefully handle corrupted data structures by
>> returning errors instead of crashing the kernel.
> mbouyer@ wants to panic always, after a successful mount.

And, sometimes, I think that's the rightest choice.  But I also would
like to be able to get errors instead of panics.

Want to pull something off a thumbdrive?  I'd rather have an error just
forcibly unmount the filesystem and flush everything using it:

mount -o onerror=unmount /dev/sd1e /mnt

But for the machine's own filesystems?  Corruption should panic:

mount -o onerror=panic /dev/wd2a /builds

Of course, actually making that work, well, I don't have any
suggestions for cat-bellers.  Unless and until I have a significant
amount of spare time, it's all just "it might be nice if".

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: Proposal: validate FFS root inode during the mount.

[Kamil Rytarowski]

On 20.11.2019 18:14, Michael van Elst wrote:
> n...@gmx.com (Kamil Rytarowski) writes:
> 
>> =46rom a high level point of view, we want to reject early corrupted FS o=
>> n
>> a mount. Today we panic the kernel needlessly.
> 
> 
> Rejecting won't help much, there are so many other parts that may be corrupt
> that you cannot validate on mount.
> 

For start we want to stop the kernel from crashing on mount.

> The goal should be to gracefully handle corrupted data structures by returning
> errors instead of crashing the kernel.
> 

mbouyer@ wants to panic always, after a successful mount.

I have no strong opinion except handling the corrupted data either with
a panic or some error returned from the kernel.



signature.asc
Description: OpenPGP digital signature

Re: Proposal: validate FFS root inode during the mount.

[Michael van Elst]

n...@gmx.com (Kamil Rytarowski) writes:

>=46rom a high level point of view, we want to reject early corrupted FS o=
>n
>a mount. Today we panic the kernel needlessly.


Rejecting won't help much, there are so many other parts that may be corrupt
that you cannot validate on mount.

The goal should be to gracefully handle corrupted data structures by returning
errors instead of crashing the kernel.

-- 
-- 
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."

Re: Proposal: validate FFS root inode during the mount.

[Warner Losh]

Kirk has added a bunch of checksum and other integrity checks to FreeBSD.
If you are looking for extra sanity, that might be a good place to snag
code from. They would be more comprehensive than just checking the root
node...

Warner

On Wed, Nov 20, 2019, 8:39 AM Mouse  wrote:

> >>> To make sure that corrupted mount won't cause harm to the user, I
> >>> want to add function to validate root inode on mount [...]
> >> Don't you have more or less the same issue with every other non-free
> >> inode in the filesystem?
> > I think the point is, when the root inode is corrupted, you can't
> > unmount then filesystem.
>
> If that were the problem, I'd expect the fix to be support for forcibly
> unmounting filesystems even when they're in bizarre states like that.
> Arguably that is something that should go in anyway.  I long ago added
> a flag to umount(8)
>
>  -R  Take the special | node argument as a path to be passed
> directly
>  to unmount(2), bypassing all attempts to be smart about
> mechani-
>  cally determining the correct path from the argument.  This
>  option is incompatible with any option that potentially
> unmounts
>  more than one filesystem, such as -a, but it can be used with
> -f
>  and/or -v.  This is the only way to unmount something that
> does
>  not appear as a directory (such as a nullfs mount of a plain
>  file); there are probably other cases where it is necessary.
>
> Could that be suitable for dealing with the "can't unmount" aspect, or
> is there kernel work needed too?  The initial post indicates that there
> is crasher behaviour involved, though it's not clear to what extent
> it's directly related to the "can't unmount" syndrome - the post says
> it can't be unmounted, but blames umount, not unmount, so it's not
> clear to me whether that's userland's fault or not.
>
> /~\ The ASCII Mouse
> \ / Ribbon Campaign
>  X  Against HTMLmo...@rodents-montreal.org
> / \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B
>

Re: Proposal: validate FFS root inode during the mount.

[Mouse]

>>> To make sure that corrupted mount won't cause harm to the user, I
>>> want to add function to validate root inode on mount [...]
>> Don't you have more or less the same issue with every other non-free
>> inode in the filesystem?
> I think the point is, when the root inode is corrupted, you can't
> unmount then filesystem.

If that were the problem, I'd expect the fix to be support for forcibly
unmounting filesystems even when they're in bizarre states like that.
Arguably that is something that should go in anyway.  I long ago added
a flag to umount(8)

 -R  Take the special | node argument as a path to be passed directly
 to unmount(2), bypassing all attempts to be smart about mechani-
 cally determining the correct path from the argument.  This
 option is incompatible with any option that potentially unmounts
 more than one filesystem, such as -a, but it can be used with -f
 and/or -v.  This is the only way to unmount something that does
 not appear as a directory (such as a nullfs mount of a plain
 file); there are probably other cases where it is necessary.

Could that be suitable for dealing with the "can't unmount" aspect, or
is there kernel work needed too?  The initial post indicates that there
is crasher behaviour involved, though it's not clear to what extent
it's directly related to the "can't unmount" syndrome - the post says
it can't be unmounted, but blames umount, not unmount, so it's not
clear to me whether that's userland's fault or not.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: Proposal: validate FFS root inode during the mount.

[Manuel Bouyer]

On Wed, Nov 20, 2019 at 10:11:14AM -0500, Mouse wrote:
> > During the fuzzing of FFS filesystem, we had a couple of issues
> > caused by corrupted inode fields.  [...]
> 
> > 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)
> 
> Don't you have more or less the same issue with every other non-free
> inode in the filesystem?  The only thing I can see that's special about
> the root inode in this regard is that it is the only inode that is used
> immediately upon mount.

I think the point is, when the root inode is corrupted, you can't unmount
then filesystem.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--

Re: Proposal: validate FFS root inode during the mount.

[Kamil Rytarowski]

On 20.11.2019 16:11, Mouse wrote:
>> During the fuzzing of FFS filesystem, we had a couple of issues
>> caused by corrupted inode fields.  [...]
> 
>> 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)
> 
> Don't you have more or less the same issue with every other non-free
> inode in the filesystem?  The only thing I can see that's special about
> the root inode in this regard is that it is the only inode that is used
> immediately upon mount.
> 

From a high level point of view, we want to reject early corrupted FS on
a mount. Today we panic the kernel needlessly.



signature.asc
Description: OpenPGP digital signature

Re: Proposal: validate FFS root inode during the mount.

[Mouse]

> During the fuzzing of FFS filesystem, we had a couple of issues
> caused by corrupted inode fields.  [...]

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

Don't you have more or less the same issue with every other non-free
inode in the filesystem?  The only thing I can see that's special about
the root inode in this regard is that it is the only inode that is used
immediately upon mount.

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Proposal: validate FFS root inode during the mount.

[Maciej]

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  < 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:
https://blog.netbsd.org/tnf/entry/write_your_own_fuzzer_for
https://blog.netbsd.org/tnf/entry/fuzzing_netbsd_filesystems_via_afldiff --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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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, );
+	if (error) {
+		printf("CANNOT bread!!");
+		return error;
+	}
+
+	/* Initialize temporary root inode. */
+	memset(, 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, , 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
+||