[PATCH v2 0/3] Restrict devmem for confidential VMs

[PATCH v2 0/3] Restrict devmem for confidential VMs

[Dan Williams]

Changes since v1 [1]:
* Fix the fact that devmem_is_allowed() == 2 does not prevent
  mmap access (Kees)
* Rather than teach devmem_is_allowed() == 2 to map zero pages in the
  mmap case, just fail (Nikolay)

[1]: http://lore.kernel.org/67f5b75c37143_71fe2949b@dwillia2-xfh.jf.intel.com.notmuch

---
The story starts with Nikolay reporting an SEPT violation due to
mismatched encrypted/non-encrypted mappings of the BIOS data space [2].

An initial suggestion to just make sure that the BIOS data space is
mapped consistently [3] ran into another issue that TDX and SEV-SNP
disagree about when that space can be mapped as encrypted.

Then, in response to a partial patch to allow SEV-SNP to block BIOS data
space for other reasons [4], Dave asked why not just give up on /dev/mem
access entirely in the confidential VM case [5].

Enter this series to:

1/ Close a subtle hole whereby /dev/mem that is supposed return zeros in
   lieu of access only enforces that for read()/write()

2/ Use that new closed hole to reliably disable all /dev/mem access for
   confidential x86 VMs

[2]: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com
[3]: http://lore.kernel.org/174346288005.2166708.14425674491111625620.stgit@dwillia2-xfh.jf.intel.com
[4]: http://lore.kernel.org/20250403120228.2344377-1-naveen@kernel.org
[5]: http://lore.kernel.org/fd683daa-d953-48ca-8c5d-6f4688ad442c@intel.com
---

Dan Williams (3):
      x86/devmem: Remove duplicate range_is_allowed() definition
      devmem: Block mmap access when read/write access is restricted
      x86/devmem: Restrict /dev/mem access for potentially unaccepted memory by default


 arch/x86/Kconfig                |    2 ++
 arch/x86/include/asm/x86_init.h |    2 ++
 arch/x86/kernel/x86_init.c      |    6 ++++++
 arch/x86/mm/init.c              |   23 +++++++++++++++++------
 arch/x86/mm/pat/memtype.c       |   31 ++++---------------------------
 drivers/char/mem.c              |   18 ------------------
 include/linux/io.h              |   26 ++++++++++++++++++++++++++
 7 files changed, 57 insertions(+), 51 deletions(-)

base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8

[PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Dan Williams]

It looks like x86 has a local re-implementation of range_is_allowed()
just to add a pat_enabled() check for the strong symbol override of
phys_mem_access_prot_allowed() from drivers/char/mem.c.

In preparation for updating range_is_allowed() logic, arrange for there
to be only one shared instance of "range_is_allowed()" in the kernel by
moving a common helper to include/linux/io.h.

Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/mm/pat/memtype.c |   31 ++++---------------------------
 drivers/char/mem.c        |   18 ------------------
 include/linux/io.h        |   21 +++++++++++++++++++++
 3 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 72d8cbc61158..c97b6598f187 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -38,6 +38,7 @@
 #include <linux/kernel.h>
 #include <linux/pfn_t.h>
 #include <linux/slab.h>
+#include <linux/io.h>
 #include <linux/mm.h>
 #include <linux/highmem.h>
 #include <linux/fs.h>
@@ -773,38 +774,14 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 	return vma_prot;
 }
 
-#ifdef CONFIG_STRICT_DEVMEM
-/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM */
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-	return 1;
-}
-#else
-/* This check is needed to avoid cache aliasing when PAT is enabled */
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-	u64 from = ((u64)pfn) << PAGE_SHIFT;
-	u64 to = from + size;
-	u64 cursor = from;
-
-	if (!pat_enabled())
-		return 1;
-
-	while (cursor < to) {
-		if (!devmem_is_allowed(pfn))
-			return 0;
-		cursor += PAGE_SIZE;
-		pfn++;
-	}
-	return 1;
-}
-#endif /* CONFIG_STRICT_DEVMEM */
-
 int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
 				unsigned long size, pgprot_t *vma_prot)
 {
 	enum page_cache_mode pcm = _PAGE_CACHE_MODE_WB;
 
+	if (!pat_enabled())
+		return 1;
+
 	if (!range_is_allowed(pfn, size))
 		return 0;
 
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 169eed162a7f..48839958b0b1 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -61,29 +61,11 @@ static inline int page_is_allowed(unsigned long pfn)
 {
 	return devmem_is_allowed(pfn);
 }
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-	u64 from = ((u64)pfn) << PAGE_SHIFT;
-	u64 to = from + size;
-	u64 cursor = from;
-
-	while (cursor < to) {
-		if (!devmem_is_allowed(pfn))
-			return 0;
-		cursor += PAGE_SIZE;
-		pfn++;
-	}
-	return 1;
-}
 #else
 static inline int page_is_allowed(unsigned long pfn)
 {
 	return 1;
 }
-static inline int range_is_allowed(unsigned long pfn, unsigned long size)
-{
-	return 1;
-}
 #endif
 
 static inline bool should_stop_iteration(void)
diff --git a/include/linux/io.h b/include/linux/io.h
index 6a6bc4d46d0a..0642c7ee41db 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -183,4 +183,25 @@ static inline void arch_io_free_memtype_wc(resource_size_t base,
 int devm_arch_io_reserve_memtype_wc(struct device *dev, resource_size_t start,
 				    resource_size_t size);
 
+#ifdef CONFIG_STRICT_DEVMEM
+static inline int range_is_allowed(unsigned long pfn, unsigned long size)
+{
+	u64 from = ((u64)pfn) << PAGE_SHIFT;
+	u64 to = from + size;
+	u64 cursor = from;
+
+	while (cursor < to) {
+		if (!devmem_is_allowed(pfn))
+			return 0;
+		cursor += PAGE_SIZE;
+		pfn++;
+	}
+	return 1;
+}
+#else
+static inline int range_is_allowed(unsigned long pfn, unsigned long size)
+{
+	return 1;
+}
+#endif
 #endif /* _LINUX_IO_H */

Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Naveen N Rao]

