Re: [RFC PATCH] fsverity: add enable sysctl

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Sep 14, 2021 at 12:11:34PM -0700, Boris Burkov wrote:
> > 
> > The mode 0 is the one I like the least, as it makes some ad-hoc changes like
> > making the fs-verity ioctls fail with -EOPNOTSUPP.  If userspace doesn't want to
> > use those ioctls, shouldn't it just not use those ioctls?
> > 
> > It might help if you elaborated on what sort of problems you are trying to plan
> > for.  One concern that was raised on Android was that on low-end flash storage,
> > files can have bit-flips that would normally be "benign" but would cause errors
> > if fs-verity detects them.  Falling back to your mode 1 (logging-only) would be
> > sufficient if that happened and caused problems.  So I am wondering more what
> > the purpose of mode 0 would be; it seems it might be overkill, and an
> > "enforcing" boolean equivalent to your modes 1 and 2 might be sufficient?
> 
> In our situation, I think we are less worried about these sorts of
> bit-flips as we already use btrfs checksums and verity would only catch
> the cases where the checksum also changed (presumably this is only the
> malicious case, in practice)
> 
> Mode 0 is actually probably more interesting to us, as it would be
> insurance against the case where there is either a serious bug in the
> btrfs implementation or if there is a performance regression on some
> unforeseen workload. Without being able to shut it off entirely, we
> would be in a tough spot of having to replace the affected files.
> 
> The most important part of this mode to me is the skip and return 0 in
> fsverity_verify_page. I agree that failing the enables is sort of lame
> because userspace would need to be ignoring errors or falling back to
> not-verity for that to even "help".
> 
> Maybe I could make them a no-op? That could be too surprising, but is
> in line with verify being a no-op and could actually have useful
> semantics in an emergency shutoff scenario.

In that case I guess it's reasonable to have all three modes, but they need to
have clearly defined semantics and have an intuitive interface, and be
documented.  Setting "enabled" to 1 to disable something is unintuitive; it
probably should be fs.verity.mode with string values, e.g. "enforcing",
"log-only" (or "audit"?), and "disabled".

For the log-only mode, you also need to consider which types of errors it
applies to, specifically.  In your patch, it appears that only data verification
errors would be log-only, whereas other errors such as bad signatures and
fsverity_descriptor corruption would still be fatal.  It probably would make
sense to have these other errors be log-only as well, so that log-only applies
to all fs-verity errors.

I don't think the "disabled" mode making the fs-verity ioctls be no-ops is a
good idea.  I think you should just make them return an error code, preferably a
distinct error code rather than overloading EOPNOTSUPP.  You can always make
userspace aware of whether fs-verity is disabled or not, if needed.  Trying to
make userspace think that it's using fs-verity when it's actually not isn't
going to work well, especially if it's using the FS_IOC_MEASURE_VERITY ioctl, as
there is no way to return a meaningful value from that if the prior call to
FS_IOC_ENABLE_VERITY was ignored.

- Eric