Re: [RFC PATCH] fsverity: add enable sysctl

[Boris Burkov <boris@bur.io>]

On Mon, Sep 13, 2021 at 08:14:02PM -0700, Eric Biggers wrote:
> On Mon, Sep 13, 2021 at 05:37:15PM -0700, Boris Burkov wrote:
> > At Facebook, we would find a global killswitch sysctl reassuring while
> > rolling fs-verity out widely. i.e., we could run it in a logging mode
> > for a while, measure how it's doing, then fully enable it later.
> > 
> > However, I feel that "let root turn off verity" seems pretty sketchy, so
> > I was hoping to ask for some feedback on it.
> > 
> > I had another idea of making it per-file sort of like MODE_LOGGING in
> > dm-verity. I could add a mode to the ioctl args, and perhaps a new ioctl
> > for getting/setting the mode?
> > 
> > The rest is the commit message from the patch I originally wrote:
> > 
> > 
> > Add a sysctl killswitch for verity:
> > 0: verity has no effect, even if configured or used
> > 1: verity is in "audit" mode, only log on failures
> > 2: verity fully enabled; the behavior before this patch
> > 
> > This also precipitated re-organizing sysctls for verity as previously
> > the only sysctl was for signatures and setting up the sysctl was coupled
> > with the signature logic.
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> 
> I don't think there's any security problem with having this root-only sysctl.
> The fs.verity.require_signatures sysctl already works that way.  We aren't
> trying to protect against root, unless you've set up your system properly with
> SELinux, in which case fine-grained access control of sysctls is available.

Good to know. I couldn't quickly think of a way a root user could really
badly defeat verity (get in an evil file that would trick userspace
hiding in dm-verity) but I didn't really think about what you could do
with modules, bpf, just rebooting into a new kernel, etc..

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

> 
> Did you also consider a filesystem mount option?  I guess the sysctl makes sense
> especially since we already have the require_signatures one, but you probably
> should CC this to the filesystem mailing lists (ext4, f2fs, and btrfs) in case
> other people have opinions on this.

I didn't consider a mount option, but I'll follow up as you suggest.

> 
> - Eric

Thanks,
Boris