On Thu, Apr 10, 2025 at 06:22:23PM -0700, Dan Williams wrote:
> It looks like x86 has a local re-implementation of range_is_allowed()
> just to add a pat_enabled() check for the strong symbol override of
> phys_mem_access_prot_allowed() from drivers/char/mem.c.
> 
> In preparation for updating range_is_allowed() logic, arrange for there
> to be only one shared instance of "range_is_allowed()" in the kernel by
> moving a common helper to include/linux/io.h.
> 
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  arch/x86/mm/pat/memtype.c |   31 ++++---------------------------
>  drivers/char/mem.c        |   18 ------------------
>  include/linux/io.h        |   21 +++++++++++++++++++++
>  3 files changed, 25 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index 72d8cbc61158..c97b6598f187 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -38,6 +38,7 @@
>  #include <linux/kernel.h>
>  #include <linux/pfn_t.h>
>  #include <linux/slab.h>
> +#include <linux/io.h>
>  #include <linux/mm.h>
>  #include <linux/highmem.h>
>  #include <linux/fs.h>
> @@ -773,38 +774,14 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  	return vma_prot;
>  }
>  
> -#ifdef CONFIG_STRICT_DEVMEM
> -/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM */
> -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> -{
> -	return 1;
> -}

It looks like no checks were done here if CONFIG_STRICT_DEVMEM was set, 
so this patch changes that.

> -#else
> -/* This check is needed to avoid cache aliasing when PAT is enabled */
> -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> -{
> -	u64 from = ((u64)pfn) << PAGE_SHIFT;
> -	u64 to = from + size;
> -	u64 cursor = from;
> -
> -	if (!pat_enabled())
> -		return 1;
> -
> -	while (cursor < to) {
> -		if (!devmem_is_allowed(pfn))
> -			return 0;
> -		cursor += PAGE_SIZE;
> -		pfn++;
> -	}
> -	return 1;
> -}
> -#endif /* CONFIG_STRICT_DEVMEM */
> -
>  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
>  				unsigned long size, pgprot_t *vma_prot)
>  {
>  	enum page_cache_mode pcm = _PAGE_CACHE_MODE_WB;
>  
> +	if (!pat_enabled())
> +		return 1;
> +

Shouldn't this test for pat_enabled() (perhaps only if 
CONFIG_STRICT_DEVMEM is set) and continue with the rest of the function 
otherwise?


- Naveen

>  	if (!range_is_allowed(pfn, size))
>  		return 0;
>  
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 169eed162a7f..48839958b0b1 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -61,29 +61,11 @@ static inline int page_is_allowed(unsigned long pfn)
>  {
>  	return devmem_is_allowed(pfn);
>  }
> -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> -{
> -	u64 from = ((u64)pfn) << PAGE_SHIFT;
> -	u64 to = from + size;
> -	u64 cursor = from;
> -
> -	while (cursor < to) {
> -		if (!devmem_is_allowed(pfn))
> -			return 0;
> -		cursor += PAGE_SIZE;
> -		pfn++;
> -	}
> -	return 1;
> -}
>  #else
>  static inline int page_is_allowed(unsigned long pfn)
>  {
>  	return 1;
>  }
> -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> -{
> -	return 1;
> -}
>  #endif
>  
>  static inline bool should_stop_iteration(void)
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 6a6bc4d46d0a..0642c7ee41db 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -183,4 +183,25 @@ static inline void arch_io_free_memtype_wc(resource_size_t base,
>  int devm_arch_io_reserve_memtype_wc(struct device *dev, resource_size_t start,
>  				    resource_size_t size);
>  
> +#ifdef CONFIG_STRICT_DEVMEM
> +static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> +{
> +	u64 from = ((u64)pfn) << PAGE_SHIFT;
> +	u64 to = from + size;
> +	u64 cursor = from;
> +
> +	while (cursor < to) {
> +		if (!devmem_is_allowed(pfn))
> +			return 0;
> +		cursor += PAGE_SIZE;
> +		pfn++;
> +	}
> +	return 1;
> +}
> +#else
> +static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> +{
> +	return 1;
> +}
> +#endif
>  #endif /* _LINUX_IO_H */
> 

Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Dan Williams]

Naveen N Rao wrote:
> On Thu, Apr 10, 2025 at 06:22:23PM -0700, Dan Williams wrote:
> > It looks like x86 has a local re-implementation of range_is_allowed()
> > just to add a pat_enabled() check for the strong symbol override of
> > phys_mem_access_prot_allowed() from drivers/char/mem.c.
> > 
> > In preparation for updating range_is_allowed() logic, arrange for there
> > to be only one shared instance of "range_is_allowed()" in the kernel by
> > moving a common helper to include/linux/io.h.
> > 
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  arch/x86/mm/pat/memtype.c |   31 ++++---------------------------
> >  drivers/char/mem.c        |   18 ------------------
> >  include/linux/io.h        |   21 +++++++++++++++++++++
> >  3 files changed, 25 insertions(+), 45 deletions(-)
> > 
> > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> > index 72d8cbc61158..c97b6598f187 100644
> > --- a/arch/x86/mm/pat/memtype.c
> > +++ b/arch/x86/mm/pat/memtype.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/pfn_t.h>
> >  #include <linux/slab.h>
> > +#include <linux/io.h>
> >  #include <linux/mm.h>
> >  #include <linux/highmem.h>
> >  #include <linux/fs.h>
> > @@ -773,38 +774,14 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> >  	return vma_prot;
> >  }
> >  
> > -#ifdef CONFIG_STRICT_DEVMEM
> > -/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM */
> > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > -{
> > -	return 1;
> > -}
> 
> It looks like no checks were done here if CONFIG_STRICT_DEVMEM was set, 
> so this patch changes that.

Yes, but this still matches the historical intent, and the historical
intent is a tad messy.

The pat_enabled check was originally added as a *bypass* of additional
logic in phys_mem_access_prot_allowed() [1] to validate that /dev/mem was
establishing compatible mappings of "System-RAM" via /dev/mem. This
patch maintains that expectation that phys_mem_access_prot_allowed()
returns immediately when there is no potential cache conflict.

However, the point is moot in current code because [2] and [3] removed
all cache type validation from phys_mem_access_prot_allowed() in favor
track_pfn_remap().

According to:
Commit 9e41bff2708e ("x86: fix /dev/mem mmap breakage when PAT is disabled") [1]
Commit 1886297ce0c8 ("x86/mm/pat: Fix BUG_ON() in mmap_mem() on QEMU/i386") [2]
Commit 0c3c8a18361a ("x86, PAT: Remove duplicate memtype reserve in devmem mmap") [3]

