| Message ID | b7a2dfb80e35dda04edd942ad715dc88b784c218.1687345663.git.alexl@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ovl: Add support for fs-verity checking of lowerdata | expand |
On Wed, Jun 21, 2023 at 2:18 PM Alexander Larsson <alexl@redhat.com> wrote: > > Historically overlay.metacopy was a zero-size xattr, and it's > existence marked a metacopy file. This change adds a versioned header > with a flag field, a length and a digest. The initial use-case of this > will be for validating a fs-verity digest, but the flags field could > also be used later for other new features. > > ovl_check_metacopy_xattr() now returns the size of the xattr, > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to > distinguish it from the no-xattr case. > > Signed-off-by: Alexander Larsson <alexl@redhat.com> Looks good. Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/namei.c | 10 +++++----- > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++- > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++---- > 3 files changed, 61 insertions(+), 10 deletions(-) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 57adf911735f..3dd480253710 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -25,7 +25,7 @@ struct ovl_lookup_data { > bool stop; > bool last; > char *redirect; > - bool metacopy; > + int metacopy; > /* Referring to last redirect xattr */ > bool absolute_redirect; > }; > @@ -270,7 +270,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d, > d->stop = true; > goto put_and_out; > } > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path); > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL); > if (err < 0) > goto out_err; > > @@ -963,7 +963,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > .stop = false, > .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe), > .redirect = NULL, > - .metacopy = false, > + .metacopy = 0, > }; > > if (dentry->d_name.len > ofs->namelen) > @@ -1120,7 +1120,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > > /* Defer lookup of lowerdata in data-only layers to first access */ > if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) { > - d.metacopy = false; > + d.metacopy = 0; > ctr++; > } > > @@ -1211,7 +1211,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > upperredirect = NULL; > goto out_free_oe; > } > - err = ovl_check_metacopy_xattr(ofs, &upperpath); > + err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL); > if (err < 0) > goto out_free_oe; > uppermetacopy = err; > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index cf92a9aaf934..6d4e08df0dfe 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -7,6 +7,7 @@ > #include <linux/kernel.h> > #include <linux/uuid.h> > #include <linux/fs.h> > +#include <linux/fsverity.h> > #include <linux/namei.h> > #include <linux/posix_acl.h> > #include <linux/posix_acl_xattr.h> > @@ -140,6 +141,26 @@ struct ovl_fh { > #define OVL_FH_FID_OFFSET (OVL_FH_WIRE_OFFSET + \ > offsetof(struct ovl_fb, fid)) > > +/* On-disk format for "metacopy" xattr (if non-zero size) */ > +struct ovl_metacopy { > + u8 version; /* 0 */ > + u8 len; /* size of this header + used digest bytes */ > + u8 flags; > + u8 digest_algo; /* FS_VERITY_HASH_ALG_* constant, 0 for no digest */ > + u8 digest[FS_VERITY_MAX_DIGEST_SIZE]; /* Only the used part on disk */ > +} __packed; > + > +#define OVL_METACOPY_MAX_SIZE (sizeof(struct ovl_metacopy)) > +#define OVL_METACOPY_MIN_SIZE (OVL_METACOPY_MAX_SIZE - FS_VERITY_MAX_DIGEST_SIZE) > +#define OVL_METACOPY_INIT { 0, OVL_METACOPY_MIN_SIZE } > + > +static inline int ovl_metadata_digest_size(const struct ovl_metacopy *metacopy) > +{ > + if (metacopy->len < OVL_METACOPY_MIN_SIZE) > + return 0; > + return (int)metacopy->len - OVL_METACOPY_MIN_SIZE; > +} > + > extern const char *const ovl_xattr_table[][2]; > static inline const char *ovl_xattr(struct ovl_fs *ofs, enum ovl_xattr ox) > { > @@ -490,7 +511,8 @@ bool ovl_need_index(struct dentry *dentry); > int ovl_nlink_start(struct dentry *dentry); > void ovl_nlink_end(struct dentry *dentry); > int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); > -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path); > +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path, > + struct ovl_metacopy *data); > 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_sync_status(struct ovl_fs *ofs); > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c > index 7ef9e13c404a..921747223991 100644 > --- a/fs/overlayfs/util.c > +++ b/fs/overlayfs/util.c > @@ -1054,8 +1054,12 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir) > return -EIO; > } > > -/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */ > -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) > +/* > + * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found. > + * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value. > + */ > +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path, > + struct ovl_metacopy *data) > { > int res; > > @@ -1063,7 +1067,8 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) > if (!S_ISREG(d_inode(path->dentry)->i_mode)) > return 0; > > - res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, NULL, 0); > + res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, > + data, data ? OVL_METACOPY_MAX_SIZE : 0); > if (res < 0) { > if (res == -ENODATA || res == -EOPNOTSUPP) > return 0; > @@ -1077,7 +1082,31 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) > goto out; > } > > - return 1; > + if (res == 0) { > + /* Emulate empty data for zero size metacopy xattr */ > + res = OVL_METACOPY_MIN_SIZE; > + if (data) { > + memset(data, 0, res); > + data->len = res; > + } > + } else if (res < OVL_METACOPY_MIN_SIZE) { > + pr_warn_ratelimited("metacopy file '%pd' has too small xattr\n", > + path->dentry); > + return -EIO; > + } else if (data) { > + if (data->version != 0) { > + pr_warn_ratelimited("metacopy file '%pd' has unsupported version\n", > + path->dentry); > + return -EIO; > + } > + if (res != data->len) { > + pr_warn_ratelimited("metacopy file '%pd' has invalid xattr size\n", > + path->dentry); > + return -EIO; > + } > + } > + > + return res; > out: > pr_warn_ratelimited("failed to get metacopy (%i)\n", res); > return res; > -- > 2.40.1 >
On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote: > Historically overlay.metacopy was a zero-size xattr, and it's > existence marked a metacopy file. This change adds a versioned header > with a flag field, a length and a digest. The initial use-case of this > will be for validating a fs-verity digest, but the flags field could > also be used later for other new features. > > ovl_check_metacopy_xattr() now returns the size of the xattr, > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to > distinguish it from the no-xattr case. > > Signed-off-by: Alexander Larsson <alexl@redhat.com> > --- > fs/overlayfs/namei.c | 10 +++++----- > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++- > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++---- > 3 files changed, 61 insertions(+), 10 deletions(-) > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index 57adf911735f..3dd480253710 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -25,7 +25,7 @@ struct ovl_lookup_data { > bool stop; > bool last; > char *redirect; > - bool metacopy; > + int metacopy; Should this be called 'metacopy_size' now? > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path); > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL); > if (err < 0) > goto out_err; This part is confusing because variables named 'err' conventionally contain only 0 or a negative errno value. But this patch makes it possible for ovl_check_metacopy_xattr() to return a positive size. - Eric
On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote: > > Historically overlay.metacopy was a zero-size xattr, and it's > > existence marked a metacopy file. This change adds a versioned header > > with a flag field, a length and a digest. The initial use-case of this > > will be for validating a fs-verity digest, but the flags field could > > also be used later for other new features. > > > > ovl_check_metacopy_xattr() now returns the size of the xattr, > > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to > > distinguish it from the no-xattr case. > > > > Signed-off-by: Alexander Larsson <alexl@redhat.com> > > --- > > fs/overlayfs/namei.c | 10 +++++----- > > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++- > > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++---- > > 3 files changed, 61 insertions(+), 10 deletions(-) > > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > > index 57adf911735f..3dd480253710 100644 > > --- a/fs/overlayfs/namei.c > > +++ b/fs/overlayfs/namei.c > > @@ -25,7 +25,7 @@ struct ovl_lookup_data { > > bool stop; > > bool last; > > char *redirect; > > - bool metacopy; > > + int metacopy; > > Should this be called 'metacopy_size' now? Honestly I don't know. That would change a lot of locations that still use this as "essentially" a boolean (i.e. != 0 means "has metacopy"), and ity would make that code less compact. I guess this is up to Amir and Miklos. Surely we could add a comment in the struct definition though. > > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path); > > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL); > > if (err < 0) > > goto out_err; > > This part is confusing because variables named 'err' conventionally contain only > 0 or a negative errno value. But this patch makes it possible for > ovl_check_metacopy_xattr() to return a positive size. It was already returning "negative, 0 or 1", so it's not fundamentally changed. Again, this is not my code so I'd rather Amir and Miklos decide such code style questions.
On Wed, Jul 5, 2023 at 11:07 AM Alexander Larsson <alexl@redhat.com> wrote: > > On Mon, Jul 3, 2023 at 9:14 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Wed, Jun 21, 2023 at 01:18:26PM +0200, Alexander Larsson wrote: > > > Historically overlay.metacopy was a zero-size xattr, and it's > > > existence marked a metacopy file. This change adds a versioned header > > > with a flag field, a length and a digest. The initial use-case of this > > > will be for validating a fs-verity digest, but the flags field could > > > also be used later for other new features. > > > > > > ovl_check_metacopy_xattr() now returns the size of the xattr, > > > emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to > > > distinguish it from the no-xattr case. > > > > > > Signed-off-by: Alexander Larsson <alexl@redhat.com> > > > --- > > > fs/overlayfs/namei.c | 10 +++++----- > > > fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++- > > > fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++---- > > > 3 files changed, 61 insertions(+), 10 deletions(-) > > > > > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > > > index 57adf911735f..3dd480253710 100644 > > > --- a/fs/overlayfs/namei.c > > > +++ b/fs/overlayfs/namei.c > > > @@ -25,7 +25,7 @@ struct ovl_lookup_data { > > > bool stop; > > > bool last; > > > char *redirect; > > > - bool metacopy; > > > + int metacopy; > > > > Should this be called 'metacopy_size' now? > > Honestly I don't know. That would change a lot of locations that still > use this as "essentially" a boolean (i.e. != 0 means "has metacopy"), > and ity would make that code less compact. I guess this is up to Amir > and Miklos. Surely we could add a comment in the struct definition > though. > I agree most of the code looks nicer when this stays 'metacopy' > > > - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path); > > > + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL); > > > if (err < 0) > > > goto out_err; > > > > This part is confusing because variables named 'err' conventionally contain only > > 0 or a negative errno value. But this patch makes it possible for > > ovl_check_metacopy_xattr() to return a positive size. > > It was already returning "negative, 0 or 1", so it's not fundamentally > changed. Again, this is not my code so I'd rather Amir and Miklos > decide such code style questions. > I agree. It wasn't 0 or negative before the change and I don't think this "convention" justifies adding another var. Thanks, Amir,
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 57adf911735f..3dd480253710 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -25,7 +25,7 @@ struct ovl_lookup_data { bool stop; bool last; char *redirect; - bool metacopy; + int metacopy; /* Referring to last redirect xattr */ bool absolute_redirect; }; @@ -270,7 +270,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d, d->stop = true; goto put_and_out; } - err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path); + err = ovl_check_metacopy_xattr(OVL_FS(d->sb), &path, NULL); if (err < 0) goto out_err; @@ -963,7 +963,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, .stop = false, .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe), .redirect = NULL, - .metacopy = false, + .metacopy = 0, }; if (dentry->d_name.len > ofs->namelen) @@ -1120,7 +1120,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, /* Defer lookup of lowerdata in data-only layers to first access */ if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) { - d.metacopy = false; + d.metacopy = 0; ctr++; } @@ -1211,7 +1211,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, upperredirect = NULL; goto out_free_oe; } - err = ovl_check_metacopy_xattr(ofs, &upperpath); + err = ovl_check_metacopy_xattr(ofs, &upperpath, NULL); if (err < 0) goto out_free_oe; uppermetacopy = err; diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index cf92a9aaf934..6d4e08df0dfe 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -7,6 +7,7 @@ #include <linux/kernel.h> #include <linux/uuid.h> #include <linux/fs.h> +#include <linux/fsverity.h> #include <linux/namei.h> #include <linux/posix_acl.h> #include <linux/posix_acl_xattr.h> @@ -140,6 +141,26 @@ struct ovl_fh { #define OVL_FH_FID_OFFSET (OVL_FH_WIRE_OFFSET + \ offsetof(struct ovl_fb, fid)) +/* On-disk format for "metacopy" xattr (if non-zero size) */ +struct ovl_metacopy { + u8 version; /* 0 */ + u8 len; /* size of this header + used digest bytes */ + u8 flags; + u8 digest_algo; /* FS_VERITY_HASH_ALG_* constant, 0 for no digest */ + u8 digest[FS_VERITY_MAX_DIGEST_SIZE]; /* Only the used part on disk */ +} __packed; + +#define OVL_METACOPY_MAX_SIZE (sizeof(struct ovl_metacopy)) +#define OVL_METACOPY_MIN_SIZE (OVL_METACOPY_MAX_SIZE - FS_VERITY_MAX_DIGEST_SIZE) +#define OVL_METACOPY_INIT { 0, OVL_METACOPY_MIN_SIZE } + +static inline int ovl_metadata_digest_size(const struct ovl_metacopy *metacopy) +{ + if (metacopy->len < OVL_METACOPY_MIN_SIZE) + return 0; + return (int)metacopy->len - OVL_METACOPY_MIN_SIZE; +} + extern const char *const ovl_xattr_table[][2]; static inline const char *ovl_xattr(struct ovl_fs *ofs, enum ovl_xattr ox) { @@ -490,7 +511,8 @@ bool ovl_need_index(struct dentry *dentry); int ovl_nlink_start(struct dentry *dentry); void ovl_nlink_end(struct dentry *dentry); int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path); +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path, + struct ovl_metacopy *data); 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_sync_status(struct ovl_fs *ofs); diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 7ef9e13c404a..921747223991 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -1054,8 +1054,12 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir) return -EIO; } -/* err < 0, 0 if no metacopy xattr, 1 if metacopy xattr found */ -int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) +/* + * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found. + * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value. + */ +int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path, + struct ovl_metacopy *data) { int res; @@ -1063,7 +1067,8 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) if (!S_ISREG(d_inode(path->dentry)->i_mode)) return 0; - res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, NULL, 0); + res = ovl_path_getxattr(ofs, path, OVL_XATTR_METACOPY, + data, data ? OVL_METACOPY_MAX_SIZE : 0); if (res < 0) { if (res == -ENODATA || res == -EOPNOTSUPP) return 0; @@ -1077,7 +1082,31 @@ int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path) goto out; } - return 1; + if (res == 0) { + /* Emulate empty data for zero size metacopy xattr */ + res = OVL_METACOPY_MIN_SIZE; + if (data) { + memset(data, 0, res); + data->len = res; + } + } else if (res < OVL_METACOPY_MIN_SIZE) { + pr_warn_ratelimited("metacopy file '%pd' has too small xattr\n", + path->dentry); + return -EIO; + } else if (data) { + if (data->version != 0) { + pr_warn_ratelimited("metacopy file '%pd' has unsupported version\n", + path->dentry); + return -EIO; + } + if (res != data->len) { + pr_warn_ratelimited("metacopy file '%pd' has invalid xattr size\n", + path->dentry); + return -EIO; + } + } + + return res; out: pr_warn_ratelimited("failed to get metacopy (%i)\n", res); return res;
Historically overlay.metacopy was a zero-size xattr, and it's existence marked a metacopy file. This change adds a versioned header with a flag field, a length and a digest. The initial use-case of this will be for validating a fs-verity digest, but the flags field could also be used later for other new features. ovl_check_metacopy_xattr() now returns the size of the xattr, emulating a size of OVL_METACOPY_MIN_SIZE for empty xattrs to distinguish it from the no-xattr case. Signed-off-by: Alexander Larsson <alexl@redhat.com> --- fs/overlayfs/namei.c | 10 +++++----- fs/overlayfs/overlayfs.h | 24 +++++++++++++++++++++++- fs/overlayfs/util.c | 37 +++++++++++++++++++++++++++++++++---- 3 files changed, 61 insertions(+), 10 deletions(-)