| Message ID | ebc9c81c31119e0ce8f810c5729b42eef4c5c3af.1631560857.git.boris@bur.io (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] fsverity: add enable sysctl | expand |
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. 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? 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. - Eric
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
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
diff --git a/fs/verity/Makefile b/fs/verity/Makefile index 435559a4fa9e..81a468ca0131 100644 --- a/fs/verity/Makefile +++ b/fs/verity/Makefile @@ -9,3 +9,5 @@ obj-$(CONFIG_FS_VERITY) += enable.o \ verify.o obj-$(CONFIG_FS_VERITY_BUILTIN_SIGNATURES) += signature.o + +obj-$(CONFIG_SYSCTL) += sysctl.o diff --git a/fs/verity/enable.c b/fs/verity/enable.c index 77e159a0346b..f7d2e1ed4b69 100644 --- a/fs/verity/enable.c +++ b/fs/verity/enable.c @@ -14,6 +14,8 @@ #include <linux/sched/signal.h> #include <linux/uaccess.h> +extern int fsverity_enable; + /* * Read a file data page for Merkle tree construction. Do aggressive readahead, * since we're sequentially reading the entire file. @@ -343,6 +345,9 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *uarg) struct fsverity_enable_arg arg; int err; + if (!fsverity_enable) + return -EOPNOTSUPP; + if (copy_from_user(&arg, uarg, sizeof(arg))) return -EFAULT; diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h index a7920434bae5..43604b80a562 100644 --- a/fs/verity/fsverity_private.h +++ b/fs/verity/fsverity_private.h @@ -136,6 +136,21 @@ int fsverity_get_descriptor(struct inode *inode, int __init fsverity_init_info_cache(void); void __init fsverity_exit_info_cache(void); +/* sysctl.c */ +#ifdef CONFIG_SYSCTL +int __init fsverity_sysctl_init(void); +void __init fsverity_exit_sysctl(void); +#else /* !CONFIG_SYSCTL */ +static inline int __init fsverity_sysctl_init(void) +{ + return 0; +} +static inline void __init fsverity_exit_sysctl(void) +{ + return; +} +#endif /* !CONFIG_SYSCTL */ + /* signature.c */ #ifdef CONFIG_FS_VERITY_BUILTIN_SIGNATURES diff --git a/fs/verity/init.c b/fs/verity/init.c index c98b7016f446..bd16495e8adf 100644 --- a/fs/verity/init.c +++ b/fs/verity/init.c @@ -45,13 +45,18 @@ static int __init fsverity_init(void) if (err) goto err_exit_info_cache; - err = fsverity_init_signature(); + err = fsverity_sysctl_init(); if (err) goto err_exit_workqueue; + err = fsverity_init_signature(); + if (err) + goto err_exit_sysctl; pr_debug("Initialized fs-verity\n"); return 0; +err_exit_sysctl: + fsverity_exit_sysctl(); err_exit_workqueue: fsverity_exit_workqueue(); err_exit_info_cache: diff --git a/fs/verity/measure.c b/fs/verity/measure.c index f0d7b30c62db..789d0a8672f9 100644 --- a/fs/verity/measure.c +++ b/fs/verity/measure.c @@ -9,6 +9,8 @@ #include <linux/uaccess.h> +extern int fsverity_enable; + /** * fsverity_ioctl_measure() - get a verity file's digest * @filp: file to get digest of @@ -28,6 +30,9 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg) const struct fsverity_hash_alg *hash_alg; struct fsverity_digest arg; + if (!fsverity_enable) + return -EOPNOTSUPP; + vi = fsverity_get_info(inode); if (!vi) return -ENODATA; /* not a verity file */ diff --git a/fs/verity/open.c b/fs/verity/open.c index 60ff8af7219f..8230def90017 100644 --- a/fs/verity/open.c +++ b/fs/verity/open.c @@ -10,6 +10,7 @@ #include <linux/slab.h> static struct kmem_cache *fsverity_info_cachep; +extern int fsverity_enable; /** * fsverity_init_merkle_tree_params() - initialize Merkle tree parameters @@ -344,7 +345,7 @@ static int ensure_verity_info(struct inode *inode) */ int fsverity_file_open(struct inode *inode, struct file *filp) { - if (!IS_VERITY(inode)) + if (!IS_VERITY(inode) || !fsverity_enable) return 0; if (filp->f_mode & FMODE_WRITE) { diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c index 7e2d0c7bdf0d..d262e26086f5 100644 --- a/fs/verity/read_metadata.c +++ b/fs/verity/read_metadata.c @@ -12,6 +12,8 @@ #include <linux/sched/signal.h> #include <linux/uaccess.h> +extern int fsverity_enable; + static int fsverity_read_merkle_tree(struct inode *inode, const struct fsverity_info *vi, void __user *buf, u64 offset, int length) @@ -157,6 +159,9 @@ int fsverity_ioctl_read_metadata(struct file *filp, const void __user *uarg) int length; void __user *buf; + if (!fsverity_enable) + return -EOPNOTSUPP; + vi = fsverity_get_info(inode); if (!vi) return -ENODATA; /* not a verity file */ diff --git a/fs/verity/signature.c b/fs/verity/signature.c index 143a530a8008..2b1280c66d21 100644 --- a/fs/verity/signature.c +++ b/fs/verity/signature.c @@ -12,11 +12,7 @@ #include <linux/slab.h> #include <linux/verification.h> -/* - * /proc/sys/fs/verity/require_signatures - * If 1, all verity files must have a valid builtin signature. - */ -static int fsverity_require_signatures; +extern int fsverity_require_signatures; /* * Keyring that contains the trusted X.509 certificates. @@ -87,45 +83,6 @@ int fsverity_verify_signature(const struct fsverity_info *vi, return 0; } -#ifdef CONFIG_SYSCTL -static struct ctl_table_header *fsverity_sysctl_header; - -static const struct ctl_path fsverity_sysctl_path[] = { - { .procname = "fs", }, - { .procname = "verity", }, - { } -}; - -static struct ctl_table fsverity_sysctl_table[] = { - { - .procname = "require_signatures", - .data = &fsverity_require_signatures, - .maxlen = sizeof(int), - .mode = 0644, - .proc_handler = proc_dointvec_minmax, - .extra1 = SYSCTL_ZERO, - .extra2 = SYSCTL_ONE, - }, - { } -}; - -static int __init fsverity_sysctl_init(void) -{ - fsverity_sysctl_header = register_sysctl_paths(fsverity_sysctl_path, - fsverity_sysctl_table); - if (!fsverity_sysctl_header) { - pr_err("sysctl registration failed!\n"); - return -ENOMEM; - } - return 0; -} -#else /* !CONFIG_SYSCTL */ -static inline int __init fsverity_sysctl_init(void) -{ - return 0; -} -#endif /* !CONFIG_SYSCTL */ - int __init fsverity_init_signature(void) { struct key *ring; @@ -139,14 +96,6 @@ int __init fsverity_init_signature(void) if (IS_ERR(ring)) return PTR_ERR(ring); - err = fsverity_sysctl_init(); - if (err) - goto err_put_ring; - fsverity_keyring = ring; return 0; - -err_put_ring: - key_put(ring); - return err; } diff --git a/fs/verity/sysctl.c b/fs/verity/sysctl.c new file mode 100644 index 000000000000..6ff1ae34931e --- /dev/null +++ b/fs/verity/sysctl.c @@ -0,0 +1,69 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include "fsverity_private.h" + +#include <linux/sysctl.h> + +static int two = 2; +/* + * /proc/sys/fs/verity/require_signatures + * If 1, all verity files must have a valid builtin signature. + */ +int fsverity_require_signatures; +/* + * /proc/sys/fs/verity/enable + * If 0, disable verity, don't verify, ignore enable ioctl. + * If 1, allow enabling verity, but only log on verification failure. + * If 2, fully enable. + * default: 2 + */ +int fsverity_enable = 2; + +#ifdef CONFIG_SYSCTL +static struct ctl_table_header *fsverity_sysctl_header; + +static const struct ctl_path fsverity_sysctl_path[] = { + { .procname = "fs", }, + { .procname = "verity", }, + { } +}; + +static struct ctl_table fsverity_sysctl_table[] = { + { + .procname = "require_signatures", + .data = &fsverity_require_signatures, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = SYSCTL_ONE, + }, + { + .procname = "enable", + .data = &fsverity_enable, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = &two, + }, + { } +}; + +int __init fsverity_sysctl_init(void) +{ + fsverity_sysctl_header = register_sysctl_paths(fsverity_sysctl_path, + fsverity_sysctl_table); + if (!fsverity_sysctl_header) { + pr_err("sysctl registration failed!\n"); + return -ENOMEM; + } + return 0; +} + +void __init fsverity_exit_sysctl(void) +{ + unregister_sysctl_table(fsverity_sysctl_header); + fsverity_sysctl_header = NULL; +} +#endif /* !CONFIG_SYSCTL */ diff --git a/fs/verity/verify.c b/fs/verity/verify.c index 0adb970f4e73..0a52e4c16038 100644 --- a/fs/verity/verify.c +++ b/fs/verity/verify.c @@ -12,6 +12,7 @@ #include <linux/ratelimit.h> static struct workqueue_struct *fsverity_read_workqueue; +extern int fsverity_enable; /** * hash_at_level() - compute the location of the block's hash at the given level @@ -98,8 +99,10 @@ static bool verify_page(struct inode *inode, const struct fsverity_info *vi, unsigned int hoffsets[FS_VERITY_MAX_LEVELS]; int err; - if (WARN_ON_ONCE(!PageLocked(data_page) || PageUptodate(data_page))) - return false; + if (WARN_ON_ONCE(!PageLocked(data_page) || PageUptodate(data_page))) { + err = -EINVAL; + goto out; + } pr_debug_ratelimited("Verifying data page %lu...\n", index); @@ -174,6 +177,8 @@ static bool verify_page(struct inode *inode, const struct fsverity_info *vi, for (; level > 0; level--) put_page(hpages[level - 1]); + if (fsverity_enable == 1) + err = 0; return err == 0; } @@ -193,6 +198,8 @@ bool fsverity_verify_page(struct page *page) struct ahash_request *req; bool valid; + if (!fsverity_enable) + return true; /* This allocation never fails, since it's mempool-backed. */ req = fsverity_alloc_hash_request(vi->tree_params.hash_alg, GFP_NOFS);
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> --- fs/verity/Makefile | 2 ++ fs/verity/enable.c | 5 +++ fs/verity/fsverity_private.h | 15 ++++++++ fs/verity/init.c | 7 +++- fs/verity/measure.c | 5 +++ fs/verity/open.c | 3 +- fs/verity/read_metadata.c | 5 +++ fs/verity/signature.c | 53 +-------------------------- fs/verity/sysctl.c | 69 ++++++++++++++++++++++++++++++++++++ fs/verity/verify.c | 11 ++++-- 10 files changed, 119 insertions(+), 56 deletions(-) create mode 100644 fs/verity/sysctl.c