> > -#else
> > -/* This check is needed to avoid cache aliasing when PAT is enabled */
> > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > -{
> > -	u64 from = ((u64)pfn) << PAGE_SHIFT;
> > -	u64 to = from + size;
> > -	u64 cursor = from;
> > -
> > -	if (!pat_enabled())
> > -		return 1;
> > -
> > -	while (cursor < to) {
> > -		if (!devmem_is_allowed(pfn))
> > -			return 0;
> > -		cursor += PAGE_SIZE;
> > -		pfn++;
> > -	}
> > -	return 1;
> > -}
> > -#endif /* CONFIG_STRICT_DEVMEM */
> > -
> >  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
> >  				unsigned long size, pgprot_t *vma_prot)
> >  {
> >  	enum page_cache_mode pcm = _PAGE_CACHE_MODE_WB;
> >  
> > +	if (!pat_enabled())
> > +		return 1;
> > +
> 
> Shouldn't this test for pat_enabled() (perhaps only if 
> CONFIG_STRICT_DEVMEM is set) and continue with the rest of the function 
> otherwise?

No because, per above, the check is here to short-circuit the rest of
phys_mem_access_prot_allowed() when PAT is disabled.

I will add some notes to the changelog to save the next person from
needing to find the history here.

I found it interesting that Venki suggested that the duplicated
"range_is_allowed()" be cleaned up back in 2008 [4], so this is a
cleanup 17 years (almost to the day) in the making:

Commit 0124cecfc85a ("x86, PAT: disable /dev/mem mmap RAM with PAT") [4]

Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Naveen N Rao]

On Wed, Apr 16, 2025 at 02:25:49PM -0700, Dan Williams wrote:
> Naveen N Rao wrote:
> > On Thu, Apr 10, 2025 at 06:22:23PM -0700, Dan Williams wrote:
> > > It looks like x86 has a local re-implementation of range_is_allowed()
> > > just to add a pat_enabled() check for the strong symbol override of
> > > phys_mem_access_prot_allowed() from drivers/char/mem.c.
> > > 
> > > In preparation for updating range_is_allowed() logic, arrange for there
> > > to be only one shared instance of "range_is_allowed()" in the kernel by
> > > moving a common helper to include/linux/io.h.
> > > 
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  arch/x86/mm/pat/memtype.c |   31 ++++---------------------------
> > >  drivers/char/mem.c        |   18 ------------------
> > >  include/linux/io.h        |   21 +++++++++++++++++++++
> > >  3 files changed, 25 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> > > index 72d8cbc61158..c97b6598f187 100644
> > > --- a/arch/x86/mm/pat/memtype.c
> > > +++ b/arch/x86/mm/pat/memtype.c
> > > @@ -38,6 +38,7 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/pfn_t.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/io.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/highmem.h>
> > >  #include <linux/fs.h>
> > > @@ -773,38 +774,14 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> > >  	return vma_prot;
> > >  }
> > >  
> > > -#ifdef CONFIG_STRICT_DEVMEM
> > > -/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM */
> > > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > > -{
> > > -	return 1;
> > > -}
> > 
> > It looks like no checks were done here if CONFIG_STRICT_DEVMEM was set, 
> > so this patch changes that.
> 
> Yes, but this still matches the historical intent, and the historical
> intent is a tad messy.
> 
> The pat_enabled check was originally added as a *bypass* of additional
> logic in phys_mem_access_prot_allowed() [1] to validate that /dev/mem was
> establishing compatible mappings of "System-RAM" via /dev/mem. This
> patch maintains that expectation that phys_mem_access_prot_allowed()
> returns immediately when there is no potential cache conflict.

Thanks for the background, that makes sense.

Do we also no longer need the devmem_is_allowed() checks in pat.c if PAT 
is enabled and !CONFIG_STRICT_DEVMEM?

> 
> However, the point is moot in current code because [2] and [3] removed
> all cache type validation from phys_mem_access_prot_allowed() in favor
> track_pfn_remap().
> 
> According to:
> Commit 9e41bff2708e ("x86: fix /dev/mem mmap breakage when PAT is disabled") [1]
> Commit 1886297ce0c8 ("x86/mm/pat: Fix BUG_ON() in mmap_mem() on QEMU/i386") [2]
> Commit 0c3c8a18361a ("x86, PAT: Remove duplicate memtype reserve in devmem mmap") [3]
> 
> > > -#else
> > > -/* This check is needed to avoid cache aliasing when PAT is enabled */
> > > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > > -{
> > > -	u64 from = ((u64)pfn) << PAGE_SHIFT;
> > > -	u64 to = from + size;
> > > -	u64 cursor = from;
> > > -
> > > -	if (!pat_enabled())
> > > -		return 1;
> > > -
> > > -	while (cursor < to) {
> > > -		if (!devmem_is_allowed(pfn))
> > > -			return 0;
> > > -		cursor += PAGE_SIZE;
> > > -		pfn++;
> > > -	}
> > > -	return 1;
> > > -}
> > > -#endif /* CONFIG_STRICT_DEVMEM */
> > > -
> > >  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
> > >  				unsigned long size, pgprot_t *vma_prot)
> > >  {
> > >  	enum page_cache_mode pcm = _PAGE_CACHE_MODE_WB;
> > >  
> > > +	if (!pat_enabled())
> > > +		return 1;
> > > +
> > 
> > Shouldn't this test for pat_enabled() (perhaps only if 
> > CONFIG_STRICT_DEVMEM is set) and continue with the rest of the function 
> > otherwise?
> 
> No because, per above, the check is here to short-circuit the rest of
> phys_mem_access_prot_allowed() when PAT is disabled.
> 
> I will add some notes to the changelog to save the next person from
> needing to find the history here.
> 
> I found it interesting that Venki suggested that the duplicated
> "range_is_allowed()" be cleaned up back in 2008 [4], so this is a
> cleanup 17 years (almost to the day) in the making:
> 
> Commit 0124cecfc85a ("x86, PAT: disable /dev/mem mmap RAM with PAT") [4]

Indeed!


Thanks,
Naveen

Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Dan Williams]

