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

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 11/9/26 20:43, no-reply@sashiko.virtuozzo.com wrote:
> 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()).

This is a security issue and we really don't want to
allow this. Just don't make an overlayfs in a CT over an overlayfs
from another, this is weird.

> 
> 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?

We want to see a call trace for debugging purposes in such case.
I think WARN_ON is fine. User should see a bunch of warnings that
something is going terribly wrong and run to us and report it.

> 
>> +
>> +	current->overrided_ve = new_ve;
>> +	return old_ve;
>> +}
> 
> [ ... ]
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

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