From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=100.103.45.18 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793199874; b=XxxUguDkaNeliO6RtbOqwtp0OQwe6/7mq8AwSqKAsCzmMR3PtAwFT0Bu+YQg+qTGmJb1 98DJ3idb1DaRxQaGKi+xTH+TJ73QpbiaVN2YY5AqV8w8591gzF01PEdCMmfKrB2fHvnhs QQ6qNUiVvv8PdJWzcZGJZcCZ6QT9tAVjf+cfKD1r5oGL10QMJfpl7NifChMQu3U7YFF11 dUPC9G5D+hFzPZsUiwsobh41gSX5OYAbBMdOkWCqsjjRoRPtuw0SBSjlhLSb6AuBUTplo BHflkhsDkY65AWaEf/YHqoP5iPXV01Bwfd9hJ4dZ87ljIqvWKQfD0dqFxlkoAIEPDpw== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793199874; h=Received:DKIM-Signature:Message-ID:From:Subject:Reply-To:To:Cc: In-Reply-To:References:Content-Type:Content-Transfer-Encoding:Date; bh=+Rsc9g24MtPLpVFEqmviyx5L0j/Yg5yRWd8d0hmWbnc=; b=rY+3thnbokM1F0FUS9pxccwmYDJgDLk0qz1eCWCF3vd1EpFFs5KxolTfWoXyCxbBFFo2 7+fizBRbm3fQguWvXRGH9ulr9NmNiGoUMk3T6EUOT8z1R16DrcBdrgzdTafsfHbozowEB Nox3Fia/ahYzQNJMCHkWp6nin3qunx5skIsiJNOHkH/c2LJKsx5I10L13msCauIW+b+4q GFi5e9KSMMiOooA0HNdKOPF2M3SnWqKCUfRbHUsKDxGhjImLkvKyT551u6WYD0fs44xUx 4ls92B6UPvvmdoJkA9VQyIY7jqWqSx4q3fHAS5S3InowOp94C42R2sU98cHYtn9eQAg== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=ZkxBsnFF; arc=none smtp.remote-ip=100.103.45.18 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1960F606A338; Wed, 28 Oct 2026 15:04:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=ZkxBsnFF Received: by smtp.kernel.org (Postfix) with ESMTPSA id 45D381F031E1; Wed, 28 Oct 2026 15:04:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1793199873; bh=+Rsc9g24MtPLpVFEqmviyx5L0j/Yg5yRWd8d0hmWbnc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=ZkxBsnFFHC/9gTRrdBEknTJoTVZswKRDlPDucehz4Dm8MSiKl46eCXT1gByUpUafd jnnLlwddU9v1iqiS4g5eIh2DqyeNhaseMSbKOJPzpRj4Y1cPELGRH+v/WQSuCyAhWh f/dC/tZ4EdQ13mxHFfYgAV+Y1a527tVWVLT/liknT+QPmlmamJuQQYsp9NHiqPq7xK DuwaHnmvGLSZ5yhpngyULqUh69y5/4tUeRPI/7ZillyArv5BpnnLfZQPdBwRXlAS8o tt8eHigoxeUfc/bo9NX5aRFDlx2pFDSsoLSIQdUALqDrV29y4gIKDqfCikxAeAVEg0 zeS8UrUwPaGyA== Message-ID: From: sashiko-bot@kernel.org Subject: Re: [PATCH v3 2/8] dma-heap: charge dma-buf memory via explicit memcg Reply-To: sashiko-reviews@lists.linux.dev To: "Albert Esteve" , "Tejun Heo" , "Johannes Weiner" , =?utf-8?b?TWljaGFsIEtvdXRuw70=?= , "Jonathan Corbet" , "Shuah Khan" , "Sumit Semwal" , =?utf-8?b?Q2hyaXN0aWFuIEvDtm5pZw==?= , "Michal Hocko" , "Roman Gushchin" , "Shakeel Butt" , "Muchun Song" , "Andrew Morton" , "Benjamin Gaignard" , "Brian Starkey" , "John Stultz" , "T.J. Mercier" , "Christian Brauner" , "Paul Moore" , "James Morris" , "Serge E. Hallyn" , "Stephen Smalley" , "Shuah Khan" , "Randy Dunlap" , =?utf-8?b?T25kcmVqIE1vc27DocSNZWs=?= , "Jens Wiklander" , "Sumit Garg" Cc: dri-devel@lists.freedesktop.org, linux-mm@kvack.org, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, op-tee@lists.trustedfirmware.org, echanude@redhat.com, mripard@kernel.org, media-ci@linuxtv.org In-Reply-To: <20261027-v2_20230123_tjmercier_google_com-v3-2-78d2c4cb8647@redhat.com> References: <20261027-v2_20230123_tjmercier_google_com-v3-0-78d2c4cb8647@redhat.com> <20261027-v2_20230123_tjmercier_google_com-v3-2-78d2c4cb8647@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Wed, 28 Oct 2026 15:04:31 +0000 X-Mailing-List: linux-security-module@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list Thank you for your contribution! Sashiko AI review found 4 potential issue(= s) to consider: - [Critical] dma-heap: missing permission check in mem_cgroup_get_from_fd() - [High] dma-buf: system_heap: 32-bit nr_pages truncation in system_heap_al= locate() - [Medium] mm: memcontrol: missing EXPORT_SYMBOL_GPL() on dmabuf memcg help= ers - [Medium] mm: memcontrol: mem_cgroup_get_from_fd() returns NULL when memcg= is disabled -- commit 961ee31303129349ee7fa6988bef4bc244f22a61 Author: Albert Esteve dma-heap: charge dma-buf memory via explicit memcg This commit adds a charge_fd field to struct dma_heap_allocation_data so DMA_HEAP_IOCTL_ALLOC can resolve a target cgroup from a pidfd or cgroupfd v= ia mem_cgroup_get_from_fd() and charge system heap allocations to that memcg. [ ... ] > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/= system_heap.c > index c8959eadc71d1..adeea8cd328b8 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -388,8 +389,6 @@ static struct page *alloc_largest_available(unsigned = long size, > if (max_order < orders[i]) > continue; > flags =3D order_flags[i]; > - if (mem_accounting) > - flags |=3D __GFP_ACCOUNT; > page =3D alloc_pages(flags, orders[i]); > if (!page) > continue; > @@ -401,11 +400,13 @@ static struct page *alloc_largest_available(unsigne= d long size, > static struct dma_buf *system_heap_allocate(struct dma_heap *heap, > unsigned long len, > u32 fd_flags, > - u64 heap_flags) > + u64 heap_flags, > + struct mem_cgroup *memcg) > { > struct system_heap_buffer *buffer; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > unsigned long size_remaining =3D len; > + unsigned int nr_pages =3D PAGE_ALIGN(len) >> PAGE_SHIFT; [Severity: High] Can nr_pages truncate on 64-bit systems when a caller passes a large heap_allocation->len via DMA_HEAP_IOCTL_ALLOC? In dma_heap_ioctl_allocate(), heap_allocation->len is page-aligned into a size_t len and only checked for zero before being passed to system_heap_allocate() as an unsigned long: drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() { ... len =3D PAGE_ALIGN(heap_allocation->len); if (!len) { mem_cgroup_put(memcg); return ERR_PTR(-EINVAL); } dmabuf =3D heap->ops->allocate(heap, len, heap_allocation->fd_flags, heap_allocation->heap_flags, memcg); } In system_heap_allocate(), size_remaining keeps the full 64-bit len, while nr_pages is stored in a 32-bit unsigned int. Because __GFP_ACCOUNT is remov= ed from alloc_largest_available(), cgroup enforcement now relies solely on mem_cgroup_reserve_dmabuf(memcg, GFP_KERNEL, nr_pages). If userspace passes len >=3D (1ULL << (32 + PAGE_SHIFT)) (for example, 16 T= iB with 4 KiB pages), nr_pages wraps to 0 (or a small value), so mem_cgroup_reserve_dmabuf() succeeds without charging the cgroup while the while (size_remaining > 0) loop continues allocating uncharged physical pag= es until global memory exhaustion. In addition, values of nr_pages in [0x80000000U, 0xffffffffU] are passed by mem_cgroup_commit_dmabuf() to mod_memcg_state(), which takes a signed int val and would corrupt MEMCG_DMABUF statistics. > unsigned int max_order =3D orders[0]; > struct system_heap_priv *priv =3D dma_heap_get_drvdata(heap); > bool cc_shared =3D priv->cc_shared; > @@ -416,10 +417,15 @@ static struct dma_buf *system_heap_allocate(struct = dma_heap *heap, > struct page *page, *tmp_page; > int i, ret =3D -ENOMEM; > =20 > - buffer =3D kzalloc_obj(*buffer); > - if (!buffer) > + if (!mem_cgroup_reserve_dmabuf(memcg, GFP_KERNEL, nr_pages)) > return ERR_PTR(-ENOMEM); > =20 > + buffer =3D kzalloc_obj(*buffer); > + if (!buffer) { > + ret =3D -ENOMEM; > + goto cancel_charge; > + } > + > INIT_LIST_HEAD(&buffer->attachments); > mutex_init(&buffer->lock); > buffer->heap =3D heap; > @@ -478,6 +484,9 @@ static struct dma_buf *system_heap_allocate(struct dm= a_heap *heap, > ret =3D PTR_ERR(dmabuf); > goto free_pages; > } > + > + mem_cgroup_commit_dmabuf(dmabuf, memcg, nr_pages); > + > return dmabuf; > =20 > free_pages: > @@ -498,6 +507,8 @@ static struct dma_buf *system_heap_allocate(struct dm= a_heap *heap, > list_for_each_entry_safe(page, tmp_page, &pages, lru) > __free_pages(page, compound_order(page)); > kfree(buffer); > +cancel_charge: > + mem_cgroup_cancel_dmabuf(memcg, nr_pages); [Severity: Medium] Will building with CONFIG_DMABUF_HEAPS_SYSTEM=3Dm and CONFIG_MEMCG=3Dy fail= at modpost with undefined symbol errors for mem_cgroup_reserve_dmabuf(), mem_cgroup_commit_dmabuf(), and mem_cgroup_cancel_dmabuf()? In drivers/dma-buf/heaps/Kconfig, DMABUF_HEAPS_SYSTEM is tristate, so drivers/dma-buf/heaps/system_heap.c can be compiled as a loadable module (system_heap.ko). However, mem_cgroup_reserve_dmabuf(), mem_cgroup_commit_dmabuf(), and mem_cgroup_cancel_dmabuf() in mm/memcontrol= .c do not have EXPORT_SYMBOL_GPL() annotations. Should those symbols be exported (or should the reservation/commit/cancel calls live in the built-in drivers/dma-buf/dma-heap.c layer as described in the commit message)? [ ... ] > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index bbd46ba40350b..f66254f52b999 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5661,6 +5661,53 @@ void mem_cgroup_flush_workqueue(void) > flush_workqueue(memcg_wq); > } > =20 > +/** > + * mem_cgroup_get_from_fd - resolve a memcg from a cgroupfd or pidfd > + * @fd: file descriptor (cgroupfd or pidfd) > + * > + * Tries @fd as a cgroupfd first; if that fails, tries it as a pidfd. > + * On success returns a referenced memcg pointer. On failure returns > + * an ERR_PTR. > + */ > +struct mem_cgroup *mem_cgroup_get_from_fd(int fd) > +{ > + struct cgroup_subsys_state *css; > + struct mem_cgroup *memcg; > + struct cgroup *cgrp; > + struct task_struct *task; > + struct mm_struct *mm; > + unsigned int pidfd_flags; > + > + cgrp =3D cgroup_get_from_fd(fd); > + if (!IS_ERR(cgrp)) { > + rcu_read_lock(); > + css =3D cgroup_css(cgrp, &memory_cgrp_subsys); > + if (!css || !css_tryget_online(css)) { > + rcu_read_unlock(); > + cgroup_put(cgrp); > + return ERR_PTR(-ENOENT); > + } > + > + memcg =3D mem_cgroup_from_css(css); > + rcu_read_unlock(); > + cgroup_put(cgrp); > + return memcg; > + } > + > + task =3D pidfd_get_task(fd, &pidfd_flags); > + if (IS_ERR(task)) > + return ERR_CAST(task); > + > + mm =3D get_task_mm(task); > + put_task_struct(task); > + if (!mm) > + return ERR_PTR(-ESRCH); > + > + memcg =3D get_mem_cgroup_from_mm(mm); > + mmput(mm); > + return memcg; > +} [Severity: Critical] What prevents an unprivileged user with access to a dma-heap device from passing a cgroupfd (opened with O_RDONLY | O_DIRECTORY or O_PATH on any world-readable /sys/fs/cgroup directory) or a pidfd (via pidfd_open() on PID 1 or another user's process) as charge_fd to DMA_HEAP_IOCTL_ALLOC? Through dma_heap_ioctl_allocate() -> mem_cgroup_get_from_fd() and system_heap_allocate() -> mem_cgroup_reserve_dmabuf() -> try_charge_memcg(), charging to the root cgroup (where mem_cgroup_is_root(memcg) skips page counter charging) allows a restricted process to bypass its own memory.max and memory.high limits, while charging to another cgroup allows exhausting that victim cgroup's quota and triggering mem_cgroup_oom() kills inside it. Later patches in the series ("security: dma-buf: Add dma_buf_charge_task LSM hook", "security: dma-buf: Add dma_buf_charge_cgroup LSM hook", "selinux: Restrict cross-cgroup dma-buf charging", and "selinux: Restrict dma-buf direct cgroup charging") add LSM hooks and SELinux checks, but there are no baseline DAC, capability, ptrace_may_access(), or cgroup write permission checks when CONFIG_SECURITY is disabled, when SELinux is not active, or when a cgroup directory has no explicit security.selinux xattr (where selinux_dma_buf_charge_cgroup() returns 0 on -ENODATA). Also, when charging a remote memcg via try_charge_memcg() breaches that target cgroup's memory.high limit, try_charge_memcg() increments current->memcg_nr_pages_over_high and calls __mem_cgroup_handle_over_high(), which resolves get_mem_cgroup_from_mm(current->mm) and reclaims from the allocating process's own cgroup rather than the remotely charged memcg. Should remote charging require appropriate permissions over the target task= or cgroup, and how should memory.high reclaim be handled when charging a forei= gn memcg? [Severity: Medium] In mem_cgroup_get_from_fd(), what happens on the pidfd path when CONFIG_MEMCG=3Dy and memory cgroups are disabled at boot via cgroup_disable=3Dmemory? When mem_cgroup_disabled() is true, get_mem_cgroup_from_mm(mm) returns NULL, so mem_cgroup_get_from_fd() returns NULL instead of an ERR_PTR as documented in its kernel-doc comment. In dma_heap_ioctl_allocate() (and in dma_buf_ioctl_xfer_charge() later in t= he series), IS_ERR(memcg) is false for NULL, so a pidfd charge_fd silently fal= ls through while a cgroupfd charge_fd fails with -ENOENT (because cgroup_css() returns NULL when the subsystem is disabled). Should the pidfd path also return an ERR_PTR when get_mem_cgroup_from_mm(mm) returns NULL? --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261027-v2_2023012= 3_tjmercier_google_com-v3-0-78d2c4cb8647@redhat.com?part=3D2