Re: [PATCH v4 4/8] security: dma-buf: Add dma_buf_charge_task LSM hook

[sashiko-bot--- via OP-TEE <op-tee@lists.trustedfirmware.org>]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: memcontrol: task cred and mm->owner mismatch in mem_cgroup_get_from_fd()
--

commit 8654d1fa67f56b32794f6b84163f6fcd4bd83d11
Author: Albert Esteve <aesteve@redhat.com>

security: dma-buf: Add dma_buf_charge_task LSM hook

This commit introduces the security_dma_buf_charge_task() LSM hook and calls
it in mem_cgroup_get_from_fd() when resolving a target cgroup from a pidfd.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3e717b5663c82..dd276e1230cc4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c

[ ... ]

> @@ -5698,6 +5701,14 @@ struct mem_cgroup *mem_cgroup_get_from_fd(int fd)
>  	if (IS_ERR(task))
>  		return ERR_CAST(task);
>  
> +	tcred = get_task_cred(task);
> +	ret = security_dma_buf_charge_task(current_cred(), tcred);
> +	put_cred(tcred);
> +	if (ret) {
> +		put_task_struct(task);
> +		return ERR_PTR(ret);
> +	}
> +
>  	mm = get_task_mm(task);
>  	put_task_struct(task);
>  	if (!mm)

[Severity: High]
As reported by Sashiko on v3 [1], in mem_cgroup_get_from_fd(),
security_dma_buf_charge_task(current_cred(), tcred) checks the credentials of
task obtained via get_task_cred(task), while the charged mem_cgroup is then
resolved indirectly via get_task_mm(task) and get_mem_cgroup_from_mm(mm):

mm/memcontrol.c:get_mem_cgroup_from_mm() {
	rcu_read_lock();
	do {
		memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
		if (unlikely(!memcg))
			memcg = root_mem_cgroup;
	} while (!css_tryget(&memcg->css));
	rcu_read_unlock();
	return memcg;
}

If task exits between get_task_mm(task) and get_mem_cgroup_from_mm(mm),
exit_mm() clears task->mm and calls mm_update_next_owner(mm), which sees
mm_users > 1 with no live task using mm and sets mm->owner to NULL:

kernel/exit.c:mm_update_next_owner() {
	...
	WRITE_ONCE(mm->owner, NULL);
	...
}

In that case, get_mem_cgroup_from_mm(mm) falls back to returning
root_mem_cgroup, where try_charge() skips accounting because
mem_cgroup_is_root(memcg) is true. Could a confined process fork a
short-lived helper child with matching credentials to pass
security_dma_buf_charge_task() and race its exit to charge or transfer
dma-buf allocations to root_mem_cgroup, escaping its cgroup memory limit?

Similarly, if task shares an mm_struct with another task via clone(CLONE_VM)
that has different credentials or cgroup membership, mm->owner may not be
task. Could security_dma_buf_charge_task() validate task's credentials while
get_mem_cgroup_from_mm(mm) charges the cgroup of a different mm->owner task?

[1] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-3-78d2c4cb8647@redhat.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-v2_20230123_tjmercier_google_com-v4-0-1819a017000b@redhat.com?part=4