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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ve: override_ve() uses a non-ratelimited WARN_ON as its permission check
--

Commit dab91ee458061d579a92288ba48a09c48c12df09
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: Introduce override_ve/revert_ve helpers

This adds override_ve() and revert_ve() helpers that mirror
override_creds() and revert_creds(). It also adds a task->overrided_ve
field, which ve_capable() and ve_capable_noaudit() now consult, and a
check in dup_task_struct() that the field is not inherited on fork.

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

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

[ ... ]

> @@ -295,6 +295,26 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
>  	return ve->vdso_64;
>  }
>  
> +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);

[Severity: Low]
Can this WARN_ON fire on a legitimate path? override_ve() is called from
ovl_override_creds() with ofs->creator_ve, which is fixed at mount time
in ovl_fill_super() as get_ve(get_exec_env()).

If a container task reaches an overlay whose creator VE is not its own VE
(for example an overlay mounted by the host and exposed to the container,
perhaps through a bind mount), the condition
new_ve != get_exec_env() && !ve_is_super(get_exec_env())
is true on every lookup, permission check, getattr, readdir and so on.

Each of those operations would then log a warning splat and fail with
-EPERM, where they previously succeeded. I could not confirm from the
repository that such exposure actually occurs, so this depends on whether
it is reachable.

The WARN_ON is neither _ONCE nor ratelimited, so repeated failures can
flood the log, and with panic_on_warn set the system would panic. Would
a ratelimited or one-time warning, or no warning and just the -EPERM
return, be more appropriate for a permission check that callers already
handle?

> +
> +	current->overrided_ve = new_ve;
> +	return old_ve;
> +}

[ ... ]

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

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