Naveen N Rao wrote:
[..]
> > The pat_enabled check was originally added as a *bypass* of additional
> > logic in phys_mem_access_prot_allowed() [1] to validate that /dev/mem was
> > establishing compatible mappings of "System-RAM" via /dev/mem. This
> > patch maintains that expectation that phys_mem_access_prot_allowed()
> > returns immediately when there is no potential cache conflict.
> 
> Thanks for the background, that makes sense.
> 
> Do we also no longer need the devmem_is_allowed() checks in pat.c if PAT 
> is enabled and !CONFIG_STRICT_DEVMEM?

The only one that is left is the one in phys_mem_access_prot_allowed()
and that one properly compiles away to nothing in the
!CONFIG_STRICT_DEVMEM case.

Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed() definition

[Naveen N Rao]

On Thu, Apr 17, 2025 at 11:27:02AM -0700, Dan Williams wrote:
> Naveen N Rao wrote:
> [..]
> > > The pat_enabled check was originally added as a *bypass* of additional
> > > logic in phys_mem_access_prot_allowed() [1] to validate that /dev/mem was
> > > establishing compatible mappings of "System-RAM" via /dev/mem. This
> > > patch maintains that expectation that phys_mem_access_prot_allowed()
> > > returns immediately when there is no potential cache conflict.
> > 
> > Thanks for the background, that makes sense.
> > 
> > Do we also no longer need the devmem_is_allowed() checks in pat.c if PAT 
> > is enabled and !CONFIG_STRICT_DEVMEM?
> 
> The only one that is left is the one in phys_mem_access_prot_allowed()
> and that one properly compiles away to nothing in the
> !CONFIG_STRICT_DEVMEM case.

I am probably missing something here, but that's the case I don't fully 
understand. Before this patch, it was not compiling away to nothing, and 
range_is_allowed() in phys_mem_access_prot_allowed() was calling out to 
devmem_is_allowed() when CONFIG_STRICT_DEVMEM was _not_ set.

I'll note that range_is_allowed() implementation in pat/memtype.c is 
exactly the inverse of that in drivers/char/mem.c, with respect to what 
checks were done with and without CONFIG_STRICT_DEVMEM.


Thanks,
Naveen

[PATCH v2 2/3] devmem: Block mmap access when read/write access is restricted

[Dan Williams]

Back in 2022 Kees noted that he is able to mmap System RAM below 1MB
even with CONFIG_STRICT_DEVMEM=y [1]. That is allowed for x86 legacy
compatibility reasons for userspace that wants to read BIOS data
resident at that address. However, the expectation is that when
devmem_is_allowed() returns 2 that the access is redirected to return
zeroes.

That happens for the read()/write() case, but by code inspection for
mmap(), there is no restriction.

Now, the confidential x86 VM (CVM) use case wants to depend on
"devmem_is_allowed() == 2" guaranteeing that no mapping to potentially
encrypted memory is established [2]. The options to enable that are
teach mmap_mem() to meet the "zeroed buffer" implication of
devmem_is_allowed() returning "2", or return -EPERM for that case.

Return -EPERM on the hope that userspace does not actually depend on the
legacy behavior of being able to reliably map the first 1MB of memory on
x86. I.e. that all legacy cases are using read()/write() to safely read
zeroes. If that turns out not to be true then either a "map zeroes"
scheme can be added, or the CVM case can return 3 from
devmem_is_allowed() to hide the CVM restriction from legacy
environments.

