| Message ID | df41e9dc96ddad9f9e1e684e39c28f4e097e9d9b.1681917551.git.alexl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovl: Add support for fs-verity checking of lowerdata | expand |
ovl_maybe_lookup_lowerdata On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote: > > When resolving lowerdata (lazily or non-lazily) we chech the > overlay.verity xattr on the metadata inode, and if set verify that the > source lowerdata inode matches it (according to the verity options > enabled). > > Signed-off-by: Alexander Larsson <alexl@redhat.com> > --- > fs/overlayfs/namei.c | 34 ++++++++++++++ > fs/overlayfs/overlayfs.h | 6 +++ > fs/overlayfs/util.c | 97 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 137 insertions(+) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index ba2b156162ca..49f3715c582d 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry, > /* Lazy lookup of lowerdata */ > int ovl_maybe_lookup_lowerdata(struct dentry *dentry) > { > + struct ovl_fs *ofs = dentry->d_sb->s_fs_info; > struct inode *inode = d_inode(dentry); > const char *redirect = ovl_lowerdata_redirect(inode); > struct ovl_path datapath = {}; > @@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry) > if (err) > goto out_err; > > + if (ofs->config.verity_validate) { > + struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, }; > + struct path metapath = {}; > + > + ovl_path_real(dentry, &metapath); > + if (!metapath.dentry) { > + err = -EIO; > + goto out_err; > + } > + > + err = ovl_validate_verity(ofs, &metapath, &data); > + if (err) > + goto out_err; > + } > + > err = ovl_dentry_set_lowerdata(dentry, &datapath); > if (err) > goto out_err; > @@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > if (err) > goto out_put; > > + /* Validate verity of lower-data */ > + if (ofs->config.verity_validate && > + !d.is_dir && (uppermetacopy || ctr > 1)) { > + struct path datapath; > + > + ovl_entry_path_lowerdata(&oe, &datapath); > + > + /* Is NULL for lazy lookup, will be verified later */ > + if (datapath.dentry) { > + struct path metapath; > + > + ovl_entry_path_real(ofs, &oe, upperdentry, &metapath); > + err = ovl_validate_verity(ofs, &metapath, &datapath); > + if (err < 0) > + goto out_free_oe; > + } > + } > + > if (upperopaque) > ovl_dentry_set_opaque(dentry); > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index 3d14770dc711..b1d639ccd5ac 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -38,6 +38,7 @@ enum ovl_xattr { > OVL_XATTR_UPPER, > OVL_XATTR_METACOPY, > OVL_XATTR_PROTATTR, > + OVL_XATTR_VERITY, > }; > > enum ovl_inode_flag { > @@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); > int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path); > bool ovl_is_metacopy_dentry(struct dentry *dentry); > char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding); > +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, > + u8 *digest_buf, int *buf_length); > +int ovl_validate_verity(struct ovl_fs *ofs, > + struct path *metapath, > + struct path *datapath); > int ovl_sync_status(struct ovl_fs *ofs); > > static inline void ovl_set_flag(unsigned long flag, struct inode *inode) > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > index 17eff3e31239..55e90aa0978a 100644 > --- a/fs/overlayfs/util.c > +++ b/fs/overlayfs/util.c > @@ -10,7 +10,9 @@ > #include <linux/cred.h> > #include <linux/xattr.h> > #include <linux/exportfs.h> > +#include <linux/file.h> > #include <linux/fileattr.h> > +#include <linux/fsverity.h> > #include <linux/uuid.h> > #include <linux/namei.h> > #include <linux/ratelimit.h> > @@ -742,6 +744,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, > #define OVL_XATTR_UPPER_POSTFIX "upper" > #define OVL_XATTR_METACOPY_POSTFIX "metacopy" > #define OVL_XATTR_PROTATTR_POSTFIX "protattr" > +#define OVL_XATTR_VERITY_POSTFIX "verity" > > #define OVL_XATTR_TAB_ENTRY(x) \ > [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \ > @@ -756,6 +759,7 @@ const char *const ovl_xattr_table[][2] = { > OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER), > OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY), > OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR), > + OVL_XATTR_TAB_ENTRY(OVL_XATTR_VERITY), > }; > > int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry, > @@ -1188,6 +1192,99 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa > return ERR_PTR(res); > } > > +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, > + u8 *digest_buf, int *buf_length) > +{ > + int res; > + > + res = ovl_path_getxattr(ofs, path, OVL_XATTR_VERITY, digest_buf, *buf_length); > + if (res == -ENODATA || res == -EOPNOTSUPP) > + return -ENODATA; > + if (res < 0) { > + pr_warn_ratelimited("failed to get digest (%i)\n", res); > + return res; > + } > + > + *buf_length = res; > + return 0; > +} > + > +static int ovl_ensure_verity_loaded(struct ovl_fs *ofs, > + struct path *datapath) > +{ > + struct inode *inode = d_inode(datapath->dentry); > + const struct fsverity_info *vi; > + const struct cred *old_cred; > + struct file *filp; > + > + vi = fsverity_get_info(inode); > + if (vi == NULL && IS_VERITY(inode)) { > + /* > + * If this inode was not yet opened, the verity info hasn't been > + * loaded yet, so we need to do that here to force it into memory. > + */ > + old_cred = override_creds(ofs->creator_cred); Even though it may work, that's the wrong place for override_creds(), because you are calling this helper from within ovl_lookup() with mounter creds already. Better to move revert_creds() in ovl_maybe_lookup_lowerdata() to out_revert_creds: goto label and call ovl_validate_verity() with mounter creds from all call sites. > + filp = dentry_open(datapath, O_RDONLY, current_cred()); You could use open_with_fake_path() here to avoid ENFILE not sure if this is critical. > + revert_creds(old_cred); > + if (IS_ERR(filp)) > + return PTR_ERR(filp); > + fput(filp); > + } > + > + return 0; > +} > + > +int ovl_validate_verity(struct ovl_fs *ofs, > + struct path *metapath, > + struct path *datapath) > +{ > + u8 required_digest[FS_VERITY_MAX_DIGEST_SIZE]; > + u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE]; > + enum hash_algo verity_algo; > + int digest_len; > + int err; > + > + if (!ofs->config.verity_validate || > + /* Verity only works on regular files */ > + !S_ISREG(d_inode(metapath->dentry)->i_mode)) > + return 0; > + > + digest_len = sizeof(required_digest); > + err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len); > + if (err == -ENODATA) { > + if (ofs->config.verity_require) { > + pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n", > + metapath->dentry); > + return -EIO; > + } > + return 0; > + } Thinking out loud: feels to me that verity xattr is a property that is always "coupled" with metacopy xattr. I wonder if we should check and store them together during lookup. On one hand that means using a bit more memory per inode before we need it. OTOH, getxattr on metapath during lazy lookup may incur extra IO to the metapath inode xattr block that will have been amortized if done during lookup. I have no strong feelings one way or the other. Thanks, Amir. > + if (err < 0) > + return err; > + > + err = ovl_ensure_verity_loaded(ofs, datapath); > + if (err < 0) { > + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n", > + datapath->dentry); > + return -EIO; > + } > + > + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); > + if (err < 0) { > + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); > + return -EIO; > + } > + > + if (digest_len != hash_digest_size[verity_algo] || > + memcmp(required_digest, actual_digest, digest_len) != 0) { > + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", > + datapath->dentry); > + return -EIO; > + } > + > + return 0; > +} > + > /* > * ovl_sync_status() - Check fs sync status for volatile mounts > * > -- > 2.39.2 >
On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote: > + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); > + if (err < 0) { > + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); > + return -EIO; > + } > + > + if (digest_len != hash_digest_size[verity_algo] || > + memcmp(required_digest, actual_digest, digest_len) != 0) { > + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", > + datapath->dentry); > + return -EIO; > + } > + > + return 0; This is incorrect because the digest algorithm is not being compared. - Eric
On Wed, Apr 26, 2023 at 11:47 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote: > > + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); > > + if (err < 0) { > > + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); > > + return -EIO; > > + } > > + > > + if (digest_len != hash_digest_size[verity_algo] || > > + memcmp(required_digest, actual_digest, digest_len) != 0) { > > + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", > > + datapath->dentry); > > + return -EIO; > > + } > > + > > + return 0; > > This is incorrect because the digest algorithm is not being compared. This is actually an interesting question. How much are things weakened by comparing the digest size, but not comparing the digest type. Like, suppose the xattr has a sha256 digest (32 bytes), how likely is there to be another new supported verity algorithm of the same digest size where you can force it to produce matching digests? I ask because ideally we want to minimize the size of the xattrs, since they are stored for each file, and not having to specify the type for each saves space. Currently the only two supported algorithms (sha256 and sha512) are different sizes, so we essentially compare type by comparing the size. I see three options here: 1) Only compare digest + size (like now) 2) Assume size 32 means sha256, and 64 means sha512 and validate that 3) Use more space in the xattr to store an algorithm type Maybe alternative 2 is the best option on balance, less extensible, but safe and uses least space. Opinions?
On Thu, Apr 20, 2023 at 2:06 PM Amir Goldstein <amir73il@gmail.com> wrote: > > ovl_maybe_lookup_lowerdata > > On Thu, Apr 20, 2023 at 10:44 AM Alexander Larsson <alexl@redhat.com> wrote: > > > > When resolving lowerdata (lazily or non-lazily) we chech the > > overlay.verity xattr on the metadata inode, and if set verify that the > > source lowerdata inode matches it (according to the verity options > > enabled). > > > > Signed-off-by: Alexander Larsson <alexl@redhat.com> > > --- > > fs/overlayfs/namei.c | 34 ++++++++++++++ > > fs/overlayfs/overlayfs.h | 6 +++ > > fs/overlayfs/util.c | 97 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 137 insertions(+) > > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > > index ba2b156162ca..49f3715c582d 100644 > > --- a/fs/overlayfs/namei.c > > +++ b/fs/overlayfs/namei.c > > @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry, > > /* Lazy lookup of lowerdata */ > > int ovl_maybe_lookup_lowerdata(struct dentry *dentry) > > { > > + struct ovl_fs *ofs = dentry->d_sb->s_fs_info; > > struct inode *inode = d_inode(dentry); > > const char *redirect = ovl_lowerdata_redirect(inode); > > struct ovl_path datapath = {}; > > @@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry) > > if (err) > > goto out_err; > > > > + if (ofs->config.verity_validate) { > > + struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, }; > > + struct path metapath = {}; > > + > > + ovl_path_real(dentry, &metapath); > > + if (!metapath.dentry) { > > + err = -EIO; > > + goto out_err; > > + } > > + > > + err = ovl_validate_verity(ofs, &metapath, &data); > > + if (err) > > + goto out_err; > > + } > > + > > err = ovl_dentry_set_lowerdata(dentry, &datapath); > > if (err) > > goto out_err; > > @@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > > if (err) > > goto out_put; > > > > + /* Validate verity of lower-data */ > > + if (ofs->config.verity_validate && > > + !d.is_dir && (uppermetacopy || ctr > 1)) { > > + struct path datapath; > > + > > + ovl_entry_path_lowerdata(&oe, &datapath); > > + > > + /* Is NULL for lazy lookup, will be verified later */ > > + if (datapath.dentry) { > > + struct path metapath; > > + > > + ovl_entry_path_real(ofs, &oe, upperdentry, &metapath); > > + err = ovl_validate_verity(ofs, &metapath, &datapath); > > + if (err < 0) > > + goto out_free_oe; > > + } > > + } > > + > > if (upperopaque) > > ovl_dentry_set_opaque(dentry); > > > > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > > index 3d14770dc711..b1d639ccd5ac 100644 > > --- a/fs/overlayfs/overlayfs.h > > +++ b/fs/overlayfs/overlayfs.h > > @@ -38,6 +38,7 @@ enum ovl_xattr { > > OVL_XATTR_UPPER, > > OVL_XATTR_METACOPY, > > OVL_XATTR_PROTATTR, > > + OVL_XATTR_VERITY, > > }; > > > > enum ovl_inode_flag { > > @@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); > > int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path); > > bool ovl_is_metacopy_dentry(struct dentry *dentry); > > char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding); > > +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, > > + u8 *digest_buf, int *buf_length); > > +int ovl_validate_verity(struct ovl_fs *ofs, > > + struct path *metapath, > > + struct path *datapath); > > int ovl_sync_status(struct ovl_fs *ofs); > > > > static inline void ovl_set_flag(unsigned long flag, struct inode *inode) > > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > > index 17eff3e31239..55e90aa0978a 100644 > > --- a/fs/overlayfs/util.c > > +++ b/fs/overlayfs/util.c > > @@ -10,7 +10,9 @@ > > #include <linux/cred.h> > > #include <linux/xattr.h> > > #include <linux/exportfs.h> > > +#include <linux/file.h> > > #include <linux/fileattr.h> > > +#include <linux/fsverity.h> > > #include <linux/uuid.h> > > #include <linux/namei.h> > > #include <linux/ratelimit.h> > > @@ -742,6 +744,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, > > #define OVL_XATTR_UPPER_POSTFIX "upper" > > #define OVL_XATTR_METACOPY_POSTFIX "metacopy" > > #define OVL_XATTR_PROTATTR_POSTFIX "protattr" > > +#define OVL_XATTR_VERITY_POSTFIX "verity" > > > > #define OVL_XATTR_TAB_ENTRY(x) \ > > [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \ > > @@ -756,6 +759,7 @@ const char *const ovl_xattr_table[][2] = { > > OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER), > > OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY), > > OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR), > > + OVL_XATTR_TAB_ENTRY(OVL_XATTR_VERITY), > > }; > > > > int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry, > > @@ -1188,6 +1192,99 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa > > return ERR_PTR(res); > > } > > > > +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, > > + u8 *digest_buf, int *buf_length) > > +{ > > + int res; > > + > > + res = ovl_path_getxattr(ofs, path, OVL_XATTR_VERITY, digest_buf, *buf_length); > > + if (res == -ENODATA || res == -EOPNOTSUPP) > > + return -ENODATA; > > + if (res < 0) { > > + pr_warn_ratelimited("failed to get digest (%i)\n", res); > > + return res; > > + } > > + > > + *buf_length = res; > > + return 0; > > +} > > + > > +static int ovl_ensure_verity_loaded(struct ovl_fs *ofs, > > + struct path *datapath) > > +{ > > + struct inode *inode = d_inode(datapath->dentry); > > + const struct fsverity_info *vi; > > + const struct cred *old_cred; > > + struct file *filp; > > + > > + vi = fsverity_get_info(inode); > > + if (vi == NULL && IS_VERITY(inode)) { > > + /* > > + * If this inode was not yet opened, the verity info hasn't been > > + * loaded yet, so we need to do that here to force it into memory. > > + */ > > + old_cred = override_creds(ofs->creator_cred); > > Even though it may work, that's the wrong place for override_creds(), > because you are calling this helper from within ovl_lookup() with > mounter creds already. > > Better to move revert_creds() in ovl_maybe_lookup_lowerdata() > to out_revert_creds: goto label and call ovl_validate_verity() with > mounter creds from all call sites. I'll do that. Although In practice I wonder how much this matters anyway, because we're not using the result of the open, other than the side affect of the first open loading the fs-verity info into the inode. > > + filp = dentry_open(datapath, O_RDONLY, current_cred()); > > You could use open_with_fake_path() here to avoid ENFILE > not sure if this is critical. > > > + revert_creds(old_cred); > > + if (IS_ERR(filp)) > > + return PTR_ERR(filp); > > + fput(filp); > > + } > > + > > + return 0; > > +} > > + > > +int ovl_validate_verity(struct ovl_fs *ofs, > > + struct path *metapath, > > + struct path *datapath) > > +{ > > + u8 required_digest[FS_VERITY_MAX_DIGEST_SIZE]; > > + u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE]; > > + enum hash_algo verity_algo; > > + int digest_len; > > + int err; > > + > > + if (!ofs->config.verity_validate || > > + /* Verity only works on regular files */ > > + !S_ISREG(d_inode(metapath->dentry)->i_mode)) > > + return 0; > > + > > + digest_len = sizeof(required_digest); > > + err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len); > > + if (err == -ENODATA) { > > + if (ofs->config.verity_require) { > > + pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n", > > + metapath->dentry); > > + return -EIO; > > + } > > + return 0; > > + } > > Thinking out loud: feels to me that verity xattr is a property that is > always "coupled" with metacopy xattr. > > I wonder if we should check and store them together during lookup. > > On one hand that means using a bit more memory per inode > before we need it. > > OTOH, getxattr on metapath during lazy lookup may incur extra > IO to the metapath inode xattr block that will have been amortized > if done during lookup. > > I have no strong feelings one way or the other. Currently the metacopy xattrs content is not used. We only check whether it exists or not. In theory we could extend it to also store the verity digest itself. Then you only need one lookup. Having non-zero size metacopy attrs seems to be backwards compatible given current code. But, as you say, then we would need to store the digest (which is not small) for later use. We would have to increase inode size by a pointer, and then temporarily store the allocated space for the digest until lazy lookup happens. > Thanks, > Amir. > > > + if (err < 0) > > + return err; > > + > > + err = ovl_ensure_verity_loaded(ofs, datapath); > > + if (err < 0) { > > + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n", > > + datapath->dentry); > > + return -EIO; > > + } > > + > > + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); > > + if (err < 0) { > > + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); > > + return -EIO; > > + } > > + > > + if (digest_len != hash_digest_size[verity_algo] || > > + memcmp(required_digest, actual_digest, digest_len) != 0) { > > + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", > > + datapath->dentry); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > /* > > * ovl_sync_status() - Check fs sync status for volatile mounts > > * > > -- > > 2.39.2 > > >
On Thu, Apr 27, 2023 at 09:22:57AM +0200, Alexander Larsson wrote: > On Wed, Apr 26, 2023 at 11:47 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote: > > > + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); > > > + if (err < 0) { > > > + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); > > > + return -EIO; > > > + } > > > + > > > + if (digest_len != hash_digest_size[verity_algo] || > > > + memcmp(required_digest, actual_digest, digest_len) != 0) { > > > + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", > > > + datapath->dentry); > > > + return -EIO; > > > + } > > > + > > > + return 0; > > > > This is incorrect because the digest algorithm is not being compared. > > This is actually an interesting question. How much are things weakened > by comparing the digest size, but not comparing the digest type. Like, > suppose the xattr has a sha256 digest (32 bytes), how likely is there > to be another new supported verity algorithm of the same digest size > where you can force it to produce matching digests? It might actually be fairly likely, considering that whenever a system includes a choice of cryptographic algorithm, it tends to grow to include many different algorithms. Some of the reasons for this include: - Algorithms can become outdated and broken, yet systems often have to continue to support such algorithms for backwards compatibility. - People sometimes insist on using "national pride" algorithms, e.g. due to government regulations. For example, in China people can be required to use Chinese algorithms instead of the U.S. / NIST algorithms. See e.g. the existing support for SM3, SM4, Streebog, and Aria in the kernel crypto API and various other kernel subsystems. - Non-cryptographic algorithms might be added for use cases that don't actually require cryptographic security, e.g. integrity-only. I'd strongly discourage you from building something whose security critically depends on every algorithm that may ever exist being cryptographically secure. Also, two hash algorithms that are each secure individually are not necessarily secure when used as alternatives sharing the same output space. E.g. consider algorithm1 = SHA-256(data) and algorithm2 = SHA-256(data with all bits flipped). > > I ask because ideally we want to minimize the size of the xattrs, > since they are stored for each file, and not having to specify the > type for each saves space. Currently the only two supported algorithms > (sha256 and sha512) are different sizes, so we essentially compare > type by comparing the size. > > I see three options here: > 1) Only compare digest + size (like now) > 2) Assume size 32 means sha256, and 64 means sha512 and validate that > 3) Use more space in the xattr to store an algorithm type Just store the algorithm alongside the digest. It's just 1 extra byte. - Eric
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index ba2b156162ca..49f3715c582d 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -892,6 +892,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry, /* Lazy lookup of lowerdata */ int ovl_maybe_lookup_lowerdata(struct dentry *dentry) { + struct ovl_fs *ofs = dentry->d_sb->s_fs_info; struct inode *inode = d_inode(dentry); const char *redirect = ovl_lowerdata_redirect(inode); struct ovl_path datapath = {}; @@ -919,6 +920,21 @@ int ovl_maybe_lookup_lowerdata(struct dentry *dentry) if (err) goto out_err; + if (ofs->config.verity_validate) { + struct path data = { .mnt = datapath.layer->mnt, .dentry = datapath.dentry, }; + struct path metapath = {}; + + ovl_path_real(dentry, &metapath); + if (!metapath.dentry) { + err = -EIO; + goto out_err; + } + + err = ovl_validate_verity(ofs, &metapath, &data); + if (err) + goto out_err; + } + err = ovl_dentry_set_lowerdata(dentry, &datapath); if (err) goto out_err; @@ -1186,6 +1202,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, if (err) goto out_put; + /* Validate verity of lower-data */ + if (ofs->config.verity_validate && + !d.is_dir && (uppermetacopy || ctr > 1)) { + struct path datapath; + + ovl_entry_path_lowerdata(&oe, &datapath); + + /* Is NULL for lazy lookup, will be verified later */ + if (datapath.dentry) { + struct path metapath; + + ovl_entry_path_real(ofs, &oe, upperdentry, &metapath); + err = ovl_validate_verity(ofs, &metapath, &datapath); + if (err < 0) + goto out_free_oe; + } + } + if (upperopaque) ovl_dentry_set_opaque(dentry); diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 3d14770dc711..b1d639ccd5ac 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -38,6 +38,7 @@ enum ovl_xattr { OVL_XATTR_UPPER, OVL_XATTR_METACOPY, OVL_XATTR_PROTATTR, + OVL_XATTR_VERITY, }; enum ovl_inode_flag { @@ -467,6 +468,11 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path); bool ovl_is_metacopy_dentry(struct dentry *dentry); char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding); +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, + u8 *digest_buf, int *buf_length); +int ovl_validate_verity(struct ovl_fs *ofs, + struct path *metapath, + struct path *datapath); int ovl_sync_status(struct ovl_fs *ofs); static inline void ovl_set_flag(unsigned long flag, struct inode *inode) diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 17eff3e31239..55e90aa0978a 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -10,7 +10,9 @@ #include <linux/cred.h> #include <linux/xattr.h> #include <linux/exportfs.h> +#include <linux/file.h> #include <linux/fileattr.h> +#include <linux/fsverity.h> #include <linux/uuid.h> #include <linux/namei.h> #include <linux/ratelimit.h> @@ -742,6 +744,7 @@ bool ovl_path_check_dir_xattr(struct ovl_fs *ofs, const struct path *path, #define OVL_XATTR_UPPER_POSTFIX "upper" #define OVL_XATTR_METACOPY_POSTFIX "metacopy" #define OVL_XATTR_PROTATTR_POSTFIX "protattr" +#define OVL_XATTR_VERITY_POSTFIX "verity" #define OVL_XATTR_TAB_ENTRY(x) \ [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \ @@ -756,6 +759,7 @@ const char *const ovl_xattr_table[][2] = { OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER), OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY), OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTATTR), + OVL_XATTR_TAB_ENTRY(OVL_XATTR_VERITY), }; int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry, @@ -1188,6 +1192,99 @@ char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int pa return ERR_PTR(res); } +int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path, + u8 *digest_buf, int *buf_length) +{ + int res; + + res = ovl_path_getxattr(ofs, path, OVL_XATTR_VERITY, digest_buf, *buf_length); + if (res == -ENODATA || res == -EOPNOTSUPP) + return -ENODATA; + if (res < 0) { + pr_warn_ratelimited("failed to get digest (%i)\n", res); + return res; + } + + *buf_length = res; + return 0; +} + +static int ovl_ensure_verity_loaded(struct ovl_fs *ofs, + struct path *datapath) +{ + struct inode *inode = d_inode(datapath->dentry); + const struct fsverity_info *vi; + const struct cred *old_cred; + struct file *filp; + + vi = fsverity_get_info(inode); + if (vi == NULL && IS_VERITY(inode)) { + /* + * If this inode was not yet opened, the verity info hasn't been + * loaded yet, so we need to do that here to force it into memory. + */ + old_cred = override_creds(ofs->creator_cred); + filp = dentry_open(datapath, O_RDONLY, current_cred()); + revert_creds(old_cred); + if (IS_ERR(filp)) + return PTR_ERR(filp); + fput(filp); + } + + return 0; +} + +int ovl_validate_verity(struct ovl_fs *ofs, + struct path *metapath, + struct path *datapath) +{ + u8 required_digest[FS_VERITY_MAX_DIGEST_SIZE]; + u8 actual_digest[FS_VERITY_MAX_DIGEST_SIZE]; + enum hash_algo verity_algo; + int digest_len; + int err; + + if (!ofs->config.verity_validate || + /* Verity only works on regular files */ + !S_ISREG(d_inode(metapath->dentry)->i_mode)) + return 0; + + digest_len = sizeof(required_digest); + err = ovl_get_verity_xattr(ofs, metapath, required_digest, &digest_len); + if (err == -ENODATA) { + if (ofs->config.verity_require) { + pr_warn_ratelimited("metacopy file '%pd' has no overlay.verity xattr\n", + metapath->dentry); + return -EIO; + } + return 0; + } + if (err < 0) + return err; + + err = ovl_ensure_verity_loaded(ofs, datapath); + if (err < 0) { + pr_warn_ratelimited("lower file '%pd' failed to load fs-verity info\n", + datapath->dentry); + return -EIO; + } + + err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo); + if (err < 0) { + pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry); + return -EIO; + } + + if (digest_len != hash_digest_size[verity_algo] || + memcmp(required_digest, actual_digest, digest_len) != 0) { + pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n", + datapath->dentry); + return -EIO; + } + + return 0; +} + /* * ovl_sync_status() - Check fs sync status for volatile mounts *
When resolving lowerdata (lazily or non-lazily) we chech the overlay.verity xattr on the metadata inode, and if set verify that the source lowerdata inode matches it (according to the verity options enabled). Signed-off-by: Alexander Larsson <alexl@redhat.com> --- fs/overlayfs/namei.c | 34 ++++++++++++++ fs/overlayfs/overlayfs.h | 6 +++ fs/overlayfs/util.c | 97 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+)