Re: [Devel] [PATCH hci-8.1 v3 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:
- [High] overlayfs: ovl_override_creds() ignores ERR_PTR from override_ve()
--

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

ve/overlayfs: Override VE together with creds

This patch changes ovl_override_creds() to return a struct ovl_creds_desc
holding both the old cred and the old VE. It also adds ovl_revert_creds()
to restore both. All overlayfs callers are converted to the new pair of
helpers.

> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -63,11 +63,21 @@ struct dentry *ovl_workdir(struct dentry *dentry)
>  	return ofs->workdir;
>  }
>  
> -const struct cred *ovl_override_creds(struct super_block *sb)
> +struct ovl_creds_desc ovl_override_creds(struct super_block *sb)
>  {
>  	struct ovl_fs *ofs = OVL_FS(sb);
> +	struct ovl_creds_desc desc;
>  
> -	return override_creds(ofs->creator_cred);
> +	desc.cred = override_creds(ofs->creator_cred);
> +	desc.ve = override_ve(ofs->creator_ve);

[Severity: High]
Can override_ve() return an error pointer here that goes unchecked?

override_ve() in include/linux/ve.h has this early return:

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

ofs->creator_ve is set at mount time, so it can differ from the VE of the
task using the filesystem. One example is a CT task using an overlay mounted
by the host or by another CT, through a bind mount or setns().

In that case override_ve() returns ERR_PTR(-EPERM) without setting
current->overrided_ve. The error pointer is stored in desc.ve, and the
overlay operation carries on.

> +
> +	return desc;
> +}
> +
> +void ovl_revert_creds(struct ovl_creds_desc *old_creds)
> +{
> +	revert_ve(old_creds->ve);

Can this store the error pointer in current->overrided_ve?

Every call site pairs ovl_override_creds() with ovl_revert_creds(). On the
path above, revert_ve(old_creds->ve) would then do:

    current->overrided_ve = ERR_PTR(-EPERM);

A nested ovl_override_creds() would also save this bogus value as its old VE.

ve_capable() and ve_capable_noaudit() in kernel/capability.c compute:

    struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
    ...
    cred = ve->init_cred;

The error pointer is non-NULL, so it would be dereferenced as a ve_struct.
That would be an oops on any later capability check by that task, including
paths that never touch overlayfs.

The WARN_ON in override_ve() would also fire on every overlay operation that
takes this path.

Would it make sense to check IS_ERR() in ovl_override_creds() and record that
no VE override was done? ovl_revert_creds() could then skip revert_ve() in
that case. Alternatively, the VE override could be limited to tasks running
in ve0.

> +	revert_creds(old_creds->cred);
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-ovl-use-orig-ve-v3-0-add042cdd087@virtuozzo.com?part=5

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