Link: http://lore.kernel.org/CAPcyv4iVt=peUAk1qx_EfKn7aGJM=XwRUpJftBhkUgQEti2bJA@mail.gmail.com [1]
Link: http://lore.kernel.org/fd683daa-d953-48ca-8c5d-6f4688ad442c@intel.com [2]
Suggested-by: Nikolay Borisov <nik.borisov@suse.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/io.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/io.h b/include/linux/io.h
index 0642c7ee41db..564934f7e70d 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -191,7 +191,12 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 	u64 cursor = from;
 
 	while (cursor < to) {
-		if (!devmem_is_allowed(pfn))
+		/*
+		 * Any restricted access is treated as "no access", i.e.
+		 * handle devmem_is_allowed() returning "2" to indicate
+		 * restricted access.
+		 */
+		if (devmem_is_allowed(pfn) != 1)
 			return 0;
 		cursor += PAGE_SIZE;
 		pfn++;

Re: [PATCH v2 2/3] devmem: Block mmap access when read/write access is restricted

[Kees Cook]

On Thu, Apr 10, 2025 at 06:22:30PM -0700, Dan Williams wrote:
> Back in 2022 Kees noted that he is able to mmap System RAM below 1MB
> even with CONFIG_STRICT_DEVMEM=y [1]. That is allowed for x86 legacy
> compatibility reasons for userspace that wants to read BIOS data
> resident at that address. However, the expectation is that when
> devmem_is_allowed() returns 2 that the access is redirected to return
> zeroes.
> 
> That happens for the read()/write() case, but by code inspection for
> mmap(), there is no restriction.
> 
> Now, the confidential x86 VM (CVM) use case wants to depend on
> "devmem_is_allowed() == 2" guaranteeing that no mapping to potentially
> encrypted memory is established [2]. The options to enable that are
> teach mmap_mem() to meet the "zeroed buffer" implication of
> devmem_is_allowed() returning "2", or return -EPERM for that case.
> 
> Return -EPERM on the hope that userspace does not actually depend on the
> legacy behavior of being able to reliably map the first 1MB of memory on
> x86. I.e. that all legacy cases are using read()/write() to safely read
> zeroes. If that turns out not to be true then either a "map zeroes"
> scheme can be added, or the CVM case can return 3 from
> devmem_is_allowed() to hide the CVM restriction from legacy
> environments.
> 
> Link: http://lore.kernel.org/CAPcyv4iVt=peUAk1qx_EfKn7aGJM=XwRUpJftBhkUgQEti2bJA@mail.gmail.com [1]
> Link: http://lore.kernel.org/fd683daa-d953-48ca-8c5d-6f4688ad442c@intel.com [2]
> Suggested-by: Nikolay Borisov <nik.borisov@suse.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  include/linux/io.h |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 0642c7ee41db..564934f7e70d 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -191,7 +191,12 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
>  	u64 cursor = from;
>  
>  	while (cursor < to) {
> -		if (!devmem_is_allowed(pfn))
> +		/*
> +		 * Any restricted access is treated as "no access", i.e.
> +		 * handle devmem_is_allowed() returning "2" to indicate
> +		 * restricted access.
> +		 */
> +		if (devmem_is_allowed(pfn) != 1)
>  			return 0;
>  		cursor += PAGE_SIZE;
>  		pfn++;

Looking through the 16 page of Debian Code Search results for
`open("/dev/mem")`, I find a LOT of mmap() use. Some random examples:

https://sources.debian.org/src/i810switch/0.6.5-7.1/i810switch.c/?hl=413#L402

https://sources.debian.org/src/radeontop/1.4-2/detect.c/?hl=91#L88

https://sources.debian.org/src/libdebian-installer/0.125/src/system/subarch-x86-linux.c/?hl=113#L93
Which includes this gem of a comment, implying that it uses mmap
_specifically to bypass the devmem restrictions_:
	/* Please note that we don't use mmap() for performance reasons here,
	 * but to workaround problems many people encountered when trying
	 * to read from /dev/mem using regular read() calls.
	 */

I don't think we can just fail the mmap. :(

-- 
Kees Cook

Re: [PATCH v2 2/3] devmem: Block mmap access when read/write access is restricted

[Dan Williams]

Kees Cook wrote:
> On Thu, Apr 10, 2025 at 06:22:30PM -0700, Dan Williams wrote:
> > Back in 2022 Kees noted that he is able to mmap System RAM below 1MB
> > even with CONFIG_STRICT_DEVMEM=y [1]. That is allowed for x86 legacy
> > compatibility reasons for userspace that wants to read BIOS data
> > resident at that address. However, the expectation is that when
> > devmem_is_allowed() returns 2 that the access is redirected to return
> > zeroes.
> > 
> > That happens for the read()/write() case, but by code inspection for
> > mmap(), there is no restriction.
> > 
> > Now, the confidential x86 VM (CVM) use case wants to depend on
> > "devmem_is_allowed() == 2" guaranteeing that no mapping to potentially
> > encrypted memory is established [2]. The options to enable that are
> > teach mmap_mem() to meet the "zeroed buffer" implication of
> > devmem_is_allowed() returning "2", or return -EPERM for that case.
> > 
> > Return -EPERM on the hope that userspace does not actually depend on the
> > legacy behavior of being able to reliably map the first 1MB of memory on
> > x86. I.e. that all legacy cases are using read()/write() to safely read
> > zeroes. If that turns out not to be true then either a "map zeroes"
> > scheme can be added, or the CVM case can return 3 from
> > devmem_is_allowed() to hide the CVM restriction from legacy
> > environments.
> > 
> > Link: http://lore.kernel.org/CAPcyv4iVt=peUAk1qx_EfKn7aGJM=XwRUpJftBhkUgQEti2bJA@mail.gmail.com [1]
> > Link: http://lore.kernel.org/fd683daa-d953-48ca-8c5d-6f4688ad442c@intel.com [2]
> > Suggested-by: Nikolay Borisov <nik.borisov@suse.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  include/linux/io.h |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/io.h b/include/linux/io.h
> > index 0642c7ee41db..564934f7e70d 100644
> > --- a/include/linux/io.h
> > +++ b/include/linux/io.h
> > @@ -191,7 +191,12 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> >  	u64 cursor = from;
> >  
> >  	while (cursor < to) {
> > -		if (!devmem_is_allowed(pfn))
> > +		/*
> > +		 * Any restricted access is treated as "no access", i.e.
> > +		 * handle devmem_is_allowed() returning "2" to indicate
> > +		 * restricted access.
> > +		 */
> > +		if (devmem_is_allowed(pfn) != 1)
> >  			return 0;
> >  		cursor += PAGE_SIZE;
> >  		pfn++;
> 
> Looking through the 16 page of Debian Code Search results for
> `open("/dev/mem")`, I find a LOT of mmap() use. Some random examples:
> 
> https://sources.debian.org/src/i810switch/0.6.5-7.1/i810switch.c/?hl=413#L402
> 
> https://sources.debian.org/src/radeontop/1.4-2/detect.c/?hl=91#L88
> 
> https://sources.debian.org/src/libdebian-installer/0.125/src/system/subarch-x86-linux.c/?hl=113#L93
> Which includes this gem of a comment, implying that it uses mmap
> _specifically to bypass the devmem restrictions_:

Oh.

> 	/* Please note that we don't use mmap() for performance reasons here,
> 	 * but to workaround problems many people encountered when trying
> 	 * to read from /dev/mem using regular read() calls.
> 	 */

Looks like in that case it is trying to grab DMI platform information
and otherwise fallback to a "generic" system which I would expect wreaks
havoc for many people.

> I don't think we can just fail the mmap. :(

For the TVM case the havoc of failing mmap for DMI info is smaller and
the recommended fallback for /dev/mem being in accessible is
/sys/firmware/dmi/tables. So I feel ok making TVMs take the modern
replacement path which is what they would need to do anyway in the
lockdown_kernel case. Tom, Dave, what do you think?

...but yes, let's skip taking away the /dev/mem backdoor for the bare
metal case.

Re: [PATCH v2 2/3] devmem: Block mmap access when read/write access is restricted

[Dave Hansen]

On 4/10/25 21:59, Dan Williams wrote:
>> I don't think we can just fail the mmap. 🙁
> For the TVM case the havoc of failing mmap for DMI info is smaller and
> the recommended fallback for /dev/mem being in accessible is
> /sys/firmware/dmi/tables. So I feel ok making TVMs take the modern
> replacement path which is what they would need to do anyway in the
> lockdown_kernel case. Tom, Dave, what do you think?

Yeah, doing the same as lockdown should be fine.

The other alternative to failing all mmap()s is to allow mmap(PROT_READ)
to succeed and then just map the zero page.

Re: [PATCH v2 2/3] devmem: Block mmap access when read/write access is restricted

[Dan Williams]

Dave Hansen wrote:
> On 4/10/25 21:59, Dan Williams wrote:
> >> I don't think we can just fail the mmap. 🙁
> > For the TVM case the havoc of failing mmap for DMI info is smaller and
> > the recommended fallback for /dev/mem being in accessible is
> > /sys/firmware/dmi/tables. So I feel ok making TVMs take the modern
> > replacement path which is what they would need to do anyway in the
> > lockdown_kernel case. Tom, Dave, what do you think?
> 
> Yeah, doing the same as lockdown should be fine.

Note that lockdown fails the open() for /dev/mem and fails the mmap()
for PCI sysfs. So the proposal here would arrange for lockdown to not be
required, but fail mmap() in both cases.

> The other alternative to failing all mmap()s is to allow mmap(PROT_READ)
> to succeed and then just map the zero page.

The only goal of mapping zeroes I can see is to attempt to break legacy
userspace less severely, but as the Debian code search shows, legacy
/dev/mem users have already found the mmap() loophole. So, zeroes for
the TVM case does not help satisfy the requirement to use
/sys/firmware/dmi/tables and other modern methods with
private-memory-safe semantics. I.e. the "success but zero" and "mmap()
failed" cases have the same outcome: legacy software falls back to no
DMI info.

For PCI sysfs resource mmap() the semantics are different. mmap() fails
only when devmem_is_allowed() says "no" *and* the kernel has marked the
range as IORESOURCE_BUSY in the iomem resource tree. That should be
sufficient to allow userpsace PCI drivers in TVMs because the goal here
is to avoid simultaneous mappings with mismatched encryption settings or
allowing userspace access to unaccepted private memory.

As long as the upcoming TDISP code is careful to hold a
request_resource() reservation over attempts to convert PCI MMIO from
shared to private, it should close any potential for mismatched
encryption settings, or access to unaccepted private MMIO.

However to get that behavior of not allowing simultaneous
kernel-ioremap() plus userspace mmap() of a PCI resource the kernel
needs to be built with CONFIG_IO_STRICT_DEVMEM=y. I note, for example
that RHEL does not set that, but Fedora does.

[PATCH v2 3/3] x86/devmem: Restrict /dev/mem access for potentially unaccepted memory by default

[Dan Williams]

Nikolay reports [1] that accessing BIOS data (first 1MB of the physical
address space) via /dev/mem results in an SEPT violation.

The cause is ioremap() (via xlate_dev_mem_ptr()) establishes an
unencrypted mapping where the kernel had established an encrypted
mapping previously.

An initial attempt to fix this revealed that TDX and SEV-SNP have
different expectations about which and when address ranges can be mapped
via /dev/mem.

Rather than develop a precise set of allowed /dev/mem capable TVM
address ranges, teach devmem_is_allowed() to always restrict access to
the BIOS data space. This means return 0s for read(), drop write(), and
-EPERM mmap(). This can still be later relaxed as specific needs arise,
but in the meantime, close off this source of mismatched
IORES_MAP_ENCRYPTED expectations.

Cc: <x86@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Vishal Annapurve <vannapurve@google.com>
Cc: Kirill Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Nikolay Borisov <nik.borisov@suse.com>
Closes: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com [1]
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Fixes: 9aa6ea69852c ("x86/tdx: Make pages shared in ioremap()")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/Kconfig                |    2 ++
 arch/x86/include/asm/x86_init.h |    2 ++
 arch/x86/kernel/x86_init.c      |    6 ++++++
 arch/x86/mm/init.c              |   23 +++++++++++++++++------
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4b9f378e05f6..12a1b5acd55b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -891,6 +891,7 @@ config INTEL_TDX_GUEST
 	depends on X86_X2APIC
 	depends on EFI_STUB
 	depends on PARAVIRT
+	depends on STRICT_DEVMEM
 	select ARCH_HAS_CC_PLATFORM
 	select X86_MEM_ENCRYPT
 	select X86_MCE
@@ -1510,6 +1511,7 @@ config AMD_MEM_ENCRYPT
 	bool "AMD Secure Memory Encryption (SME) support"
 	depends on X86_64 && CPU_SUP_AMD
 	depends on EFI_STUB
+	depends on STRICT_DEVMEM
 	select DMA_COHERENT_POOL
 	select ARCH_USE_MEMREMAP_PROT
 	select INSTRUCTION_DECODER
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 213cf5379a5a..0ae436b34b88 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -305,6 +305,7 @@ struct x86_hyper_runtime {
  * 				semantics.
  * @realmode_reserve:		reserve memory for realmode trampoline
  * @realmode_init:		initialize realmode trampoline
+ * @devmem_is_allowed		restrict /dev/mem and PCI sysfs resource access
  * @hyper:			x86 hypervisor specific runtime callbacks
  */
 struct x86_platform_ops {
@@ -323,6 +324,7 @@ struct x86_platform_ops {
 	void (*set_legacy_features)(void);
 	void (*realmode_reserve)(void);
 	void (*realmode_init)(void);
+	bool (*devmem_is_allowed)(unsigned long pfn);
 	struct x86_hyper_runtime hyper;
 	struct x86_guest guest;
 };
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 0a2bbd674a6d..346301375bd4 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -143,6 +143,11 @@ static void enc_kexec_begin_noop(void) {}
 static void enc_kexec_finish_noop(void) {}
 static bool is_private_mmio_noop(u64 addr) {return false; }
 
+static bool platform_devmem_is_allowed(unsigned long pfn)
+{
+	return !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
+}
+
 struct x86_platform_ops x86_platform __ro_after_init = {
 	.calibrate_cpu			= native_calibrate_cpu_early,
 	.calibrate_tsc			= native_calibrate_tsc,
@@ -156,6 +161,7 @@ struct x86_platform_ops x86_platform __ro_after_init = {
 	.restore_sched_clock_state	= tsc_restore_sched_clock_state,
 	.realmode_reserve		= reserve_real_mode,
 	.realmode_init			= init_real_mode,
+	.devmem_is_allowed		= platform_devmem_is_allowed,
 	.hyper.pin_vcpu			= x86_op_int_noop,
 	.hyper.is_private_mmio		= is_private_mmio_noop,
 
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index bfa444a7dbb0..df5435c8dbea 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -861,18 +861,23 @@ void __init poking_init(void)
  * area traditionally contains BIOS code and data regions used by X, dosemu,
  * and similar apps. Since they map the entire memory range, the whole range
  * must be allowed (for mapping), but any areas that would otherwise be
- * disallowed are flagged as being "zero filled" instead of rejected.
+ * disallowed are flagged as being "zero filled" instead of rejected, for
+ * read()/write().
+ *
  * Access has to be given to non-kernel-ram areas as well, these contain the
  * PCI mmio resources as well as potential bios/acpi data regions.
  */
 int devmem_is_allowed(unsigned long pagenr)
 {
+	bool platform_allowed = x86_platform.devmem_is_allowed(pagenr);
+
 	if (region_intersects(PFN_PHYS(pagenr), PAGE_SIZE,
 				IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)
 			!= REGION_DISJOINT) {
 		/*
-		 * For disallowed memory regions in the low 1MB range,
-		 * request that the page be shown as all zeros.
+		 * For disallowed memory regions in the low 1MB range, request
+		 * that the page be shown as all zeros for read()/write(), fail
+		 * mmap()
 		 */
 		if (pagenr < 256)
 			return 2;
@@ -885,14 +890,20 @@ int devmem_is_allowed(unsigned long pagenr)
 	 * restricted resource under CONFIG_STRICT_DEVMEM.
 	 */
 	if (iomem_is_exclusive(pagenr << PAGE_SHIFT)) {
-		/* Low 1MB bypasses iomem restrictions. */
-		if (pagenr < 256)
+		/*
+		 * Low 1MB bypasses iomem restrictions unless the platform says
+		 * the physical address is not suitable for direct access.
+		 */
+		if (pagenr < 256) {
+			if (!platform_allowed)
+				return 2;
 			return 1;
+		}
 
 		return 0;
 	}
 
-	return 1;
+	return platform_allowed;
 }
 
 void free_init_pages(const char *what, unsigned long begin, unsigned long end)

Re: [PATCH v2 3/3] x86/devmem: Restrict /dev/mem access for potentially unaccepted memory by default

[Naveen N Rao]

On Thu, Apr 10, 2025 at 06:22:38PM -0700, Dan Williams wrote:
> Nikolay reports [1] that accessing BIOS data (first 1MB of the physical
> address space) via /dev/mem results in an SEPT violation.
> 
> The cause is ioremap() (via xlate_dev_mem_ptr()) establishes an
> unencrypted mapping where the kernel had established an encrypted
> mapping previously.
> 
> An initial attempt to fix this revealed that TDX and SEV-SNP have
> different expectations about which and when address ranges can be mapped
> via /dev/mem.
> 
> Rather than develop a precise set of allowed /dev/mem capable TVM
> address ranges, teach devmem_is_allowed() to always restrict access to
> the BIOS data space.

This patch does more than just restrict the BIOS data space - it rejects 
all accesses to /dev/mem _apart_ from the first 1MB. That should be made 
clear here.

> This means return 0s for read(), drop write(), and
> -EPERM mmap(). This can still be later relaxed as specific needs arise,
> but in the meantime, close off this source of mismatched
> IORES_MAP_ENCRYPTED expectations.
> 
> Cc: <x86@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Vishal Annapurve <vannapurve@google.com>
> Cc: Kirill Shutemov <kirill.shutemov@linux.intel.com>
> Reported-by: Nikolay Borisov <nik.borisov@suse.com>
> Closes: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com [1]
> Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
> Fixes: 9aa6ea69852c ("x86/tdx: Make pages shared in ioremap()")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  arch/x86/Kconfig                |    2 ++
>  arch/x86/include/asm/x86_init.h |    2 ++
>  arch/x86/kernel/x86_init.c      |    6 ++++++
>  arch/x86/mm/init.c              |   23 +++++++++++++++++------
>  4 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4b9f378e05f6..12a1b5acd55b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -891,6 +891,7 @@ config INTEL_TDX_GUEST
>  	depends on X86_X2APIC
>  	depends on EFI_STUB
>  	depends on PARAVIRT
> +	depends on STRICT_DEVMEM
>  	select ARCH_HAS_CC_PLATFORM
>  	select X86_MEM_ENCRYPT
>  	select X86_MCE
> @@ -1510,6 +1511,7 @@ config AMD_MEM_ENCRYPT
>  	bool "AMD Secure Memory Encryption (SME) support"
>  	depends on X86_64 && CPU_SUP_AMD
>  	depends on EFI_STUB
> +	depends on STRICT_DEVMEM
>  	select DMA_COHERENT_POOL
>  	select ARCH_USE_MEMREMAP_PROT
>  	select INSTRUCTION_DECODER
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 213cf5379a5a..0ae436b34b88 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -305,6 +305,7 @@ struct x86_hyper_runtime {
>   * 				semantics.
>   * @realmode_reserve:		reserve memory for realmode trampoline
>   * @realmode_init:		initialize realmode trampoline
> + * @devmem_is_allowed		restrict /dev/mem and PCI sysfs resource access
>   * @hyper:			x86 hypervisor specific runtime callbacks
>   */
>  struct x86_platform_ops {
> @@ -323,6 +324,7 @@ struct x86_platform_ops {
>  	void (*set_legacy_features)(void);
>  	void (*realmode_reserve)(void);
>  	void (*realmode_init)(void);
> +	bool (*devmem_is_allowed)(unsigned long pfn);
>  	struct x86_hyper_runtime hyper;
>  	struct x86_guest guest;
>  };
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 0a2bbd674a6d..346301375bd4 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -143,6 +143,11 @@ static void enc_kexec_begin_noop(void) {}
>  static void enc_kexec_finish_noop(void) {}
>  static bool is_private_mmio_noop(u64 addr) {return false; }
>  
> +static bool platform_devmem_is_allowed(unsigned long pfn)
> +{
> +	return !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> +}
> +
>  struct x86_platform_ops x86_platform __ro_after_init = {
>  	.calibrate_cpu			= native_calibrate_cpu_early,
>  	.calibrate_tsc			= native_calibrate_tsc,
> @@ -156,6 +161,7 @@ struct x86_platform_ops x86_platform __ro_after_init = {
>  	.restore_sched_clock_state	= tsc_restore_sched_clock_state,
>  	.realmode_reserve		= reserve_real_mode,
>  	.realmode_init			= init_real_mode,
> +	.devmem_is_allowed		= platform_devmem_is_allowed,
>  	.hyper.pin_vcpu			= x86_op_int_noop,
>  	.hyper.is_private_mmio		= is_private_mmio_noop,
>  
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index bfa444a7dbb0..df5435c8dbea 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -861,18 +861,23 @@ void __init poking_init(void)
>   * area traditionally contains BIOS code and data regions used by X, dosemu,
>   * and similar apps. Since they map the entire memory range, the whole range
>   * must be allowed (for mapping), but any areas that would otherwise be
> - * disallowed are flagged as being "zero filled" instead of rejected.
> + * disallowed are flagged as being "zero filled" instead of rejected, for
> + * read()/write().
> + *
>   * Access has to be given to non-kernel-ram areas as well, these contain the
>   * PCI mmio resources as well as potential bios/acpi data regions.
>   */
>  int devmem_is_allowed(unsigned long pagenr)
>  {
> +	bool platform_allowed = x86_platform.devmem_is_allowed(pagenr);
> +

If we are going to do this, I don't see the point of having an 
x86_platform_op. It may be better to simply gate this on  
cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) directly here.


Thanks,
Naveen

>  	if (region_intersects(PFN_PHYS(pagenr), PAGE_SIZE,
>  				IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)
>  			!= REGION_DISJOINT) {
>  		/*
> -		 * For disallowed memory regions in the low 1MB range,
> -		 * request that the page be shown as all zeros.
> +		 * For disallowed memory regions in the low 1MB range, request
> +		 * that the page be shown as all zeros for read()/write(), fail
> +		 * mmap()
>  		 */
>  		if (pagenr < 256)
>  			return 2;
> @@ -885,14 +890,20 @@ int devmem_is_allowed(unsigned long pagenr)
>  	 * restricted resource under CONFIG_STRICT_DEVMEM.
>  	 */
>  	if (iomem_is_exclusive(pagenr << PAGE_SHIFT)) {
> -		/* Low 1MB bypasses iomem restrictions. */
> -		if (pagenr < 256)
> +		/*
> +		 * Low 1MB bypasses iomem restrictions unless the platform says
> +		 * the physical address is not suitable for direct access.
> +		 */
> +		if (pagenr < 256) {
> +			if (!platform_allowed)
> +				return 2;
>  			return 1;
> +		}
>  
>  		return 0;
>  	}
>  
> -	return 1;
> +	return platform_allowed;
>  }
>  
>  void free_init_pages(const char *what, unsigned long begin, unsigned long end)
> 

Re: [PATCH v2 3/3] x86/devmem: Restrict /dev/mem access for potentially unaccepted memory by default

[Dan Williams]

Naveen N Rao wrote:
> On Thu, Apr 10, 2025 at 06:22:38PM -0700, Dan Williams wrote:
> > Nikolay reports [1] that accessing BIOS data (first 1MB of the physical
> > address space) via /dev/mem results in an SEPT violation.
> > 
> > The cause is ioremap() (via xlate_dev_mem_ptr()) establishes an
> > unencrypted mapping where the kernel had established an encrypted
> > mapping previously.
> > 
> > An initial attempt to fix this revealed that TDX and SEV-SNP have
> > different expectations about which and when address ranges can be mapped
> > via /dev/mem.
> > 
> > Rather than develop a precise set of allowed /dev/mem capable TVM
> > address ranges, teach devmem_is_allowed() to always restrict access to
> > the BIOS data space.
> 
> This patch does more than just restrict the BIOS data space - it rejects 
> all accesses to /dev/mem _apart_ from the first 1MB. That should be made 
> clear here.
> 

Agree, and per the follow on conversation [1] even that low 1MB access to
return zeroes is not helpful. Confidential Computing userspace should
drop its dependency on interfaces that are difficult to make compatible
with consistent encryption policy and acceptance status.

http://lore.kernel.org/67f98e27a799e_7205294e3@dwillia2-xfh.jf.intel.com.notmuch [1]

[..]
> >  int devmem_is_allowed(unsigned long pagenr)
> >  {
> > +	bool platform_allowed = x86_platform.devmem_is_allowed(pagenr);
> > +
> 
> If we are going to do this, I don't see the point of having an 
> x86_platform_op. It may be better to simply gate this on  
> cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) directly here.

That is fair, no point in premature flexibility.

Re: [PATCH v2 0/3] Restrict devmem for confidential VMs

[Nikolay Borisov]

On 11.04.25 г. 4:22 ч., Dan Williams wrote:
> Changes since v1 [1]:
> * Fix the fact that devmem_is_allowed() == 2 does not prevent
>    mmap access (Kees)
> * Rather than teach devmem_is_allowed() == 2 to map zero pages in the
>    mmap case, just fail (Nikolay)
> 
> [1]: http://lore.kernel.org/67f5b75c37143_71fe2949b@dwillia2-xfh.jf.intel.com.notmuch
> 
> ---
> The story starts with Nikolay reporting an SEPT violation due to
> mismatched encrypted/non-encrypted mappings of the BIOS data space [2].
> 
> An initial suggestion to just make sure that the BIOS data space is
> mapped consistently [3] ran into another issue that TDX and SEV-SNP
> disagree about when that space can be mapped as encrypted.
> 
> Then, in response to a partial patch to allow SEV-SNP to block BIOS data
> space for other reasons [4], Dave asked why not just give up on /dev/mem
> access entirely in the confidential VM case [5].
> 
> Enter this series to:
> 
> 1/ Close a subtle hole whereby /dev/mem that is supposed return zeros in
>     lieu of access only enforces that for read()/write()
> 
> 2/ Use that new closed hole to reliably disable all /dev/mem access for
>     confidential x86 VMs
> 
> [2]: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com
> [3]: http://lore.kernel.org/174346288005.2166708.14425674491111625620.stgit@dwillia2-xfh.jf.intel.com
> [4]: http://lore.kernel.org/20250403120228.2344377-1-naveen@kernel.org
> [5]: http://lore.kernel.org/fd683daa-d953-48ca-8c5d-6f4688ad442c@intel.com
> ---
> 
> Dan Williams (3):
>        x86/devmem: Remove duplicate range_is_allowed() definition
>        devmem: Block mmap access when read/write access is restricted
>        x86/devmem: Restrict /dev/mem access for potentially unaccepted memory by default
> 
> 
>   arch/x86/Kconfig                |    2 ++
>   arch/x86/include/asm/x86_init.h |    2 ++
>   arch/x86/kernel/x86_init.c      |    6 ++++++
>   arch/x86/mm/init.c              |   23 +++++++++++++++++------
>   arch/x86/mm/pat/memtype.c       |   31 ++++---------------------------
>   drivers/char/mem.c              |   18 ------------------
>   include/linux/io.h              |   26 ++++++++++++++++++++++++++
>   7 files changed, 57 insertions(+), 51 deletions(-)
> 
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>