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

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

On 11/3/26 10:30, no-reply@sashiko.virtuozzo.com wrote:
> 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;

override_ve by design is changing current only.
It should never happen in fork path.
To save us from stupid hiccups in the future, I'll
add a WARN and field reset in dup_task_struct.

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

True. I'll add WARN_ON in revert_ve to protect from error values
reaching overrided_ve.

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

With updated revert_ve with IS_ERR protection it wouldn't.

I'll fix this in v4.

> 

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

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