| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFgf54p1T_Vsx_QLX1XOJJz6v6K337G5T0Gw=V=9LL4h_CWZHQ@mail.gmail.com>
Date: Mon, 24 Nov 2025 12:45:06 +0000
From: Mostafa Saleh <smostafa@...gle.com>
To: Will Deacon <will@...nel.org>
Cc: linux-mm@...ck.org, iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, corbet@....net, joro@...tes.org,
robin.murphy@....com, akpm@...ux-foundation.org, vbabka@...e.cz,
surenb@...gle.com, mhocko@...e.com, jackmanb@...gle.com, hannes@...xchg.org,
ziy@...dia.com, david@...hat.com, lorenzo.stoakes@...cle.com,
Liam.Howlett@...cle.com, rppt@...nel.org, Qinxin Xia <xiaqinxin@...wei.com>
Subject: Re: [PATCH v2 1/4] drivers/iommu: Add page_ext for IOMMU_DEBUG_PAGEALLOC
On Mon, Nov 24, 2025 at 11:10 AM Mostafa Saleh <smostafa@...gle.com> wrote:
>
> On Thu, Nov 13, 2025 at 10:05:19AM +0000, Will Deacon wrote:
> > Hi Mostafa,
> >
> > On Thu, Nov 06, 2025 at 04:39:50PM +0000, Mostafa Saleh wrote:
> > > Add a new config IOMMU_DEBUG_PAGEALLOC, which registers new data to
> > > page_ext.
> > > This config will be used by the IOMMU API to track pages mapped in
> > > the IOMMU to catch drivers trying to free kernel memory that they
> > > still map in their domains, causing all types of memory corruption.
> > > This behaviour is disabled by default and can be enabled using
> > > kernel cmdline iommu.debug_pagealloc.
> > >
> > > Signed-off-by: Mostafa Saleh <smostafa@...gle.com>
> > > Tested-by: Qinxin Xia <xiaqinxin@...wei.com>
> > > ---
> > > .../admin-guide/kernel-parameters.txt | 6 ++++
> > > drivers/iommu/Kconfig | 15 +++++++++
> > > drivers/iommu/Makefile | 1 +
> > > drivers/iommu/iommu-debug-pagealloc.c | 32 +++++++++++++++++++
> > > include/linux/iommu-debug-pagealloc.h | 17 ++++++++++
> > > mm/page_ext.c | 4 +++
> > > 6 files changed, 75 insertions(+)
> > > create mode 100644 drivers/iommu/iommu-debug-pagealloc.c
> > > create mode 100644 include/linux/iommu-debug-pagealloc.h
> >
> > This looks like a pretty handy feature to me, but I have some nits below.
>
> Thanks for taking the time to review the patches!
>
> >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 6c42061ca20e..9a1c4ac8ba96 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -2557,6 +2557,12 @@
> > > 1 - Bypass the IOMMU for DMA.
> > > unset - Use value of CONFIG_IOMMU_DEFAULT_PASSTHROUGH.
> > >
> > > + iommu.debug_pagealloc=
> > > + [KNL,EARLY] When CONFIG_IOMMU_DEBUG_PAGEALLOC is set, this
> > > + parameter enables the feature at boot time. By default, it
> > > + is disabled and the system will work mostly the same as a
> > > + kernel built without CONFIG_IOMMU_DEBUG_PAGEALLOC.
> >
> > Can you be more specific about "mostly the same"?
>
> The only difference is that the static key to gate the calls, I was not sure if
> saying “exactly the same” is correct, but I think it’s better avoid “mostly” as
> it might be confusing and as the data in the cover letter shows no overhead,
> I will re-write the whole help anyway.
>
Ah, I think I copied the base of this from the documenation of
"debug_pagealloc="
which actually slightly changes system behviour by not using THP on some archs,
which is not relevant in our case, I will drop it then.
Thanks,
Mostafa
> >
> > > +
> > > io7= [HW] IO7 for Marvel-based Alpha systems
> > > See comment before marvel_specify_io7 in
> > > arch/alpha/kernel/core_marvel.c.
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index 70d29b14d851..6b5e9a2d936a 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -383,4 +383,19 @@ config SPRD_IOMMU
> > >
> > > Say Y here if you want to use the multimedia devices listed above.
> > >
> > > +config IOMMU_DEBUG_PAGEALLOC
> > > + bool "Debug page memory allocations against IOMMU"
> >
> > Perhaps "IOMMU mappings" would make this a little clearer?
>
> Will do.
>
> >
> > > + depends on DEBUG_PAGEALLOC && IOMMU_API && PAGE_EXTENSION
> > > + help
> > > + This config checks that a page is freed(unmapped) or mapped by the
> > > + kernel is not mapped in any IOMMU domain.
> >
> > I can't really parse this sentence :/
>
> I will re-write it.
>
> >
> > > It can help with debugging
> > > + use-after-free or out-of-bound maps from drivers doing DMA through
> > > + the IOMMU API.
> > > + This santaizer can have false-negative cases where some problems
> > > + won't be detected.
> >
> > Maybe just say "The sanitizer is best-effort and can fail to detect problems
> > in the case that ...".
>
> Makes sense, will do.
>
> >
> > > + Expect overhead when enabling this and enabling the kernel command
> > > + line iommu.debug_pagealloc.
> >
> > I'd reword this to say something like "Due to the overhead of the sanitiser,
> > iommu.debug_pagealloc must also be passed on the kernel command-line to
> > enable this feature".
>
> Will do.
>
> >
> > > +
> > > + If unsure, say N here.
> > > +
> > > endif # IOMMU_SUPPORT
> > > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> > > index 355294fa9033..8f5130b6a671 100644
> > > --- a/drivers/iommu/Makefile
> > > +++ b/drivers/iommu/Makefile
> > > @@ -34,3 +34,4 @@ obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
> > > obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
> > > obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
> > > obj-$(CONFIG_APPLE_DART) += apple-dart.o
> > > +obj-$(CONFIG_IOMMU_DEBUG_PAGEALLOC) += iommu-debug-pagealloc.o
> > > diff --git a/drivers/iommu/iommu-debug-pagealloc.c b/drivers/iommu/iommu-debug-pagealloc.c
> > > new file mode 100644
> > > index 000000000000..385c8bfae02b
> > > --- /dev/null
> > > +++ b/drivers/iommu/iommu-debug-pagealloc.c
> > > @@ -0,0 +1,32 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2025 - Google Inc
> > > + * Author: Mostafa Saleh <smostafa@...gle.com>
> > > + * IOMMU API debug page alloc sanitizer
> > > + */
> > > +#include <linux/atomic.h>
> > > +#include <linux/iommu-debug-pagealloc.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/page_ext.h>
> > > +
> > > +static bool needed;
> > > +
> > > +struct iommu_debug_metadate {
> > > + atomic_t ref;
> > > +};
> >
> > s/metadate/metadata/
>
> Ah, that's embarrassing, I will fix it.
>
> Thanks,
> Mostafa
>
> >
> > Will
Powered by blists - more mailing lists