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 <[email protected]> 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/[email protected]?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
