Re: [Devel] [PATCH hci-8.1 v4 5/5] ve/overlayfs: Override VE together with creds

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ovl: ovl_override_creds() fails with WARN and -EPERM for CT callers of a foreign-VE overlay
--

commit 9e04559c3517d9690eb025bf97e30b602d11f4ca
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve/overlayfs: Override VE together with creds

This patch changes ovl_override_creds() to also override the VE
(through override_ve()) using ofs->creator_ve. It returns both the old
cred and the old VE in a new struct ovl_creds_desc, and converts all
overlayfs callers to the new ovl_override_creds()/ovl_revert_creds()
pair. The stated goal is to let a host user access an overlayfs created
inside a CT without ve_capable() checks failing.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405

> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f95090f0d29c..d28d572d6099 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c

[ ... ]

> @@ -63,11 +63,27 @@ struct dentry *ovl_workdir(struct dentry *dentry)
>  	return ofs->workdir;
>  }
>  
> -const struct cred *ovl_override_creds(struct super_block *sb)
> +/* On error no revert is needed. */
> +int ovl_override_creds(struct super_block *sb, struct ovl_creds_desc *desc)
>  {
>  	struct ovl_fs *ofs = OVL_FS(sb);
>  
> -	return override_creds(ofs->creator_cred);
> +	desc->ve = override_ve(ofs->creator_ve);
> +	if (IS_ERR(desc->ve))
> +		return PTR_ERR(desc->ve);
> +
> +	desc->cred = override_creds(ofs->creator_cred);
> +	return 0;
> +}

[Severity: Medium]
Can this turn a previously working access into a WARN plus -EPERM for
tasks running in a container?

ovl_fill_super() stores ofs->creator_ve = get_ve(get_exec_env()), so
creator_ve is the VE of the mounting task. Every converted entry point
(ovl_lookup(), ovl_permission(), ovl_getattr(), ovl_iterate(),
ovl_open_realfile(), the xattr and copy-up paths) now calls
ovl_override_creds(), which calls override_ve(ofs->creator_ve).

With CONFIG_VE, override_ve() in include/linux/ve.h does:

    if (WARN_ON(new_ve != get_exec_env() && !ve_is_super(get_exec_env())))
        return ERR_PTR(-EPERM);

That condition is true for any non-host task whose VE differs from
creator_ve. One example is a CT task reaching a host-mounted overlay
through a bind mount or mount propagation. Another is a CT task reaching
another CT's overlay. In both cases, ovl_override_creds() returns -EPERM
and the operation fails.

Before this patch only the creds were swapped, so that access worked.
Each failure now also emits a WARN backtrace, which is a panic with
panic_on_warn. Is this reachable from within a container?

The commit message covers only the host-accessing-CT direction and does
not mention this one. Is it intended that a CT task accessing an overlay
created in a different VE fails with -EPERM?

If it is intended, would it be better to avoid the WARN_ON backtrace on
this path, for example by checking the VE relationship in
ovl_override_creds() before calling override_ve()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109-ovl-use-orig-ve-v4-0-88e65484dc5b@virtuozzo.com?part=5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel