On 11/9/26 20:52, Vladimir Riabchun wrote:
>
>
> On 11/9/26 20:43, [email protected] 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 <[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()).
>
> 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.
We have gpu passthrough vzctl code adding host overlay to CT, and this
error path breaks the use of this overlay.
I suggest instead of WARN_ON + EPERM, just skip override. As a bonus we
would not need to handle error paths everywhere. Gives us same security
I guess.
AI suggests:
/* Only host tasks may switch VE, CT tasks keep their own */
if (!ve_is_super(get_exec_env()))
return old_ve;
Also suggestion from AI that we don't need crashing WARN_ON and should be
fine with pr_warn_once to avoid spamming dmesg - seems reasonable.
>
>>
>> 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, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
