On Fri, Jul 5, 2024 at 7:25 AM Mike Baynton <mike@xxxxxxxxxxxx> wrote:
>
> There seem to be a few scenarios where it is possible to successfully
> mount up an overlay filesystem including data-only layer(s), but in
> configurations where it will never be possible to read data successfully
> from the data-only layers. I think this should result in a mount-time
> error instead of the current behavior of being unable to read data from
> the files that should normally return data from a data-only layer.
>
> Both cases were found by attempting to use data-only lower layers from a
> user namespace, a proposition that appears to be guaranteed to not end
> well since data-only lower layers requires use of trusted xattrs, but
> trusted xattrs can only be accessed in the initial user namespace.
>
> Case 1: upper dirs in use but xattrs cannot be written to the filesystem
> containing workdir (for any reason, user namespace-related or not.) This
> triggers a fallback behavior of disabling metacopy after an existing
> validation in ovl_fs_params_verify ensured metacopy is on when
> data-only layers are present. This is now rechecked after possibly
> disabling metacopy.
>
> Case 2: upper dirs are not in use, data-only layer(s) in use, mount
> initiated from a user namespace other than the initial one.
>
> When the filesystem consists of only lower layers, the test of xattrs
> is not performed and so metacopy remains on, satisfying Case 1.
> Therefore it is also neceessary to explicitly check for data-only layers
> in a mount whose initiator lacks CAP_SYS_ADMIN in the initial user
> namespace.
>
> Signed-off-by: Mike Baynton <mike@xxxxxxxxxxxx>
> ---
> fs/overlayfs/super.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 06a231970cb5..4382f21c36a0 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1394,6 +1394,19 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
> if (IS_ERR(oe))
> goto out_err;
>
> + if (ofs->numdatalayer) {
> + if (!ofs->config.metacopy) {
> + pr_err("lower data-only dirs require metacopy support.\n");
> + err = -EINVAL;
> + goto out_err;
> + }
Is that not already handled by?
int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
struct ovl_config *config)
{
struct ovl_opt_set set = ctx->set;
if (ctx->nr_data > 0 && !config->metacopy) {
pr_err("lower data-only dirs require metacopy support.\n");
return -EINVAL;
}
Probably because of:
ofs->config.metacopy = false;
pr_warn("...falling back to metacopy=off.\n");
in xattr check, but it could also happen from:
/* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
if (config->userxattr) {
...
/*
* Silently disable default setting of redirect and metacopy.
* This shall be the default in the future as well: these
* options must be explicitly enabled if used together with
* userxattr.
*/
config->redirect_mode = OVL_REDIRECT_NOFOLLOW;
config->metacopy = false;
}
So maybe also the lowerdatadirs vs metacopy conflict should be moved
to the end of
ovl_fs_params_verify()?
> + if (!capable(CAP_SYS_ADMIN)) {
> + pr_err("lower data-only dirs require CAP_SYS_ADMIN in the initial user namespace.\n");
> + err = -EPERM;
> + goto out_err;
> + }
This is too specific IMO.
If we really want to check CAP_SYS_ADMIN at mount time, we should error
on any configuration that requires trusted xattrs and suggest that the user
will use -o userxattr, and maybe disable the conflicting config if it was not
explicitly specified in mount options.
Of course, userxattr conflicts with some other options including
redirect_dir, metacopy and verity, but that just means that the errors
will have to be smarter and the check for CAP_SYS_ADMIN should
definitely be in ovl_fs_params_verify() if we add them.
Thanks,
Amir.
