Re: [Devel] [PATCH hci-8.1 v3 2/5] ve: Introduce override_ve/revert_ve helpers

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ve: revert_ve() can store ERR_PTR from override_ve() into overrided_ve
- [High] ve: override_ve() ERR_PTR(-EPERM) stored into current->overrided_ve by revert_ve()
- [Medium] ve: task->overrided_ve is inherited across fork and never reset in copy_process()
--

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

ve: Introduce override_ve/revert_ve helpers

This adds override_ve() and revert_ve(), which mirror
override_creds()/revert_creds(). It also adds a task->overrided_ve field.
ve_capable() and ve_capable_noaudit() now use that field in preference to
get_exec_env().

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

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h

[ ... ]

> @@ -1065,6 +1065,7 @@ struct task_struct {
>  #ifdef CONFIG_VE
>  	struct ve_struct		*task_ve;
>  	struct ve_namespace		*ve_ns;
> +	struct ve_struct		*overrided_ve; /* NULL when no overrides */
>  #endif

[Severity: Medium]
Can this pointer be inherited across fork? dup_task_struct() copies the
whole task_struct, so a child created while the parent is between
override_ve() and revert_ve() gets the parent's non-NULL overrided_ve.

copy_process()->copy_ve_ns() only sets these fields:

    p->ve_ns = new_ve_ns;
    p->task_ve = new_ve_ns->ve;

It never clears overrided_ve, so the comment "NULL when no overrides" may
not hold for the child. The child never runs revert_ve(), because that
only acts on current.

ve_capable() and ve_capable_noaudit() in the child would then use the
parent's overriding VE init_cred instead of the child's own
get_exec_env().

I did not find a concrete in-tree caller that forks inside the window.
Should overrided_ve be reset to NULL at fork time?

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h

[ ... ]

> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
> +{
> +	struct ve_struct *old_ve = current->overrided_ve;
> +
> +	/* CT tasks are not supposed to switch VE context, this is a breach */
> +	if (WARN_ON(new_ve != get_exec_env() && !ve_is_super(get_exec_env())))
> +		return ERR_PTR(-EPERM);
> +
> +	current->overrided_ve = new_ve;
> +	return old_ve;
> +}
> +
> +static inline void revert_ve(struct ve_struct *orig_ve)
> +{
> +	current->overrided_ve = orig_ve;
> +}

[Severity: High]
Can revert_ve() store an ERR_PTR into current->overrided_ve? When the
WARN_ON condition holds, override_ve() returns ERR_PTR(-EPERM) and leaves
overrided_ve unchanged. That return value has the same type as a valid
previous-override cookie.

revert_ve() has no IS_ERR() check and stores whatever it is given:

    current->overrided_ve = orig_ve;

In the final series state, ovl_override_creds() does this:

fs/overlayfs/util.c:ovl_override_creds() {
    ...
    desc.ve = override_ve(ofs->creator_ve);
    ...
}

It does not check the result with IS_ERR(). ovl_revert_creds() then calls
revert_ve(old_creds->ve) unconditionally.

A container task that uses an overlayfs superblock whose creator_ve
differs from its own VE, for example one created by the host, would hit
the WARN_ON and end up with ERR_PTR(-EPERM) in current->overrided_ve.

> diff --git a/kernel/capability.c b/kernel/capability.c
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -389,10 +389,11 @@ EXPORT_SYMBOL(ns_capable);
>  bool ve_capable(int cap)
>  {
>  	struct cred *cred;
> +	struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
>  	bool ret;
>  
>  	rcu_read_lock();
> -	cred = get_exec_env()->init_cred;
> +	cred = ve->init_cred;

[Severity: High]
Can this dereference an ERR_PTR? ERR_PTR(-EPERM) is non-NULL, so the ?:
expression selects it. The next ve_capable() or ve_capable_noaudit() call
by that task then reads ve->init_cred through an invalid pointer.
ve_capable_noaudit() has the same pattern.

Should override_ve() callers check IS_ERR(), and should revert_ve() skip
error pointers?

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

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