linux-kernel - Re: [PATCH v2 1/4] drivers/iommu: Add page_ext for IOMMU_DEBUG

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aRWtX3n2I7El4Ykv@willie-the-truck>
Date: Thu, 13 Nov 2025 10:05:19 +0000
From: Will Deacon <will@...nel.org>
To: Mostafa Saleh <smostafa@...gle.com>
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

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.

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

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

> +	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 :/

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

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

> +
> +	  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/

Will