On Tue, Apr 25, 2023 at 1:19 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Thu, 20 Apr 2023 at 09:44, Alexander Larsson <alexl@xxxxxxxxxx> wrote:
> >
> > This adds the scaffolding (docs, config, mount options) for supporting
> > for a new overlay xattr "overlay.verity", which contains a fs-verity
> > digest. This is used for metacopy files, and the actual fs-verity
> > digest of the lowerdata file needs to match it. The mount option
> > "verity" specifies how this xattrs is handled.
> >
> > Unless you explicitly disable it ("verity=off") all existing xattrs
> > are validated before use. This is all that happens by default
> > ("verity=validate"), but, if you turn on verity ("verity=on") then
> > during metacopy we generate verity xattr in the upper metacopy file if
> > the source file has verity enabled. This means later accesses can
> > guarantee that the correct data is used.
> >
> > Additionally you can use "verity=require". In this mode all metacopy
> > files must have a valid verity xattr. For this to work metadata
> > copy-up must be able to create a verity xattr (so that later accesses
> > are validated). Therefore, in this mode, if the lower data file
> > doesn't have fs-verity enabled we fall back to a full copy rather than
> > a metacopy.
>
> Maybe we can reduce the number of modes. Which mode does your use case need?
For composefs I typically always create images with full verity info
so they *can* be used with verity, but I want to allow using these
even on systems that don't support verity. So, at the very minimum
composefs needs "verity=off" (don't even try to verify anything) and
"verity=require" (ensure all redirects have a verity xattr and it is
valid).
These are kind of extremes though, and I think it makes sense to have
something in between where not everything has to be verified.
Currently we have both "validate" and "on". But maybe those could be
consolidated.
> >
> > Actual implementation follows in a separate commit.
> >
> > Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
> > ---
> > Documentation/filesystems/overlayfs.rst | 33 +++++++++++++++++
> > fs/overlayfs/Kconfig | 14 +++++++
> > fs/overlayfs/ovl_entry.h | 4 ++
> > fs/overlayfs/super.c | 49 +++++++++++++++++++++++++
> > 4 files changed, 100 insertions(+)
> >
> > diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> > index c8e04a4f0e21..66895bf71cd1 100644
> > --- a/Documentation/filesystems/overlayfs.rst
> > +++ b/Documentation/filesystems/overlayfs.rst
> > @@ -403,6 +403,39 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect"
> > to the absolute path of the "lower data" file in the "data-only" lower layer.
> >
> >
> > +fs-verity support
> > +----------------------
> > +
> > +When metadata copy up is used for a file, then the xattr
> > +"trusted.overlay.verity" may be set on the metacopy file. This
> > +specifies the expected fs-verity digest of the lowerdata file. This
> > +may then be used to verify the content of the source file at the time
> > +the file is opened. If enabled, overlayfs can also set this xattr
> > +during metadata copy up.
> > +
> > +This is controlled by the "verity" mount option, which supports
> > +these values:
> > +
> > +- "off":
> > + The verity xattr is never used.
> > +- "validate":
> > + Whenever a metacopy files specifies an expected digest, the
> > + corresponding data file must match the specified digest.
> > +- "on":
> > + Same as validate, but additionally, when generating a metacopy
> > + file the verity xattr will be set from the source file fs-verity
> > + digest (if it has one).
> > +- "require":
> > + Same as "on", but additionally all metacopy files must specify a
> > + verity xattr. Additionally metadata copy up will only be used if
> > + the data file has fs-verity enabled, otherwise a full copy-up is
> > + used.
> > +
> > +There are two ways to tune the default behaviour. The kernel config
> > +option OVERLAY_FS_VERITY, or the module option "verity=BOOL". If
> > +either of these are enabled, then verity mode is "on" by default,
> > +otherwise it is "validate".
>
> I'm not sure that enabling verity by default is safe. E.g. a script
> mounts overalyfs but doesn't set the verity mount, since it's on by
> default. Then the script is moved to a different system where the
> default is off, which will result in verity not being enabled, even
> though that was not intended. Is there an advantage to allowing to
> change the default? I know it's done for most of the overlayfs
> options, but I think this is different.
I sort of agree, in particular because many filesystems still don't
support verity, or need it to be specifically enabled.
So, what about dropping "validate" and go with modes: "off, on,
require", where "off" is the default?
> > +
> > Sharing and copying layers
> > --------------------------
> >
> > diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
> > index 6708e54b0e30..98d6b1a7baf5 100644
> > --- a/fs/overlayfs/Kconfig
> > +++ b/fs/overlayfs/Kconfig
> > @@ -124,3 +124,17 @@ config OVERLAY_FS_METACOPY
> > that doesn't support this feature will have unexpected results.
> >
> > If unsure, say N.
> > +
> > +config OVERLAY_FS_VERITY
> > + bool "Overlayfs: turn on verity feature by default"
> > + depends on OVERLAY_FS
> > + depends on OVERLAY_FS_METACOPY
> > + help
> > + If this config option is enabled then overlay filesystems will
> > + try to copy fs-verity digests from the lower file into the
> > + metacopy file at metadata copy-up time. It is still possible
> > + to turn off this feature globally with the "verity=off"
> > + module option or on a filesystem instance basis with the
> > + "verity=off" or "verity=validate" mount option.
> > +
> > + If unsure, say N.
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index a7b1006c5321..f759e476dfc7 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -13,6 +13,10 @@ struct ovl_config {
> > bool redirect_dir;
> > bool redirect_follow;
> > const char *redirect_mode;
> > + bool verity_validate;
> > + bool verity_generate;
> > + bool verity_require;
> > + const char *verity_mode;
> > bool index;
> > bool uuid;
> > bool nfs_export;
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index ef78abc21998..953d76f6a1e3 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -59,6 +59,11 @@ module_param_named(metacopy, ovl_metacopy_def, bool, 0644);
> > MODULE_PARM_DESC(metacopy,
> > "Default to on or off for the metadata only copy up feature");
> >
> > +static bool ovl_verity_def = IS_ENABLED(CONFIG_OVERLAY_FS_VERITY);
> > +module_param_named(verity, ovl_verity_def, bool, 0644);
> > +MODULE_PARM_DESC(verity,
> > + "Default to on or validate for the metadata only copy up feature");
> > +
> > static struct dentry *ovl_d_real(struct dentry *dentry,
> > const struct inode *inode)
> > {
> > @@ -235,6 +240,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
> > kfree(ofs->config.upperdir);
> > kfree(ofs->config.workdir);
> > kfree(ofs->config.redirect_mode);
> > + kfree(ofs->config.verity_mode);
> > if (ofs->creator_cred)
> > put_cred(ofs->creator_cred);
> > kfree(ofs);
> > @@ -325,6 +331,11 @@ static const char *ovl_redirect_mode_def(void)
> > return ovl_redirect_dir_def ? "on" : "off";
> > }
> >
> > +static const char *ovl_verity_mode_def(void)
> > +{
> > + return ovl_verity_def ? "on" : "validate";
> > +}
> > +
> > static const char * const ovl_xino_str[] = {
> > "off",
> > "auto",
> > @@ -374,6 +385,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> > seq_puts(m, ",volatile");
> > if (ofs->config.userxattr)
> > seq_puts(m, ",userxattr");
> > + if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0)
> > + seq_printf(m, ",verity=%s", ofs->config.verity_mode);
> > return 0;
> > }
> >
> > @@ -429,6 +442,7 @@ enum {
> > OPT_METACOPY_ON,
> > OPT_METACOPY_OFF,
> > OPT_VOLATILE,
> > + OPT_VERITY,
> > OPT_ERR,
> > };
> >
> > @@ -451,6 +465,7 @@ static const match_table_t ovl_tokens = {
> > {OPT_METACOPY_ON, "metacopy=on"},
> > {OPT_METACOPY_OFF, "metacopy=off"},
> > {OPT_VOLATILE, "volatile"},
> > + {OPT_VERITY, "verity=%s"},
> > {OPT_ERR, NULL}
> > };
> >
> > @@ -500,6 +515,25 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode)
> > return 0;
> > }
> >
> > +static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode)
> > +{
> > + if (strcmp(mode, "validate") == 0) {
> > + config->verity_validate = true;
> > + } else if (strcmp(mode, "on") == 0) {
> > + config->verity_validate = true;
> > + config->verity_generate = true;
> > + } else if (strcmp(mode, "require") == 0) {
> > + config->verity_validate = true;
> > + config->verity_generate = true;
> > + config->verity_require = true;
> > + } else if (strcmp(mode, "off") != 0) {
> > + pr_err("bad mount option \"verity=%s\"\n", mode);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int ovl_parse_opt(char *opt, struct ovl_config *config)
> > {
> > char *p;
> > @@ -511,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> > if (!config->redirect_mode)
> > return -ENOMEM;
> >
> > + config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL);
> > + if (!config->verity_mode)
> > + return -ENOMEM;
> > +
> > while ((p = ovl_next_opt(&opt)) != NULL) {
> > int token;
> > substring_t args[MAX_OPT_ARGS];
> > @@ -611,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> > config->userxattr = true;
> > break;
> >
> > + case OPT_VERITY:
> > + kfree(config->verity_mode);
> > + config->verity_mode = match_strdup(&args[0]);
> > + if (!config->verity_mode)
> > + return -ENOMEM;
> > + break;
> > +
> > default:
> > pr_err("unrecognized mount option \"%s\" or missing value\n",
> > p);
> > @@ -642,6 +687,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> > if (err)
> > return err;
> >
> > + err = ovl_parse_verity_mode(config, config->verity_mode);
> > + if (err)
> > + return err;
> > +
> > /*
> > * This is to make the logic below simpler. It doesn't make any other
> > * difference, since config->redirect_dir is only used for upper.
> > --
> > 2.39.2
> >
>
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
